Skip to content

Conversation

@amartya4256
Copy link
Contributor

@amartya4256 amartya4256 commented Sep 18, 2025

With both Async and Sync DPI scaling, ImageBasedFrame scales itself on ZoomChanged event to pack the control to wrap in it. However, with the current approach of scaling on DPI_CHANGED it works fine as it only receives the event after the children have been resized. However, since ImageBasedFrame is a Control wrapped inside a Canvas, those widgets are already scaled implicitly and there's no need to pack them on ZoomChanged event but rather when a Resize event is received at the parent, allowing the ImageBasedFrame to only recalibrate its size when the parent is resizing.

This approach moves the responsibility of handling ZoomChanged event from outside SWT for ImageBasedFrame and also provides a possibility to scale it properly when the scaling is Async, i.e. the children might not have scaled when ZoomChanged is received.

###How to test
Simply A/B testing with master to check if the toolbar images scale properly since they are all wrapped in ImageBasedFrames, especially the perspective switcher in the top right.

Before:
image

After:
image

This PR improves the state of eclipse-platform/eclipse.platform.swt#2520, although not dependent on it. Must be merged before it.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 18, 2025

Test Results

 2 904 files  ±0   2 904 suites  ±0   1h 55m 53s ⏱️ - 8m 52s
 8 017 tests ±0   7 772 ✅ +1  245 💤 ±0  0 ❌  - 1 
23 585 runs  ±0  22 803 ✅ +1  782 💤 ±0  0 ❌  - 1 

Results for commit 266df0d. ± Comparison against base commit c2fe144.

♻️ This comment has been updated with latest results.

@fedejeanne
Copy link
Member

How does one test this?

@amartya4256
Copy link
Contributor Author

How does one test this?

It's just an improvement in how it ImageBasedFrame implemenation for this case. I would expect ImageBasedFrame to behave the same as it does right now on master, with resize of Parent and on DPI change.

Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

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

This will now react to every resize event and on every OS with a relayouting of the parent and other adaptations to execute. Since this kind of adaptation only seems to be necessary on a DPI change, is that really the right thing to do? I would expect these adaptations to then be executed far too often.

@fedejeanne
Copy link
Member

How does one test this?

It's just an improvement in how it ImageBasedFrame implemenation for this case. I would expect ImageBasedFrame to behave the same as it does right now on master, with resize of Parent and on DPI change.

Then please provide a snippet (or name an existing one) so that we can validate the behavior and see if your expectations are met :-)

@amartya4256
Copy link
Contributor Author

amartya4256 commented Sep 18, 2025

This will now react to every resize event and on every OS with a relayouting of the parent and other adaptations to execute. Since this kind of adaptation only seems to be necessary on a DPI change, is that really the right thing to do? I would expect these adaptations to then be executed far too often.

I agree. We can make this resize handler dependent on after ZoomChanged event. How do you like such idea?

AtomicBoolean isPostZoomChangedEvent = new AtomicBoolean(false);

toWrap.addListener(SWT.ZoomChanged, event -> isPostZoomChangedEvent.set(true));

parent.addListener(SWT.Resize, event -> {
	if (isDisposed() || !isPostZoomChangedEvent.get())
		return;
	toWrap.pack(true);
	setFramedControlLocation();
	parent.layout();
	isPostZoomChangedEvent.set(false);
});

@HeikoKlare
Copy link
Contributor

That would definitely be better. Couldn't even the SWT.ZoomChanged listener register the SWT.Resize listener as a one-time listener? I.e., the SWT.ZoomChanged listener would register the SWT.Resize listener that deregisters itself once it was executed.

@amartya4256 amartya4256 force-pushed the amartya4256/imageBasedFrame_dpi_handling_fix branch from ed0ded8 to da8190d Compare September 19, 2025 11:36
@amartya4256
Copy link
Contributor Author

That would definitely be better. Couldn't even the SWT.ZoomChanged listener register the SWT.Resize listener as a one-time listener? I.e., the SWT.ZoomChanged listener would register the SWT.Resize listener that deregisters itself once it was executed.

Adapted the suggested change.

@HeikoKlare
Copy link
Contributor

Adapted the suggested change.

@amartya4256 is the latest change as intended? My proposal was to keep the SWT.Resize listener but only register and execute it once, but now we are back to only having an (enhanced) SWT.ZoomChanged listener. If that works, it's of course fine.

@amartya4256 amartya4256 force-pushed the amartya4256/imageBasedFrame_dpi_handling_fix branch from da8190d to 12ca9be Compare September 19, 2025 12:53
@amartya4256
Copy link
Contributor Author

My bad, I pushed without adding the latest change ;D

Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

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

The change works fine for me. I only have minor comments.

@amartya4256 amartya4256 force-pushed the amartya4256/imageBasedFrame_dpi_handling_fix branch 2 times, most recently from 62e0987 to 8770526 Compare September 19, 2025 15:15
@HeikoKlare HeikoKlare force-pushed the amartya4256/imageBasedFrame_dpi_handling_fix branch from 8770526 to b063676 Compare September 20, 2025 08:37
@HeikoKlare HeikoKlare force-pushed the amartya4256/imageBasedFrame_dpi_handling_fix branch from b063676 to 0309086 Compare September 20, 2025 09:47
This commit adapts ImageBasedFrame scaling to be triggered when it's
parent recieves a resize event after ZoomChanged event on its wrapped
control.
@HeikoKlare HeikoKlare force-pushed the amartya4256/imageBasedFrame_dpi_handling_fix branch from 0309086 to 266df0d Compare September 22, 2025 06:11
@HeikoKlare HeikoKlare merged commit 2f6a390 into eclipse-platform:master Sep 22, 2025
18 checks passed
@HeikoKlare HeikoKlare deleted the amartya4256/imageBasedFrame_dpi_handling_fix branch September 22, 2025 07:50
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.

Improve ImageBasedFrame scaling logic to make it more dynamic

3 participants