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

Prepare Renderer classes for Composites #167

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tmssngr
Copy link

@tmssngr tmssngr commented Mar 16, 2025

ControlRenderer became generic to avoid casting or control specific fields in subclasses

@tmssngr tmssngr requested review from fedejeanne and DieterMai March 16, 2025 09:49
Copy link

github-actions bot commented Mar 16, 2025

Test Results

   341 files  ±0     341 suites  ±0   2m 25s ⏱️ ±0s
 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 2983983. ± Comparison against base commit 81595b9.

♻️ This comment has been updated with latest results.

@tmssngr tmssngr force-pushed the feature/renderer-prepare-for-composites branch from ab74a52 to d4f2aa0 Compare March 16, 2025 12:08
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.

Looks ok to me.

I'm not sure if the hole renderer class hierarchy adds enough value to justify the added complexity. We have to see how this develops.

@@ -107,7 +107,7 @@ public static <T> T executeOnGC(Control control, Function<GC, T> operation) {
}
}

public static Point getTextExtent(CustomControl control, String text, int drawFlags) {
public static Point getTextExtent(Control control, String text, int drawFlags) {

Choose a reason for hiding this comment

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

I also thought of this and later realized that this might leave some methods "unhooked". See this part of #164 (comment)

That's why it overrides Control::computeSize and calls its very own (abstract) CustomControl::computeDefaultSize

What I meant is that I wasn't sure that the "old" Control hierarchy would play well with the "new" CustomControl hierarchy, e.g. I'm not sure the size of Controls would be calculated correctly. There may be other methods that are not "hooked" yet too.

Did you by any chance already checked/experienced this @tmssngr ? I haven't gotten that far yet.

Copy link
Author

Choose a reason for hiding this comment

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

I've didn't have such problems (yet), but IMHO changing the parameter type from CustomControl to Control should not have any negative impact.

@tmssngr tmssngr force-pushed the feature/renderer-prepare-for-composites branch from 8381585 to 6a07cf1 Compare March 17, 2025 08:29
@tmssngr tmssngr requested a review from HeikoKlare March 17, 2025 08:33
@tmssngr tmssngr force-pushed the feature/renderer-prepare-for-composites branch from 6a07cf1 to 3eaadd4 Compare March 17, 2025 08:35
Thomas Singer added 3 commits March 17, 2025 09:35
@tmssngr tmssngr force-pushed the feature/renderer-prepare-for-composites branch from 3eaadd4 to 2983983 Compare March 17, 2025 08:36
@tmssngr
Copy link
Author

tmssngr commented Mar 17, 2025

@HeikoKlare I'm not sure whether the commit ControlRenderer: make generic is best suitable. @DieterMai mentioned that control.getItem(i) might be harder to understand than toolBar.getItem(i).

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