Skip to content

Conversation

@estebanlm
Copy link
Contributor

this PR adapts the examples and explanations to the new action architecture, that will be used to add context menus and keybindings to presenters.

@Ducasse
Copy link
Member

Ducasse commented Sep 19, 2024

Thanks esteban.
@koendehondt I will not integrate it immediately so that we can update the code in the code repo.

@estebanlm
Copy link
Contributor Author

also, code is not yet integrated into the image so... better to wait a bit. But I wanted to start fixing this otherwise I will forget it later ;)

@Ducasse
Copy link
Member

Ducasse commented Sep 19, 2024

Oki

@koendehondt
Copy link
Collaborator

@estebanlm I see that you introduced commands in chapters before the chapter on Commander. Using concepts before they are introduced is not a good idea. I propose to keep the menus in the chapters before the Commander chapter and use commands only in the Commander chapter.

@estebanlm
Copy link
Contributor Author

thing is, there is not anymore just menus, they are now handled through actions (which are, indeed, "generic" commands). you can introduce actions as simple definitions, that define both "visible" actions which will be displayed as context menus and "not visible" actions that will be accessible through shortcuts.
later you can use it and "level up" the complexity by explaining the whole commander.

@koendehondt
Copy link
Collaborator

thing is, there is not anymore just menus, they are now handled through actions (which are, indeed, "generic" commands). you can introduce actions as simple definitions, that define both "visible" actions which will be displayed as context menus and "not visible" actions that will be accessible through shortcuts. later you can use it and "level up" the complexity by explaining the whole commander.

Could you explain what you mean with "now" in " they are now handled through actions"? Do you refer to a particular Pharo version?

### Installing shortcuts

Adding shortcuts to menu items does not automatically install them. Keyboard shortcuts have to be installed after the window has been opened. Therefore we have to adapt the `initializeWindow:` method with the `whenOpenedDo:` message, so that the keyboard shortcuts can be installed after opening the window. `SpMenuPresenter`, which is the superclass of `SpMenuBarPresenter`, implements the method `addKeybindingsTo:`, which comes in handy here.
Adding shortcuts to menu items does not automatically install them. Keyboard shortcuts have to be installed after the window has been opened. Therefore we have to adapt the `initializeWindow:` method with the `whenOpenedDo:` message, so that the keyboard shortcuts can be installed after opening the window. `SpMenuPresenter`, which is the superclass of `SpMenuBarPresenter`, implements the method `addKeyBindingsTo:`, which comes in handy here.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This suggestion is not correct. SpMenuPresenter>>#addKeybindingsTo: is written with a lowercase b, contrary to other method selectors with "KeyBinding" in them, like SpAbstractMorphicAdapter>>#addKeyBindingsTo:.

"this will copy the menubar shortcuts from menuBar to the window presenter,
to allow the window to answer to them"
menuBar addKeyBindingsTo: aWindowPresenter
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
menuBar addKeyBindingsTo: aWindowPresenter
menuBar addKeybindingsTo: aWindowPresenter

toolbar: toolBar
toolbar: toolBar.
menuBar addKeyBindingsTo: aWindowPresenter
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
menuBar addKeyBindingsTo: aWindowPresenter
menuBar addKeybindingsTo: aWindowPresenter

statusBar: statusBar
statusBar: statusBar.
menuBar addKeyBindingsTo: aWindowPresenter
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
menuBar addKeyBindingsTo: aWindowPresenter
menuBar addKeybindingsTo: aWindowPresenter

Typically, a presenter adds a context menu to a subpresenter. Given that the tree of folders and emails is a subpresenter of the `MailAccountPresenter`, we would expect the `MailAccountPresenter` to install a context menu on the tree presenter. However, the `MailAccountPresenter` cannot decide what needs to be done for deleting or sending an email. What needs to be done is the responsibility of the `MailClientPresenter`, which defines the methods `deleteMail` and `sendMail`. Both methods do what they have to do to perform the action, and then send the `modelChanged` message and update the status bar.

Therefore `MailClientPresenter` defines the menu.
Therefore `MailClientPresenter` defines the context menu for the subpresenter, creating an *action grouop*.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Therefore `MailClientPresenter` defines the context menu for the subpresenter, creating an *action grouop*.
Therefore `MailClientPresenter` defines the context menu for the subpresenter, creating an *action group*.

@koendehondt
Copy link
Collaborator

@estebanlm, @Ducasse and I discussed this PR. We will not merge it because it requires Pharo 13 and the book uses Pharo 12.

I applied this change suggested by this PR: SquareBracketAssociates/CodeOfSpec20Book@ea88929.

Let's not delete this PR so that it can serve as the basis for the book upgrade to Pharo 13, sometime during the year 2025.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants