Skip to content

[win32] Unify scaling inside Control::setBounds #1769

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

Merged

Conversation

akoch-yatta
Copy link
Contributor

This commit unifies scale up results in Control::setBounds. With zoom values not dividable by 100 different result depending on using setBounds(Rectangle) or setBounds(int, int, int, int) were possible. Scaling the bounds always as rectangle solves this limitation.

Copy link
Contributor

github-actions bot commented Jan 27, 2025

Test Results

   494 files  ±0     494 suites  ±0   9m 23s ⏱️ -1s
 4 333 tests +1   4 320 ✅ +1   13 💤 ±0  0 ❌ ±0 
16 574 runs  +1  16 466 ✅ +1  108 💤 ±0  0 ❌ ±0 

Results for commit 8dff732. ± Comparison against base commit 23684ac.

♻️ This comment has been updated with latest results.

@akoch-yatta akoch-yatta force-pushed the fix-scaling-control-setBounds branch 2 times, most recently from 59d2913 to ad2025a Compare January 27, 2025 14:39
This commit unifies scale up results in Control::setBounds. With zoom
values not dividable by 100 different result depending on using
setBounds(Rectangle) or setBounds(int, int, int, int) were possible.
Scaling the bounds always as rectangle solves this limitation.
@HeikoKlare HeikoKlare force-pushed the fix-scaling-control-setBounds branch from ad2025a to 8dff732 Compare January 28, 2025 08:40
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 is part of completing a fix in 2016 (3b55f60) improving the initial HiDPI support (e02d49a) for fractional scale values, which was only applied to according calculations in DPIUtil but not to all consumers that contained the same inaccuracy.

@HeikoKlare HeikoKlare merged commit 081eb9b into eclipse-platform:master Jan 28, 2025
14 checks passed
@HeikoKlare HeikoKlare deleted the fix-scaling-control-setBounds branch January 28, 2025 09:13
@fedejeanne
Copy link
Contributor

@akoch-yatta this PR caused a regression --> #2003. What was the observable bug that was fixed by this PR? Is it safe to revert?

@akoch-yatta
Copy link
Contributor Author

@fedejeanne this PRs prevents rendering artifacts that would reappear when reverting. I dont't Out of my mind which ones are prevented by this change

@fedejeanne
Copy link
Contributor

I see. Well, let's discuss next week how to proceed with this since the fix seems broken. I share the idea of using only 1 logic to scale things up/down but maybe some use cases are still using the "old" approach in disguise (i.e. scaling width/height instead of calculating them like DPIUtil::scaleUp(rect, zoom) does) and this leads to #2003 .

If my assumption is correct, we should explore going exactly the other way i.e. changing DPIUtil::scaleUp(rect, zoom) so it uses the "old" algorithm, like this:

public static Rectangle scaleUp(Rectangle rect, int zoom) {
	if (zoom == 100 || rect == null)
		return rect;

	return new Rectangle(DPIUtil.scaleUp(rect.x, zoom), //
			DPIUtil.scaleUp(rect.y, zoom), //
			DPIUtil.scaleUp(rect.width, zoom), //
			DPIUtil.scaleUp(rect.height, zoom));
}

But I need to know which use cases were fixed by this PR in order to validate this proposal.

Thank you, @akoch-yatta !

@HeikoKlare
Copy link
Contributor

fwiw: the mentioned regression should have been fixed via this PR:

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