pharo-spec / Spec

Spec is a framework in Pharo for describing user interfaces.
MIT License
61 stars 63 forks source link

Pharo 12 regression? SpPresenter>>#hide removes the presenter #1520

Closed koendehondt closed 3 months ago

koendehondt commented 4 months ago

Consider this code:

| presenter button1 button2 button3 box |
presenter := SpPresenter new.
button1 := presenter newButton label: 'Button 1'.
button2 := presenter newButton
    label: 'Button 2';
    action: [ button1 show ].
button3 := presenter newButton
    label: 'Button 3';
    action: [ button1 hide ].
box := SpBoxLayout newLeftToRight 
    add: button1;
    add: button2;
    add: button3;
    yourself.
presenter layout: box; open

It opens this window:

Screenshot 2024-02-16 at 17 01 17

Pharo 11

When clicking Button 3, this is the result:

Screenshot 2024-02-16 at 17 02 18

Button 1 is hidden. Button 2 and Button 3 do not move.

When clicking Button 2, Button 1 is shown again in its original place.

Screenshot 2024-02-16 at 17 05 40

This is the expected behaviour when hiding and showing a presenter.

Pharo 12

When clicking Button 3, this is the result:

Screenshot 2024-02-16 at 17 02 59

Button 2 and Button 3 do not stay in place. They consume the space of Button 1.

When clicking Button 2, Button 1 is shown again in its original place.

Screenshot 2024-02-16 at 17 06 54

Is this a regression?

I consider this to be a regression. The behaviour of #hide and #show has been changed. This has consequences in my Pharo applications.

estebanlm commented 3 months ago

no, this is not a regression, this is the expected behavior (being the old behavior the buggy one). when you hide a component is no longer visible within the context of the layout that contains it, hence the layout will recalculate its boundaries effectively removing it. if you still want to show the empty space, there are other ways to achieve that result.

koendehondt commented 3 months ago

@estebanlm I do not understand why the old behavior was the buggy one. Did someone report that it behaved incorrectly?

The implementation in Pharo 12 has now the same visual effect as removing a presenter from a box layout. So what is the fundamental difference between hiding and removing a presenter?

I would expect that:

This is an important decision because the Spec book, which I am actively reviewing, and @Ducasse likes to finish, does not say a word about the difference between hiding and removing presenters in a layout, and it should in my opinion.

On top of that, SpPresenter>>#hide is implemented as:

hide
    "hide current presenter making it invisible for the user"

    visible := false

The method comment is very clear: making the presenter invisible. The comment does not state that there will be a visual impact on the parent UI components. We can compare it with visibility: hidden in CSS. It makes an element invisible without impact on the layout.

estebanlm commented 3 months ago

because that was the intended functioning always (and it was like that at first) and the "not show but keep space" is a bug I introduced when fixing other problems in layouts. and why this was the intended functioning always ? because when developing this we try to follow the rationale "what others do in this case, in particular gtk which is our secondary backend?", and that's how gtk works (and cocoa, which I also review at the time)

koendehondt commented 3 months ago

If it was the intended functioning, then I still have these questions:

estebanlm commented 3 months ago

removing the presenter is an implementation detail in the backend, as it is the only realistic (and not completely overwhelming as a task) of perform the hide in a TableLayout of morphic. I do not thing the message should not be adapted because the concept applying here is to make the component not visible, not removing it (which as I said, is an implementation detail on morphic, other backends may behave differently).

estebanlm commented 3 months ago

to complete this: If you check, you will notice that when you hide a component (e.g. button1 in your example), the button presenter is still included in the presenter tree, while it is removed in the morphic tree, showing that this is an implementation detail. this is a difference between hide and remove:, one does not shows a component that's marked as hidden while keeping it in the tree and the other removes it from it. While working with Spec, you need to remember is Spec the one that is the one the user sees, while the backend should not be seen and its implementation details are just a way to a mean, not a concept itself.

koendehondt commented 3 months ago

I checked in Morphic before writing this issue. I know :-)

While working with Spec, you need to remember is Spec the one that is the one the user sees, while the backend should not be seen and its implementation details are just a way to a mean, not a concept itself.

That is an interesting statement that I agree with. The statement is interesting because that requires the concept to be very clear so that users of the concept know what to expect. However, the concept is not clear today. Therefore I challenge the idea that the concept of presenter visibility in Spec is implemented correctly by the backends.

Let me compare it with CSS again. I expect the behavior associated with visibility: hidden, but it is implemented as display: none. The former makes the element invisible, but it still takes space. The latter makes an element invisible too, but it does not take space. In CSS these are two concepts, controlled by two CSS properties. In the current version of Spec, there is only one of these concepts.

Although I am not happy with the current implementation (mainly because upgrading to Pharo 12 broke my application), the main message I want to convey is that the behavior is not clear. The visual behavior of SpPresenter>>#hide and SpPresenter>>#show is not documented. Not in the code. Not in the book. For people using other UI frameworks, like CSS, the current behavior is probably not what they expect.