Questions on enabling toolbar buttons created from commands

[koendehondt]

@estebanlm, cc @Ducasse

Context: Spec book, Pharo 12, creating a toolbar based on commands.

Question 1

When using code like this in my presenter:

toolBar := self newToolbar.
toolBar fillWith: self rootCommandsGroup / 'ToolBar'

I noticed that the button enablement is not correct.

When digging into it, I read the method SpCommand>>#configureAsButtonOfClass:, implemented as:

configureAsButtonOfClass: aButtonClass
	self
		buildPresenterBlock: [ :specCommand | 
			aButtonClass new
				label: specCommand name;
				help: specCommand description;
				in: [ :button | 
					specCommand hasIcon
						ifTrue: [ button icon: specCommand icon ] ];
				action: [ specCommand execute ];
				yourself ]

#enabled: is not sent to the button presenter. Consequently, the created button presenter is enabled, even if sending #canBeExecuted to specCommand would answer false.

Isn't that a bug? I would say it is. I think the code above should be (see the addition of enabled: specCommand canBeExecuted;):

configureAsButtonOfClass: aButtonClass
	self
		buildPresenterBlock: [ :specCommand | 
			aButtonClass new
				label: specCommand name;
				help: specCommand description;
				in: [ :button | 
					specCommand hasIcon
						ifTrue: [ button icon: specCommand icon ] ];
				action: [ specCommand execute ];
				enabled: specCommand canBeExecuted;
				yourself ]

If you agree, I will create a PR.

Question 2

The implementation above has another disadvantage, even if the above issue is not a bug. What is the prescribed way to enable/disable toolbar buttons after creating the toolbar with SpToolbarPresenter>>#fillWith:?

Because the toolbar buttons are created by SpToolbarPresenter>>#fillWith:, they cannot be held in instance variables in a presenter. Therefore they cannot be enabled/disabled easily when the state of the application changes. One could re-fill the toolbar with SpToolbarPresenter>>#fillWith:, but then the button enablement is not correct, as described in question 1.

Please explain how to update toolbar buttons with the correct enablement when the state of the toolbar's presenter changes. Thank you.

[koendehondt]

@estebanlm Please respond to this question because it blocks finalising the book. Thank you!

[estebanlm]

I'd say is a bug, yes

[koendehondt]

Thank you for the confirmation.
Tonight, I will create a PR for Pharo 12.

[koendehondt]

@estebanlm Here is the PR for Pharo 12: #1621. Please merge as soon as possible because the fix is required for the Spec book. Thank you!