SpAbstractWidgetPresenter>>#isEnabled is not implemented correctly

[koendehondt]

SpAbstractWidgetPresenter>>#enabled: is implemented as:

enabled: aBooleanOrValuable
    "Set if the widget is enabled (clickable or focusable).
    This can either be a boolean or a block returning a boolean."

    enabled := aBooleanOrValuable

but SpAbstractWidgetPresenter>>#isEnabled is implemented as:

isEnabled
    "Answer if presenter is enabled"

    ^ enabled

so that

presenter := SpPresenter new.
button := SpButtonPresenter new
    icon: (presenter iconNamed: #smallSave);
    enabled: [ false ];
    action: [ button inspect ];
    yourself.
presenter layout: (SpBoxLayout newLeftToRight
    add: button;
    yourself).
presenter open

fails with NonBooleanReceiver: proceed for truth.

SpAbstractWidgetPresenter>>#enabled: allows a aBooleanOrValuable argument, so the implementation of SpAbstractWidgetPresenter>>#isEnabled should be:

isEnabled
    "Answer if presenter is enabled"

    ^ enabled value

Please note that this bug makes code examples in the Spec book unusable. cc @Ducasse.

[koendehondt]

Fixing it for the Spec book means fixing it in Pharo 12.

[koendehondt]

My reasoning was based on what the argument name and the comment express:

enabled: aBooleanOrValuable
    "Set if the widget is enabled (clickable or focusable).
    This can either be a boolean or a block returning a boolean."

Maybe the argument name and the comment are wrong? After all, does it make sense to pass a block if it is evaluated only once? There is no way to evaluate it again to have an effect on the visual representation. See #1561.

So maybe a presenter with a (toolbar) button should update the button explicitly by sending #enabled: true or #enabled: false. That would be fine for me, but then the method above has to be adapted to avoid the confusion.

[koendehondt]

@estebanlm, @Ducasse and I discussed this issue.

SpAbstractWidgetPresenter>>#enabled: should not take a block. The argument name and the method comment are wrong.

[koendehondt]

I would suggest to change the method in SpAbstractWidgetPresenter to:

enabled: aBoolean
    "Set whether the widget is enabled (clickable or focusable)."

    enabled := aBoolean

but SpMenuItemPresenter is a subclass, and it allows a block or a boolean. That is not expressed anywhere, not even in the comment of the SpMenuItemPresenter class.