New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 8 users

Issue metadata

Status: WontFix
Owner: ----
Closed: Mar 2018
Area: ----
NextAction: ----
Priority: Low
Type: Idea

Sign in to add a comment

Issue 431: Handling of SkPath* return parameters is inconsistent

Reported by, Jan 3 2012 Project Member

Issue description

Some classes call SkPath::reset() or rewind() before writing into a SkPath parameter; others do not, appending instead of overwriting.

This is distributed throughout the interface (publicly: SkPathEffect, SkPaint, SkPath, SkPathMeasure, SkRegion, SkScalerContext, SkStroke; presumably others internally). Is it worth the effort to make it consistent?

(inspired by

Comment 1 by, Aug 20 2014

Project Member
Labels: -Type-Review -Priority-Low Type-Idea Priority-Icebox
Seems we are not getting to this one- do our best to pick a best practice, share with the team, and implement consistently moving fwd?

Comment 2 by, Aug 20 2014

Project Member
Labels: -Priority-Icebox Priority-Low

Comment 3 by, Dec 7 2015

Project Member
Labels: Hotlist-Fixit

Comment 4 by, Feb 25 2016

Project Member
Proposal: change the documentation of all public interfaces that append to the path  that they do so.

Alternatively: Document that these functions expect empty paths as receptacles and  assert if the path is not empty.

Comment 5 by, Mar 2 2016

Project Member
Some concrete examples:

     *  Returns true if the region is non-empty, and if so, appends the
     *  boundary(s) of the region to the specified path.
     *  If the region is empty, returns false, and path is left unmodified.
    bool SkRegion::getBoundaryPath(SkPath* path) const;

     *  Applies any/all effects (patheffect, stroking) to src, returning the
     *  result in dst. The result is that drawing src with this paint will be
     *  the same as drawing dst with a default paint (at least from the
     *  geometric perspective).
     *  @param src  input path
     *  @param dst  output path (may be the same as src)
     *  @param cullRect If not null, the dst path may be culled to this rect.
     *  @param resScale If > 1, increase precision, else if (0 < res < 1) reduce precision
     *              in favor of speed/size.
     *  @return     true if the path should be filled, or false if it should be
     *              drawn with a hairline (width == 0)
    bool SkPaint::getFillPath(const SkPath& src, SkPath* dst, const SkRect* cullRect,
                     SkScalar resScale = 1) const;

     *  Given a src path (input) and a stroke-rec (input and output), apply
     *  this effect to the src path, returning the new path in dst, and return
     *  true. If this effect cannot be applied, return false and ignore dst
     *  and stroke-rec.
     *  The stroke-rec specifies the initial request for stroking (if any).
     *  The effect can treat this as input only, or it can choose to change
     *  the rec as well. For example, the effect can decide to change the
     *  stroke's width or join, or the effect can change the rec from stroke
     *  to fill (or fill to stroke) in addition to returning a new (dst) path.
     *  If this method returns true, the caller will apply (as needed) the
     *  resulting stroke-rec to dst and then draw.
    virtual bool SkPathEffect::filterPath(SkPath* dst, const SkPath& src,
                            SkStrokeRec*, const SkRect* cullR) const = 0;

Comment 6 by, Mar 2 2016

Project Member
Seems like we have some options:

- sometimes append may be fine, but perhaps not always...
    - filterPath seems (to me) like it should "create" its result, and not append it

Perhaps we could ask ourselves
A) when might the API be reorg'd to just return its path?
B) when is it useful to pass in the dst?

In the (A) case, perhaps we should assert that the dst is already empty, so it *could* be reformulated as a return value.

In the (B) case, lets see if its interesting/useful to pass in a non-empty path and append to it.

Of course, we could always require empty, and force the caller to manually append afterwards if they wished...

Comment 7 by, Mar 6 2018

Project Member
Owner: ----

Comment 8 by, Mar 8 2018

Project Member
Status: WontFix (was: New)

Sign in to add a comment