SKRect constructor with width/height broken

[joreg]

according to https://developer.xamarin.com/api/member/SkiaSharp.SKRect.Create/p/System.Single/System.Single/System.Single/System.Single/

this constructor should take: x, y, width, height
but apparently it is more like: left, top, right, bottom#

(using libskia_windows.dll x86)

[migueldeicaza]

I agree, i was writing this documentation today, and I found this to be confusing.

[mattleibow]

@migueldeicaza @joreg That is the Create method. There is a constructor that is LTRB: https://developer.xamarin.com/api/constructor/SkiaSharp.SKRect.SKRect/p/System.Single/System.Single/System.Single/System.Single/

I can't overload the constructor, but I wanted to do that as a helper method in case you are coming from some other rect object. I didn't want to force the user to calculate the LTRB when he had the XYWH coordinates.

[mattleibow]

This is the SKRect definition:

public struct SKRect { 
    public float Left, Top, Right, Bottom; 
    public SKRect (float left, float top, float right, float bottom) { 
        Left = left; 
        Right = right; 
        Top = top; 
        Bottom = bottom; 
    } 
    public static SKRect Create (float width, float height) { 
        return new SKRect (0, 0, width, height); 
    } 
    public static SKRect Create (float x, float y, float width, float height) { 
        return new SKRect (x, y, x + width, y + height); 
    } 
} 

https://github.com/mono/SkiaSharp/blob/master/binding/Binding/Definitions.cs#L402-L421

[mattleibow]

I had a look some more at the Skia API and see this:
https://github.com/mono/skia/blob/xamarin-mobile-bindings/include/core/SkRect.h

static SkRect SK_WARN_UNUSED_RESULT MakeWH(SkScalar w, SkScalar h);
static SkRect SK_WARN_UNUSED_RESULT MakeIWH(int w, int h);
static SkRect SK_WARN_UNUSED_RESULT MakeSize(const SkSize& size);
static SkRect SK_WARN_UNUSED_RESULT MakeLTRB(SkScalar l, SkScalar t, SkScalar r, SkScalar b);
static SkRect SK_WARN_UNUSED_RESULT MakeXYWH(SkScalar x, SkScalar y, SkScalar w, SkScalar h);

The SKRect type does not have a constructor, but rather several overloads of the Make (or Create) methods

I think we need to decide what we want for the constructor. Or even no constructor. I am in favour of XYWH as the constructor and LTRB as the Create as this is what .NET usually does.

[mattleibow]

So we have several options:

1. No constructor (just the default)
2. XYWH constructor (with CreateLTRB)
3. LTRB constructor (with CreateXYWH)

I vote 2.

[joreg]

i'd vote for no constructor and all helpers called: From...

• no constructor because then there are not 2 different ways to create a rect
• From.... because FromXYWH, FromLTRB,... reads more suitable than Make.. or Create..

[tebjan]

I also vote for no constructor to have everything in one place.
The same might be true for other basic shapes as well, for consistency.

[migueldeicaza]

I do not see an advantage on making the change, so let us keep the code as is for now.