broken paginator in inspector because of some missing thing in SpBoxLayout changes

[estebanlm]

#1813

this PR introduced a problem in the calculus of box size, at least in the case of the paginator.

[estebanlm]

cc @koendehondt :)

[koendehondt]

@estebanlm and @Ducasse

Next week, I will attend the Pharo Sprint. That is a good time to investigate the issue. Then we can also talk about it.
But it would be helpful to have a description of the paginator's expected behaviour. Could you add it to this ticket?
With that description, I can take a look at the issue before the Pharo Sprint.

[estebanlm]

Hello, well... this is easy. I expect it to be visible :)

next sprint is highly unlikely I will manage to go as this is school holidays, and I will most probably need to work from home.
I will try to be there at least a bit, but no promises :(

[koendehondt]

The PR I submitted was rejected by @estebanlm. I re-investigated the issue reported in #1735 and "fixed" in #1813.

This ticket proves that the issue reported in #1735 was not fixed by #1813. I know why. I'd like to explain below.

Inspector

It all starts in StInspectorPresenter>>#initializePresenters. It creates a SpMillerPaginatorPresenter.

initializePresenters

	millerList := self instantiate: SpMillerPaginatorPresenter.
	millerList millerListPresenter
		spacing: self spacing;
		presenterBlock: [ :aModel | self newInspectorFor: aModel ];
		setRootModel: self model;
		whenColumnsWillChangeDo: [ self keepLastPageSelectedTabName ]

SpMillerPaginatorPresenter>>#initializePresenters creates a SpMillerColumnPresenter and, more interestingly, an empty SpBoxLayout, which is added to the main layout with add: ... expand: false. The presenter keeps the empty box layout in an instance variable paginatorContainerLayout. Here is the implementation:

initializePresenters

	self layout: (SpBoxLayout newTopToBottom
		add: (millerList := self instantiate: SpMillerColumnPresenter);
		add: ((paginatorContainerLayout := SpBoxLayout newLeftToRight)
				hAlignCenter; 
				yourself)
			expand: false;
		yourself).

	millerList withoutHorizontalScrollBar.
	millerList whenColumnsChangedDo: [ :page | self updatePaginator: page ].
	self updatePaginator: 1

When the inspector has more than one page, the paginator is added to the paginatorContainerLayout. As reported in this ticket, the paginator doesn't appear. More correctly, it is displayed, but it has a height of 2 pixels. See the short horizontal darker grey line at the bottom of the window. Read Morph>>#minHeight why it is 2 pixels high.

Adding a presenter to a box layout invokes the method SpMorphicBoxAdapter>>#verifyBoxExtentOf:withChild:, which is implemented as:

verifyBoxExtentOf: aPanel withChild: childMorph
	| width height newPanelWidth newPanelHeight spacing |

	width := childMorph width.
	height := childMorph height.
	"The cellInset of the startPanel and the endPanel hold the spacing of the box layout.
	 They are the same. See #layout."
	spacing := startPanel cellInset.
	startPanel submorphCount + endPanel submorphCount = 0
		ifTrue: [
			width := width + (widget borderWidth * 2).
			height := height + (widget borderWidth * 2).
			spacing := 0 ].

	layout isVertical 
		ifTrue: [ height := height + aPanel height + spacing ]
		ifFalse: [ width := width + aPanel width + spacing ].

	newPanelWidth := aPanel hResizing = #rigid
		ifTrue: [ aPanel width ]
		ifFalse: [ aPanel width max: width ].
	newPanelHeight := aPanel vResizing = #rigid
		ifTrue: [ aPanel height ]
		ifFalse: [ aPanel height max: height ].
	aPanel extent: newPanelWidth @ newPanelHeight

I made a mistake when I submitted this code. The last part, which calculates newPanelWidth and newPanelHeight, is not correct. I assumed that #rigid means that the panel has a fixed size. That was a wrong assumption. It is clear from the methods described below, which set the hResizing and vResizing, while the width and height are not set, because they are not specified in the constraints.

SpMorphicBoxAdapter>>#addConstraints:toChild: is implemented as follows.

addConstraints: constraints toChild: childMorph
	"Adds constraits by child"
	| theMorph |
	
	theMorph := self addPadding: constraints padding toMorph: childMorph.
	constraints isExpand 
		ifTrue: [ 
			"if is not 'fill', in fact I need to pad it with another (since it 'expands', 
			 but does not 'fills')"
			constraints isFill ifFalse: [ 
				theMorph
					hResizing: #rigid;
					vResizing: #rigid.
				theMorph := self newPanel
					addMorphBack: theMorph;
					yourself ]. 
			"Set to fill"
			theMorph 
				hResizing: #spaceFill;
				vResizing: #spaceFill ]
		ifFalse: [
			"Set morph to stay rigid"
			layout direction setRigidityOfNonExpandedMorph: theMorph.
			constraints width ifNotNil: [ :w | theMorph width: w ].
			constraints height ifNotNil: [ :h | theMorph height: h ] ].
	
	^ theMorph 

Note that constraints isExpand evaluates to false in the case of adding the paginator to the empty box layout in the SpMillerPaginatorPresenter, which means that layout direction setRigidityOfNonExpandedMorph: theMorph is invoked. For a SpHorizontalLayoutDirection, which is the direction of the box layout bound to paginatorContainerLayout, it is implemented as:

setRigidityOfNonExpandedMorph: aMorph

	aMorph 
		vResizing: #spaceFill; 
		hResizing: #rigid

Adding presenters with a fixed width

I made the wrong assumption because the method SpBoxLayout>>#add:width:, used to specify a fixed width for a presenter in a box layout, results in settinghResizing as well.

add: aPresenterLayoutOrSymbol width: anInteger
	"Add a presenter and assign a specific width. 
	 This constraint is applied just when using an Horizontal (Left to Right) layout, 
	 otherwise is ignored.
	 Please notice that 99% of the time you DO NOT NEED (and you do not want) to use 
	 a fixed constraint like this one. 
	 Instead, consider using `SpBoxLayout>>#add:expand:` method. "

	self
		add: aPresenterLayoutOrSymbol
		withConstraints: [ :constraints | constraints width: anInteger ].

The method above adds a presenter with constraints that specify a width. SpBoxConstraints>>#width: sets expand to false:

width: aNumber
	"Width of component.
	 Set the constraint of the presenter to assign a specific width. 
	 This constraint is applied just when using a Horizontal (Left to Right) layout, 
	 otherwise is ignored.
	 Please notice that 99% of the time you DO NOT NEED (and you do not want) to use 
	 a fixed constraint like this one. 
	 Instead, consider using `SpBoxLayout>>#add:expand:` method, and let the component
	 take its 'natural' width (often defined in a style)."

	self expand: false.
	width := aNumber

And then based on the above contraints, SpMorphicBoxAdapter>>#addConstraints:toChild: sets hResizing to #rigid and the width to the given fixed width. Note that when a SpLayoutConstraints has a width or a height, they are intended to be fixed (see the class comment of SpLayoutConstraints).

However, and this is the crucial part of the story, the Morph representing the added presenter does not know that it has a fixed width. The same reasoning applies for a fixed height.

The bug

So, SpBoxLayout has different methods that result in hresizing and vResizing being #rigid.

1. #add: ... expand: false
2. #add: ... width: ... and #add: ... height:

The old implementation of SpMorphicBoxAdapter>>#verifyBoxExtentOf:withChild: works for the paginator in the inspector, but not for the bug reported in #1735.

The new implementation in #1813 fixes the bug reported in #1735, but it does not work for the paginator in the inspector.

Let's have a look at SpMorphicBoxAdapter>>#verifyBoxExtentOf:withChild: again. This part is wrong:

	newPanelWidth := aPanel hResizing = #rigid
		ifTrue: [ aPanel width ]
		ifFalse: [ aPanel width max: width ].
	newPanelHeight := aPanel vResizing = #rigid
		ifTrue: [ aPanel height ]
		ifFalse: [ aPanel height max: height ].

It should be:

	newPanelWidth := (layout isHorizontal and: [ aPanel hasFixedSize ])
		ifTrue: [ aPanel width ]
		ifFalse: [ aPanel width max: width ].
	newPanelHeight := (layout isVertical and: [ aPanel hasFixedSize ])
		ifTrue: [ aPanel height ]
		ifFalse: [ aPanel height max: height ].

where hasFixedWidth and hasFixedHeight answer whether fixed width or fixed height should be applied.

However, a Morph cannot determine whether it has a fixed width or a fixed height, because it is never instructed to be of a fixed size. A Morph has a width and a height, but they are the current dimensions of a Morph. The layout determines the current dimensions.

Towards a solution

SpMorphicBoxAdapter>>#addConstraints:toChild: can tell a Morph to be of fixed size. Instead of sending #width: and #height:, it could send #fixedWidth: and #fixedHeight:, like this:

addConstraints: constraints toChild: childMorph
	"Adds constraits by child"
	| theMorph |
	
	theMorph := self addPadding: constraints padding toMorph: childMorph.
	constraints isExpand 
		ifTrue: [ 
			"if is not 'fill', in fact I need to pad it with another (since it 'expands', 
			 but does not 'fills')"
			constraints isFill ifFalse: [ 
				theMorph
					hResizing: #rigid;
					vResizing: #rigid.
				theMorph := self newPanel
					addMorphBack: theMorph;
					yourself ]. 
			"Set to fill"
			theMorph 
				hResizing: #spaceFill;
				vResizing: #spaceFill ]
		ifFalse: [
			"Set morph to stay rigid"
			layout direction setRigidityOfNonExpandedMorph: theMorph.
			constraints width ifNotNil: [ :w | theMorph fixedWidth: w ].
			constraints height ifNotNil: [ :h | theMorph fixedHeight: h ] ].
	
	^ theMorph 

#fixedWidth: and #fixedHeight: express intent much better. However, they do not exist. We can implement them as follows:

Morph>>#fixedWidth:

fixedWidth: aNumber

	self assureTableProperties
		minCellSize: aNumber;
		maxCellSize: aNumber.
	self width: aNumber

Morph>>#fixedHeight:

fixedHeight: aNumber

	self assureTableProperties
		minCellSize: aNumber;
		maxCellSize: aNumber.
	self height: aNumber

What remains is implementing #hasFixedSize:

Morph>>#hasFixedSize

hasFixedSize
	"Layout specific."

	| props |
	props := self layoutProperties.
	^ props ifNil: [ false ] ifNotNil: [ props hasFixedSize ]

TableLayoutProperties>>#hasFixedSize

hasFixedSize

	^ self minCellSize isNotNil and: [ self minCellSize = self maxCellSize ]

PR

I have implemented the above solution. It fixes the bug reported in this ticket. It also passes the unit tests that were added when #1735 was fixed. All Spec unit tests are green, but that may not prove anything, because not all aspects of SpBoxLayout are tested.

@estebanlm Before I submit a PR, please review my analysis above and let me know whether the solution is the right one. Thank you.

[estebanlm]

ok, I will not be able to answer for the next two weeks, but let's address this quickly so you do not have to wait.
In general, I think you analysis of what is happening is correct, but I have some noise with the proposed answer (bah with the place of the answer).
Let's explain a bit the concepts I manage, in case I am just wrong :P

• In morphic, the layout defines how the children (submorphs) of the morph will be arranged
• the layoutProperties instead (of which h/vResizing are part of), affect how the morph with be arranged in relation to its parent.

This is very confusing in part because, for example, a table layout is called TableLayout while a layout properties for the case of which the morph is contained inside a table is called TableLayoutProperties, which is also used to add properties to the TableLayout !

I believe this was because the one implementing that did not understand fully the concept originally. Or maybe them just wanted to save some attribute space by unifying the two concerns ? Anyway, this is bad :(

Which is as always the problem with morphic: is so complex that is hard to understand, hence most of its contributors (me between them ;) ) touched things without knowing for real the design and consequences of its modifications, creating with time a big ball of mud nobody can understand for real.

And this is why I do not like completely the proposal of add a fixedWidth:/Height: to Morph class. Instead, since all if very complex and mixed, I understand that would be maybe the way to fix both issues, but I would add that two methods in the spec table layout that arranges them (or in the helper, I do not remember exactly the implementation right now) , e.g. by adding setFixedWidth:toMorph: and setFixedHeight:toMorph:.

EDIT: typo