Oh thanks, that explains the problem I observed and why I did not spot the bug in all the time I spent staring at it in the debugger: the values were there in the rectangle, only swapped between origin and corner! Maybe receiving a rectangle as the second argument of #fractions:offsets: should cause a warning to be printed on the Transcript. It is such an easy mistake with such a tricky behaviour… Anyway, the improved FrameLayout documentation will already help a lot.

On 10 Jan 2016, at 23:16, Thierry Goubier <thierry.goubier@gmail.com> wrote:

Hi David,

this is the bug with LayoutFrame>>#fractions:offsets: we were talking about relative to that class comment.

In Pharo, Rectangles are constrained to have the smallest vertical value as the top, smallest horizontal value as the left, largest vertical value as bottom and largest horizontal value as right. So, your rectangle 'offsets' was in fact: -3 @ -3 corner: 3 @ 3 which is exactly the reverse of what you intended ;)

Using the elementary distance accessors is the correct approach.

Regards,

Thierry

Le 10/01/2016 23:05, David Allouche a écrit :

...

I guess that is a class comment for LayoutFrame. I think this is great improvement.

By I the way, I noticed something really strange when I was using it in my learning project of a TicTacToe game. I started initializing a LayoutFrame using #fractions:offsets:, but I had strange incorrect results. I have fixed them by using the elementary distance accessors.

Here is the relevant method in my code.

drawCircleOn: aCanvas "Draw a circle cell" | layout oval | "That should worlk, but does not!" "offsets := (1@1 corner: (-1)@(-1)) scaleBy: lineWidth*3." "layout := LayoutFrame fractions: (0@0 corner: 1@1) offsets: offsets." layout := (0@0 corner: 1@1) asLayoutFrame. layout topOffset: lineWidth*3; leftOffset: lineWidth*3; bottomOffset: lineWidth*(-3); rightOffset: lineWidth*(-3). oval := layout transform: self bounds. aCanvas frameOval: oval width: lineWidth color: color.

I could not find an online syntax highlighter that supports Smalltalk, so here it comes unstyled.

Should I write a test case for that, or did I try to do something obviously incorrect?

...

On 9 Jan 2016, at 10:22, stepharo <stepharo@free.fr> wrote:

I define a transformation frame relative to some rectangle. I'm basic data structure used for graphics. I represent two groups of distances: - The fractional distance (between 0 and 1) to place the morph in its owner's bounds - Fixed pixel offset to apply after fractional positioning (e.g., "10 pixel right of the center of the owner")

!! API usage It is important to understand that it is better to use the fine grained API using elementary distances (bottomFraction:, bottomOffset:, leftFraction: ....) than the ones (historical) using points and rectangles (fractions:offsets:)

The reason is that the old API (fractions:offsets:) is only interesting if you already have a rectangle and point at hand. If you need to create new ones, then they are created for nothing because they will be destructured to extract their information to be feed into the layoutFrame. So please do not blindly copy and paste code!

Example: Favor

(LayoutFrame identity leftFraction: 0; yourself);

(LayoutFrame identity leftFraction: 0.5; rightFraction: 0.95;

(LayoutFrame identity topOffset: topHeight; bottomFraction: 0; bottomOffset: self buttonsBarHeight; leftOffset: -1; rightOffset: 1) over

(LayoutFrame fractions: (0 @ 0 corner: 1 @ 1))

For this one in particular it is better to use LayoutFrame identity

It is faster and more readable

because you are creating for nothing a new rectangle and some points.

!! Final point: Do not use Rectangles for specifying layoutFrames

If you happen to need to create a rectangle and use the fractions:offsets: class methods, refrain from creating a rectangle. The reason is that a rectangle is not 4 numbers, it represents a space area and when you use a rectangle to represent random numbers you end up creating rectangles with negative extents and this is not good. In this case use Margin, Margin is a nice class holding either one number (the same "margin" on the four sides of a rectangle), or two number (top/bottom, left/right margins) or 4 numbers.

!! Implementation

Instance variables: The fractional distance (between 0 and 1) to place the morph in its owner's bounds is represented by the following instance variables: leftFraction topFraction rightFraction bottomFraction <Float>

Fixed pixel offset to apply after fractional positioning (e.g., "10 pixel right of the center of the owner") is represented by the following instance variables: leftOffset topOffset rightOffset bottomOffset <Integer>

And it probably won't be integrated in Squeak, because it's not worth the pain. The change was conducted without analyzing all the impacts, because it was probably not possible to analyze the huge code base. (though it would have been possible to instrument code and collect the sender stacks producing degenerated rectangles) The only motto was to make code more "pure". Generally, I tend to be wary... On the other hand, storing the offsets in a rectangle just because there are 4 edges is sort of hackish usage, so while at cleaning... 2016-01-11 9:42 GMT+01:00 Stephan Eggermont <stephan@stack.nl>:

On 10-01-16 23:16, Thierry Goubier wrote:

...

Hi David,

this is the bug with LayoutFrame>>#fractions:offsets: we were talking about relative to that class comment.

In Pharo, Rectangles are constrained to have the smallest vertical value as the top, smallest horizontal value as the left, largest vertical value as bottom and largest horizontal value as right.

Indeed. And there is no Morphic documentation at all that is aware of that, as nearly all of it was written before we changed the behavior of Rectangle in Pharo, and it is a change that is not done in Squeak. The result is that nearly no old Morphic code will run unmodified in Pharo.

I would like to collect a list of these gotchas, preferably with solutions, to make it easier for people to update/migrate old morphic code. Post them here, or mail them to me.

Stephan

On Tue, Jan 12, 2016 at 6:02 PM, David Allouche <david@allouche.net> wrote:

I wonder whether it would be worth reversing this change, to remove an unnecessary incompatibility with other dialects.

Of course we should not introduce *unnecessary* incompatibilities, but its be hard to judge the subtle dampening effect of compatibility on innovation & API cleaning, against the benefits obtained from these. So Pharo tends to favour the latter over the former. Equally, mistakes sometimes aren't apparent until later, so no "improvement" should be treated as golden goose. I'm not qualified to have an opinion in this case, but I remember several people being quite pleased with the change. You'd probably have to clearly identify other benefits of reverting. (Compatibility with existing Morphic examples noted.)

But maybe there is no development activity outside of Pharo, that would justify making any effort to maintain compatibility. I do not know.

Practicality beats purity, if cleaning up existing APIs means cutting out Pharo from a wider ecosystem, and introducing painful bitrot, it might not be worth the cost.

You may find interesting the first three chapters... https://gforge.inria.fr/frs/download.php/30434/PharoVision.pdf cheers -ben

On 11 Jan 2016, at 22:25, Nicolas Cellier <nicolas.cellier.aka.nice@gmail.com> wrote:

And it probably won't be integrated in Squeak, because it's not worth the pain.

The change was conducted without analyzing all the impacts, because it was probably not possible to analyze the huge code base. (though it would have been possible to instrument code and collect the sender stacks producing degenerated rectangles)

The only motto was to make code more "pure". Generally, I tend to be wary...

On the other hand, storing the offsets in a rectangle just because there are 4 edges is sort of hackish usage, so while at cleaning...

2016-01-11 9:42 GMT+01:00 Stephan Eggermont <stephan@stack.nl>:

...

On 10-01-16 23:16, Thierry Goubier wrote:

...

Hi David,

this is the bug with LayoutFrame>>#fractions:offsets: we were talking about relative to that class comment.

In Pharo, Rectangles are constrained to have the smallest vertical value as the top, smallest horizontal value as the left, largest vertical value as bottom and largest horizontal value as right.

Indeed. And there is no Morphic documentation at all that is aware of that, as nearly all of it was written before we changed the behavior of Rectangle in Pharo, and it is a change that is not done in Squeak. The result is that nearly no old Morphic code will run unmodified in Pharo.

I would like to collect a list of these gotchas, preferably with solutions, to make it easier for people to update/migrate old morphic code. Post them here, or mail them to me.

Stephan

I wonder whether it would be worth reversing this change, to remove an unnecessary incompatibility with other dialects.

But maybe there is no development activity outside of Pharo, that would justify making any effort to maintain compatibility. I do not know.

Practicality beats purity, if cleaning up existing APIs means cutting out Pharo from a wider ecosystem, and introducing painful bitrot, it might not be worth the cost.

Hum.

That change induced a lot of pain because touching Rectangle is very invasive (tend to crash the GUI), but I can't avoid thinking that places creating those strange rectangles were in fact bugs.

They were bugs. This API is plain crap. It produces intermediate objects for nothing. Now we have margin.

For LayoutFrame, a simple refactoring when importing Morphic code would do the trick.

In practice, a significant amount of code in Pharo 5 still creates wrong rectangles.

Less and less. We spent a lot of time chasing them. We used the trick of nicolas to spot them based on exception. and I would be curious to see if some degenerated rectangles are left. Then it is important to clean it because we end up having from clipping and the code just worked because primitives werer robust.

A specific package even added an API to Rectangle to be able to do so.

Bad idea.

Thierry

...

On 11 Jan 2016, at 22:25, Nicolas Cellier <nicolas.cellier.aka.nice@gmail.com <mailto:nicolas.cellier.aka.nice@gmail.com>> wrote:

And it probably won't be integrated in Squeak, because it's not worth the pain.

The change was conducted without analyzing all the impacts, because it was probably not possible to analyze the huge code base. (though it would have been possible to instrument code and collect the sender stacks producing degenerated rectangles)

The only motto was to make code more "pure". Generally, I tend to be wary...

On the other hand, storing the offsets in a rectangle just because there are 4 edges is sort of hackish usage, so while at cleaning...

2016-01-11 9:42 GMT+01:00 Stephan Eggermont <stephan@stack.nl <mailto:stephan@stack.nl>>:

On 10-01-16 23:16, Thierry Goubier wrote:

Hi David,

this is the bug with LayoutFrame>>#fractions:offsets: we were talking about relative to that class comment.

In Pharo, Rectangles are constrained to have the smallest vertical value as the top, smallest horizontal value as the left, largest vertical value as bottom and largest horizontal value as right.

Indeed. And there is no Morphic documentation at all that is aware of that, as nearly all of it was written before we changed the behavior of Rectangle in Pharo, and it is a change that is not done in Squeak. The result is that nearly no old Morphic code will run unmodified in Pharo.

I would like to collect a list of these gotchas, preferably with solutions, to make it easier for people to update/migrate old morphic code. Post them here, or mail them to me.

Stephan