Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce SpMenuPresenter>>#fillWith: and add tests and example #1602

Merged
merged 5 commits into from
Sep 27, 2024

Conversation

koendehondt
Copy link
Contributor

Code changes for #1601.

  • Addition of SpCommandGroup>>#asToolbarPresenter
  • Addition of SpCommandGroup>>#asToolbarPresenterWith:
  • Addition of example
  • Some code cleanup and class comment improvements

…ToolbarPresenterWith:

Also add an example and improve the SpCommandGroup class comment.
@koendehondt
Copy link
Contributor Author

This change is important for the Spec book. Please merge the PR to Pharo 12 as well.

@Ducasse Ducasse requested a review from estebanlm September 15, 2024 20:35
@estebanlm
Copy link
Member

thing is, I removed this precisely for a reason: it does not produces a correct "DOM", and I cannot be sure that the resulting toolbar will be attached to a parent or an application (hence, I cannot know if backend will be right) :)
In this case, I dropped this in favor of the fillWith: idiom, which will end working like this:

toolbar := self newToolbar.
toolbar fillWith: aCommandGroup
... etc...

and with this approach, I do not need two methods, since the configuration of the toolbar can happen where it is created.

@estebanlm
Copy link
Member

uhm... but now I see that there are other idioms (asMenuPresenter, asMenuBarPresenter) in the same problem, I need to fix those too...

@koendehondt
Copy link
Contributor Author

uhm... but now I see that there are other idioms (asMenuPresenter, asMenuBarPresenter) in the same problem, I need to fix those too...

Please do, so that we do not write wrong patterns in the Spec book. cc @Ducasse

@Ducasse
Copy link
Contributor

Ducasse commented Sep 18, 2024

Yes this is really painful to write a book more than people can think.

@koendehondt
Copy link
Contributor Author

@estebanlm

thing is, I removed this precisely for a reason: it does not produces a correct "DOM", and I cannot be sure that the resulting toolbar will be attached to a parent or an application (hence, I cannot know if backend will be right) :)
In this case, I dropped this in favor of the fillWith: idiom

I read the code and tried some examples. You are right. The fillWith: idom is the way to go. Sadly, there are 33 senders of SpCommandGroup>>#asMenuPresenter, which give bad examples to developers.

uhm... but now I see that there are other idioms (asMenuPresenter, asMenuBarPresenter) in the same problem, I need to fix those too...

Please read the changes in this PR. I adapted the code:

  • Remove SpCommandGroup>>#asToolbarPresenter and SpCommandGroup>>#asToolbarPresenterWith: that I added initially.
  • Adapted the example SpCommandGroupExample to use SpToolbarPresenter>>#fillWith:.
  • Added SpMenuPresenter>>#fillWith: and related methods. Note the move of the initialization of the stack from SpMenuPresenterBuilder>>#initialize to SpMenuPresenterBuilder>>#menuPresenter:, otherwise building a menu or menubar would not work with a given menu or menubar (stack would still hold the default menuPresenter).
  • Added tests for the two new methods.
  • Adapted the example to send fillWith: instead of asMenuBarPresenter.
  • Added a context menu to the example, filling it with fillWith:.

I will rename this PR to express the changes.

I did not:

  • Remove SpCommandGroup>>#asMenuPresenter, as it has many senders.
  • Remove SpCommandGroup>>#asMenuPresenterWith:, although it has no senders.
  • Remove SpCommandGroup>>#asMenuBarPresenter, although it has no senders.
  • Remove SpCommandGroup>>#asMenuBarPresenterWith:, although it has no senders.

The removal should be part of another Github ticket to clean up the senders. That is out of the scope of this ticket.

If you approve the changes, I will also be happy to make a PR for Pharo 12, as Pharo 12 is the stable version the Spec book is based on.

@koendehondt koendehondt changed the title Introduce SpCommandGroup>>#asToolbarPresenter and SpCommandGroup>>#asToolbarPresenterWith: Introduce SpMenuPresenter>>#fillWith: and add tests and example Sep 25, 2024
@koendehondt
Copy link
Contributor Author

I forgot to mention: I fixed a bug in SpMenuBarPresenterTest. SpMenuBarPresenterTest>>#classToTest was missing, so that the test class used SpMenuPresenter as the class to test, not SpMenuBarPresenter.

@estebanlm estebanlm merged commit f30f58b into pharo-spec:Pharo13 Sep 27, 2024
1 of 2 checks passed
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.

3 participants