DSL for PlasticView Anchors and Values #76

m4tbat · 2016-11-22T12:19:53Z

This PR adds a small DSL intended to improve readability and speed of development when laying out PlasticViews, i.e. setting Anchors with offsets.

The current way to set an anchor to a PlasticView with an offset is something like:

button.setTop(view.top, offset: .scalable(300))
button.setRight(view.right, offset: .fixed(-50))

With this DSL, the above example would turn into:

button.top = view.top + 300
button.right = view.right - 50.fixed

The contents of this PR are (see each commit message for more details):

Add offset property to Anchor 
Add ability to add an offset to an anchor through + and - operators
Make Value conform to ExpressibleByIntegerLiteral and ExpressibleByFloatLiteral
Add scaled and fixed properties to CGFloat, Double and Int as a convenient way to instantiate Value instances

Add an `offset` property to `Anchor`, in order to be able to set anchors at a certain offset from the receiving view's anchor

This commit adds the `+=`, `-`, `-=`, `*=` and `/=` operators to Value

…oatLiteral This allows `Value` instances to be initialized by integer and floating point literals, having the scalable part equal to the numeric literal, and the fixed part equal to 0.

…as a convenient way to instantiate `Value` instances

bolismauro · 2016-11-22T14:33:04Z

I'll review it this evening.
Some feedback

remove .scalable and 'fixed
Remove the init(value) (also from Size and EdgeInsets)
Make sure all the examples work
Check the documentation / readme and make sure that everything is updated

bolismauro · 2016-11-22T19:58:00Z

I have a huge doubt about this.
We are introducing 30.fixed and because of this, we should remove .fixed(30) (otherwise we have multiple methods to do the same thing, which is not good.
What about size and edgeInsets? We can't use .fixed anymore, for consistency. How would you replace them?

Another thing I don't like is that you changed the Anchor struct by adding the offset. An Anchor represents an edge of a view, and adding an offset doesn't really make sense...
Is it possible to implement the functionalities you want without doing this?

Third and last one, can you make a complete list of things you are adding? I saw several operator overloads and I'd like to have a list (e.g, to create the changelog of the bump) of things added/removed

m4tbat · 2016-11-22T21:06:02Z

@bolismauro thanks for your thoughts on the PR!

Let me address them:

We are introducing 30.fixed and because of this, we should remove .fixed(30) (otherwise we have multiple methods to do the same thing, which is not good.

The fact is: I'm not sure that I agree with this.

I do agree that having to renounce to the ability to have a nice way to instantiate sizes and edge insets – i.e. by using .scalable(...) or .fixed(...) – would be bad.

In fact, I wouldn't remove them! I see the extensions to Int and Double that I proposed in this PR as a nice, harmless, convenience, not intended to replace anything. It just felt like a really spot-on use case to be able to get a Value (which I see as an implementation detail I'd rather not be concerned about) from an Int or a Double.

As for your concern regarding multiple ways to do the same thing: I'm not convinced it's intrinsically bad, as long as we're talking about things that differ just slightly in syntax and do not have any other bigger implication that one should be aware of.

So, I'd rather give people a bit more flexibility to adapt to different needs/contexts, and leave to each project's guidelines to set the rules for this kind of details.

That said, no big deal if people feel we're better off without them :)

Another thing I don't like is that you changed the Anchor struct by adding the offset. An Anchor represents an edge of a view, and adding an offset doesn't really make sense... Is it possible to implement the functionalities you want without doing this?

I think it's the only solution to achieve something like button.top = view.top + 300.

I kind of agree that offset doesn't really belong to an Anchor if you think of it as "an edge of a view".

But, if you think of an Anchor as "a mechanism to link one side of a view to a side of another view, with a — possibly non zero — offset between them", it suddenly makes more sense.

Third and last one, can you make a complete list of things you are adding? I saw several operator overloads and I'd like to have a list (e.g, to create the changelog of the bump) of things added/removed

Sure thing!

sroddy · 2016-11-22T22:45:31Z

If we are going to add the offset property directly to Anchors, I suggest to follow one single approach and entirely remove all the set*(anchor:, offset:) and the other convenience methods that use the offsets and just keep the top/bottom/left/right properties.
This will avoid the unnecessary confusion of having multiple offsets within a single set*() operation.

Neither I am 100% convinced but for sure this approach would:

Simplify the interface removing a lot of convenience methods.
Solve one of the biggest problems i had so far with the good old Plastic, which was having an offset in the view.pl_centerBetweenTop:andBottom: (and also seems to be an issue even in the current version of Plastic).

Another thing that I've noticed is that this new Anchor's offset property currently has no impact on the coordinate getter. Is that on purpose? And if so, don't you think this might be even more misleading?

m4tbat · 2016-11-22T23:10:58Z

Another thing that I've noticed is that this new Anchor's offset property currently has no impact on the coordinate getter. Is that on purpose? And if so, don't you think this might be even more misleading?

It's not on purpose. Thanks for making me notice it. Adding offset to the coordinate would be the right thing to do if we decide to go with my proposed changes.

bolismauro · 2016-11-23T07:46:25Z

@sroddy

Solve one of the biggest problems i had so far with the good old Plastic, which was having an offset
in the view.pl_centerBetweenTop:andBottom:
(and also seems to be an issue even in the current version of Plastic).

can you elaborate?

sroddy · 2016-11-23T09:55:03Z

Here is a concrete example I faced a couple of months ago. Suppose you have to layout something like this:

The red horizontal line is meant to be placed in the middle of the space between the bottom of the Title and the bottom of the View plus an offset that is highlighted with the red arrow.

In the current version of Plastic this layout is a bit tricky to achieve and most of the solutions involve the creation of transparent views (e.g. a transparent line placed below the title anchor with an offset) which serve no other purpose than aiding in the layout.

With the proposed change, this would be easily achieved like that: view.centerBetween(top: title.bottom + 20.scaled, bottom: root.bottom).

I guess this won't be the only example where adding an offset to anchors would simplify things, and would avoid the need of creating offset variants of the helper methods.

bolismauro · 2016-11-23T10:01:14Z

There are ways to do it without additional views but I get your point. Ok so, let's add this concept of offset for the anchors.

I think we should solve the issue with size/edgeinsets and then we can proceed with the implementation of everything.
I still don't have a clear idea about how to solve it. To me, we should try to be consistent though

…from PlasticView

m4tbat · 2016-11-27T17:59:03Z

I've updated my PR to:

add offset to Anchor's coordinate
remove setTop/Bottom/Left/Right/CenterX/CenterY/Width/Height methods from PlasticView, as they're superseded by top/bottom/left/right/centerX/centerY setters
update PlasticView convenience methods – and relative tests – that depend upon the aforementioned methods.
Remove Value initializer init(_ scalable: CGFloat)

As for the debated Int, Double and CGFloat extensions that produce scalable and fixed values, I can remove them from this PR, and we may discuss about their appropriateness in another PR.

bolismauro

Can you also make sure that

all the examples work
the documentation is updated
the readme is updated
you have added relevant tests, if needed

Plus (I can't do it myself since you forked the repo) I'd remove the following methods

public init(_ width: CGFloat, _ height: CGFloat) (Size)
public init(scalableWidth: CGFloat, fixedWidth: CGFloat, scalableHeight: CGFloat, fixedHeight: CGFloat) { (Size)
public init(_ top: CGFloat, _ left: CGFloat, _ bottom: CGFloat, _ right: CGFloat) { (Insets)
public init(scalableTop: CGFloat, fixedTop: CGFloat, scalableLeft: CGFloat, fixedLeft: CGFloat, scalableBottom: CGFloat, fixedBottom: CGFloat, scalableRight: CGFloat, fixedRight: CGFloat) (Insets)

bolismauro · 2016-11-27T18:11:49Z

Katana/Plastic/Anchor.swift


    }
+
+    coord += view.scaleValue(offset)


I'd change this in return coord + view.scaleValue(offset)

bolismauro · 2016-11-27T18:20:21Z

Katana/Plastic/Value.swift

+ This allows `Value` instances to be initialized by integer and floating point literals,
+ having the scalable part equal to the numeric literal, and the fixed part equal to 0.
+ */
+extension Value: ExpressibleByIntegerLiteral, ExpressibleByFloatLiteral {


Can we remove this?

We can define the + operator also for Anchor/Int and Anchor/Double.
I don't really like these protocols to be honest
@smaramba @lucaquerella opinions?

In my opinion it's better to have Value conforming to ExpressibleByIntegerLiteral and ExpressibleByFloatLiteral, since they exist for this purpose: they make it clear to users of Value why using integer/float literals in operations expecting Values works.

Defining an overload of the + operator for Anchor/Int and Anchor/Double would be more confusing IMHO.

they make it clear to users of Value why using integer/float literals in operations expecting Values works. to me, they don't make it clear. You have to discover that you have implemented that protocol somewhere to understand the code, otherwise you don't get why it works.

If you define the proper + operator, if is oblivious the reason why it works.

In general we decided to don't use them multiple times because they can create confusing situations..
For instance (not tested, but it should work)

init(width: Value, height: Value) -> Size // you can do Size(width: 5, height: 7)

You are basically propagating this ability to use double instead of values everywhere and to me this is not good (consistency & clarity first, type less characters then)

Ok, this discussion would go the same way the discussion on .scalable/.fixed went 😄

Should I proceed and remove all these additions from the PR, or would you prefer to learn @lucaquerella's and @smaramba's opinions first?

I asked opinions to both. As always I will be happy with the decision of the majority of us.

I really don't get why I'm the only once concerned about losing clarity. With that additions you can swap numbers/Values.

I'm kinda neutral, you are strongly against, so it's a no go to me. 👍

I absolutely agree that automatic type conversions/promotions make understanding what's going on less clear than having explicit inizializers.

But I also believe that, to the user new to Katana that has never used Plastic, reading code filled with .scalable(...), Value(scalable: ..., fixed: ...) would be quite puzzling. And, in general, it adds boilerplate, slowing down both the reading and writing of the code (not in a major way of course, but still).

So what I'm trying to convey is: since Values are an implementation detail I'd rather not be concerned about wherever possible, what if I'm just told "Plastic scales dimensions for you in order to make your views lay out consistently across different screen sizes. If you don't want something to be scaled, just use .fixed". And I use ints and floats as I've always done since the beginning of time when doing UI work on any platform.

BUT I understand this is my point of view, and may not be anyone else's 🙃.

But I also believe that, to the user new to Katana that has never used Plastic, reading code filled with .scalable(...), Value(scalable: ..., fixed: ...) would be quite puzzling.

You can't use a library without knowing the basic concepts. Value is a basic concept of Plastic. Because of this I don't buy the puzzling argument. I agree with the boilerplate and in fact I agree with the layout DSL. I just don't want to extend this DSL everywhere

"Plastic scales dimensions for you in order to make your views lay out consistently across different screen sizes. If you don't want something to be scaled, just use .fixed"

in plastic you have to use fixed also when things are scaled. I can give you infinite examples of this

as discussed offline with @bolismauro I do agree that going with the most conservative implementation (overloading of +) in this specific case should be the right option, avoiding to leak out the bridge Int<->Value to other parts of the framework/application

👍 let's limit it to this specific use case as you propose then, it's probably the safest and more useful thing to do.

Besides adding the + and - operators with Int and CGFloat/Double versions, do you prefer we keep also the ones accepting Values (in case I already have a Value instance I want to use as a offset – which would be rare) or we remove them?

bolismauro · 2016-11-27T18:20:47Z

Katana/Plastic/Value.swift

+  }
+}
+
+extension CGFloat {


I'd remove these from this PR and move this discussion in another issue/PR

m4tbat · 2016-11-27T20:32:01Z

Can you also make sure that

all the examples work

the documentation is updated

the readme is updated

you have added relevant tests, if needed

Everything on the list should be good (no readme changes nor new tests needed).

Plus (I can't do it myself since you forked the repo) I'd remove the following methods

Done.

…leByFloatLiteral

… methods `scalable` and `fixed`

…lues

…loat as right-hand side argument

m4tbat · 2016-11-27T22:26:03Z

I've

🔥 the conformance of Value to ExpressibleByIntegerLiteral and ExpressibleByFloatLiteral
🔥 extensions to CGFloat, Double and Float that implemented the Value-returning methods scalable and fixed
added overloads to + and - operators operating on Anchor and CGFloat values, with corresponding tests.

m4tbat added 10 commits November 22, 2016 10:42

Add offset property to Anchor

ba74bf9

Add an `offset` property to `Anchor`, in order to be able to set anchors at a certain offset from the receiving view's anchor

Add ability to add an offset to an anchor through + and - operators

b5c634a

Support anchor's new offset property in PlasticView setters

0438cb7

Add missing operators to Value for completeness

480c2cf

This commit adds the `+=`, `-`, `-=`, `*=` and `/=` operators to Value

Make Value conform to ExpressibleByIntegerLiteral and ExpressibleByFl…

16a8918

…oatLiteral This allows `Value` instances to be initialized by integer and floating point literals, having the scalable part equal to the numeric literal, and the fixed part equal to 0.

Add scaled and fixed properties to CGFloat, Double and Int …

2e04d17

…as a convenient way to instantiate `Value` instances

Merge remote-tracking branch 'BendingSpoons/master' into anchor-dsl

599fdb9

Merge remote-tracking branch 'BendingSpoons/master' into anchor-dsl

2948814

Add tests for laying out plastic views with anchors with offsets

dde411e

Merge remote-tracking branch 'BendingSpoons/master' into anchor-dsl

f590924

m4tbat added 6 commits November 27, 2016 17:30

Add offset to anchor's coordinate

80a408e

Remove setTop/Bottom/Left/Right/CenterX/CenterY/Width/Height methods …

51ae831

…from PlasticView

Merge remote-tracking branch 'BendingSpoons/master' into anchor-dsl

486eab4

Use new anchor + offset setters in Plastic convenience methods

7b90a30

Update tests to match changes to PlasticView anchor setters

3254e6a

Remove Value initializer init(_ scalable: CGFloat)

762a493

bolismauro suggested changes Nov 27, 2016

View reviewed changes

m4tbat added 4 commits November 27, 2016 21:00

Improve Anchor.coordinate

b9c5287

Remove ambiguous initializers from Size

cf6aa33

Remove ambiguous inizializers from EdgeInsets

c8109b4

Update Size- and EdgeInsets-related tests

c2c279b

m4tbat added 4 commits November 27, 2016 23:04

Remove Value conformance to ExpressibleByIntegerLiteral and Expressib…

af88c63

…leByFloatLiteral

Remove extensions to CGFloat, Double and Float to add Value-returning…

17aad30

… methods `scalable` and `fixed`

Add overloads to + and - operators operating on Anchor and CGFloat va…

eaea787

…lues

Add tests for creation of Anchors through the + operator taking a CGF…

7791124

…loat as right-hand side argument

bolismauro merged commit 4fa4ee6 into BendingSpoons:master Nov 28, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DSL for PlasticView Anchors and Values #76

DSL for PlasticView Anchors and Values #76

m4tbat commented Nov 22, 2016 •

edited

bolismauro commented Nov 22, 2016

bolismauro commented Nov 22, 2016

m4tbat commented Nov 22, 2016

sroddy commented Nov 22, 2016 •

edited

m4tbat commented Nov 22, 2016

bolismauro commented Nov 23, 2016 •

edited

sroddy commented Nov 23, 2016 •

edited

bolismauro commented Nov 23, 2016

m4tbat commented Nov 27, 2016

bolismauro left a comment •

edited

bolismauro Nov 27, 2016

m4tbat Nov 27, 2016

bolismauro Nov 27, 2016

m4tbat Nov 27, 2016 •

edited

bolismauro Nov 27, 2016 •

edited

m4tbat Nov 27, 2016

bolismauro Nov 27, 2016

smaramba Nov 27, 2016

m4tbat Nov 27, 2016 •

edited

bolismauro Nov 27, 2016 •

edited

smaramba Nov 27, 2016

m4tbat Nov 27, 2016

bolismauro Nov 27, 2016

m4tbat commented Nov 27, 2016

m4tbat commented Nov 27, 2016

DSL for PlasticView Anchors and Values #76

DSL for PlasticView Anchors and Values #76

Conversation

m4tbat commented Nov 22, 2016 • edited

bolismauro commented Nov 22, 2016

bolismauro commented Nov 22, 2016

m4tbat commented Nov 22, 2016

sroddy commented Nov 22, 2016 • edited

m4tbat commented Nov 22, 2016

bolismauro commented Nov 23, 2016 • edited

sroddy commented Nov 23, 2016 • edited

bolismauro commented Nov 23, 2016

m4tbat commented Nov 27, 2016

bolismauro left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m4tbat Nov 27, 2016 • edited

Choose a reason for hiding this comment

bolismauro Nov 27, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m4tbat Nov 27, 2016 • edited

Choose a reason for hiding this comment

bolismauro Nov 27, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m4tbat commented Nov 27, 2016

m4tbat commented Nov 27, 2016

m4tbat commented Nov 22, 2016 •

edited

sroddy commented Nov 22, 2016 •

edited

bolismauro commented Nov 23, 2016 •

edited

sroddy commented Nov 23, 2016 •

edited

bolismauro left a comment •

edited

m4tbat Nov 27, 2016 •

edited

bolismauro Nov 27, 2016 •

edited

m4tbat Nov 27, 2016 •

edited

bolismauro Nov 27, 2016 •

edited