pharo-spec / Spec

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

Question on SpBoxLayout>>#addLast: #1614

Closed koendehondt closed 3 days ago

koendehondt commented 3 days ago

In Pharo 12 and Pharo 13, the implementation of SpBoxLayout>>#addLast: is:

addLast: aName
    "Adds `aPresenterLayoutOrSymbol` to the **end of the list** of presenters to be arranged 
     in the layout. 
     `aPresenterLayoutOrSymboll` can be
        - any instance of `SpPresenter` hierarchy, 
        - another layout or 
        - a Symbol, matching the name of the instance variable who will contain the element to add."

    self flag: #doNotUse.

    self 
        addLast: aName 
        withConstraints: [ :constraints | ]

Why is self flag: #doNotUse. here? It would be good to have an explanation in the method. What is the alternative? There are many senders of the message, so why are they still present if this method should not be used anymore?

Also note that the name of the method argument (aName) does not correspond with the name used in the method comment (aPresenterLayoutOrSymbol).

koendehondt commented 3 days ago

The method has been changed several times in the past year. self flag: #doNotUse. has been added and removed several times. So is the flag still valid?

Screenshot 2024-10-04 at 08 10 41
koendehondt commented 3 days ago

Also see https://github.com/pharo-spec/Spec/issues/1615

Ducasse commented 3 days ago

@estebanlm what is the replacement for this message? Because we must update the book because we use it.

estebanlm commented 3 days ago

Hi,

you have several (and keeping addLast: imposes some problems on backend implementation, particularly on morphic).

You can do:

SpBoxLayout newLeftToRight
  add: aPresenter; "this presenter will be expanded"
  add: otherPresenter expand: false; "this one will not, making it 'last'"
  yourself

or you can do:

SpBoxLayout newLeftToRight
  hAlignEnd;
  add: otherPresenter;  "this presenter will be placed at the 'end' of the box."
  yourself
estebanlm commented 3 days ago

@koendehondt was asking why?' : well, because I implemented that before implementing the hAlignStart/Center/End idioms and it looked like a good idea at first. Then I implemented it and suddenly I have 3 ways of doing the same (and 3 things to maintain) so I decided to slowly deprecate the less flexible way ( slowly: that's why is marked as #doNotUse and not deprecated directly ;) )

Can we close this issue now?

koendehondt commented 3 days ago

@estebanlm In case other people read this ticket, I like to add a comment.

The examples that you gave are not inline replacements for the things you can do with addLast:.

This code applies a box layout that puts a button on the left and on the right:

SpPresenter new
    layout: (SpBoxLayout newLeftToRight
        add: (SpButtonPresenter new label: 'Button 1'; yourself) expand: false;
        addLast: (SpButtonPresenter new label: 'Button 2'; yourself) expand: false;
        yourself);
    open
Screenshot 2024-10-04 at 12 28 44

This cannot be achieved with the examples you gave. Nesting presenters is required, like this:

left := SpPresenter new.
left layout: (SpBoxLayout newLeftToRight
    add: (SpButtonPresenter new label: 'Button 1'; yourself) expand: false;
    yourself).
right := SpPresenter new.
right layout: (SpBoxLayout newLeftToRight
    hAlignEnd;
    add: (SpButtonPresenter new label: 'Button 2'; yourself) expand: false;
    yourself).
SpPresenter new
    layout: (SpBoxLayout newLeftToRight
        add: left;
        add: right;
        yourself);
    open

It opens a window in the same way as the previous code:

Screenshot 2024-10-04 at 12 31 44

I will adapt the text of the book accordingly. In particuar, this piece of text in Chapter "Layouts" has to be replaced, and the accompanying code in the code repo has to be adapted:

The defaultLayout method adds button button3 to the box layout with addLast:expand:fill:padding:. For every method starting with add:, the SpBoxLayout class provides a similar method starting with addLast:.

A box layout has two parts: a "start" and an "end". Messages starting with add:, add subpresenters to the "start". Messages starting with addLast:, add subpresenters to the "end". As you can see in Figure @ThreeButtons@, there is a large gap between button2 and button3. For vertical box layouts, the "start" part of a box layout aligns to the top side of the box. The "end" part aligns to the bottom side of the box. The gap between the "start" and the "end" is all the excess space not used by the subpresenters.

koendehondt commented 3 days ago

As to your question "Can we close this issue now?", I would say yes, although it would be better to add a ticket for deprecating the addLast:... methods and cleaning up the senders in the Pharo tools. Otherwise other people will ask the same question I asked here. After all, just writing self flag: #doNotUse does not convey any helpful information to the reader.

estebanlm commented 3 days ago

the behavior you ask for is still covered by the other, more flexible functionality, this script will cover what you want in a more elegant way than your example (yes, by nesting boxes, and aligning them correctly) :

(p := SpPresenter new)
    layout: (SpBoxLayout newLeftToRight
        add: (p newButton label: 'Button 1'; yourself) expand: false;
        add: (SpBoxLayout newLeftToRight
            hAlignEnd;
            add: (p newButton label: 'Button 2'; yourself);
            yourself)   ;
        yourself);
    open.
estebanlm commented 3 days ago

added an issue: https://github.com/pharo-spec/Spec/issues/1616

koendehondt commented 3 days ago

Thank you for the answers and the new ticket.