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

ToolBar: use new Renderers #171

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft

Conversation

tmssngr
Copy link

@tmssngr tmssngr commented Mar 16, 2025

No description provided.

@tmssngr tmssngr requested a review from DieterMai March 16, 2025 12:36
Copy link

github-actions bot commented Mar 16, 2025

Test Results

   341 files  ±0     341 suites  ±0   2m 46s ⏱️ +9s
 3 955 tests ±0   3 671 ✅ ±0  284 💤 ±0  0 ❌ ±0 
11 693 runs  ±0  10 808 ✅ ±0  885 💤 ±0  0 ❌ ±0 

Results for commit ea87dad. ± Comparison against base commit 9f93921.

♻️ This comment has been updated with latest results.

@tmssngr
Copy link
Author

tmssngr commented Mar 16, 2025

#167 should be applied before

Copy link

@DieterMai DieterMai left a comment

Choose a reason for hiding this comment

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

The usage of "control" instead of (Tool)"bar" in unfortunate and obfuscates the meaning a bit
control.getItem(i) -> A control does not have items. What is returned?
toolbar.getItem(i) -> Obvious: the items from the toolbar

@fedejeanne
Copy link

The usage of "control" instead of (Tool)"bar" in unfortunate and obfuscates the meaning a bit control.getItem(i) -> A control does not have items. What is returned? toolbar.getItem(i) -> Obvious: the items from the toolbar

A consequence of parameterizing ControlRenderer <...>. I agree that the name of the field is now a bit "too general" but I prefer that over having each subclass declare its own field since in my experience, parameterization ends up avoiding subtle bugs.

@tmssngr tmssngr force-pushed the feature/toolbar-renderer branch 2 times, most recently from 7b2670c to fa8625e Compare March 17, 2025 13:52
@tmssngr tmssngr marked this pull request as draft March 17, 2025 15:29
@tmssngr tmssngr force-pushed the feature/toolbar-renderer branch from fa8625e to 5ed1abb Compare March 17, 2025 15:29
@tmssngr
Copy link
Author

tmssngr commented Mar 17, 2025

@HeikoKlare Should we have delegating methods like in ButtonRenderer to access properties, or should they be called directly like in the ToolBarRenderer? Note, for the ButtonRenderer at least getImage() would be required.

@HeikoKlare
Copy link

I am not completely sure what you mean by "delegating methods". Are you referring to accessors? In general, I don't like direct access to fields from other classes, as you then need to take access and changes to those values into account when considering the contract of your class (in particular its invariants). So providing explicit accessors to state and what is allowed to be queried or changes is usually beneficial.

…e hints

expecially in DefaultToolBarRenderer.computeSize it is not obvious
whether computeLayout changes the passed Point object.
@tmssngr
Copy link
Author

tmssngr commented Mar 17, 2025

I am not completely sure what you mean by "delegating methods". Are you referring to accessors?

I mean: what do you prefer: DefaultButtonRenderer.computeDefaultSize() accessing ButtonRenderer.getText() accessing Button.getText(), or DefaultToolBarRenderer.render() accessing ToolBar.getItem() (one less indirection)?

Maybe related to #174.

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.

None yet

4 participants