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

[win32] Revert and adapt fix for coordinate system for rescaling scenario #1590

Merged
merged 2 commits into from
Nov 13, 2024

Conversation

akoch-yatta
Copy link
Contributor

@akoch-yatta akoch-yatta commented Nov 12, 2024

While testing we noticed some regressions, that were introduced with the commits 4f60cb6 and 7a04f7a. These commits were focused on providing a proper point coordinate system when multiple zoom values are involved and rescaling at runtime is active, but they also introduced behavioral changes when the rescaling at runtime is not active.

There are mainly three possible approaches to handle this regression:

  1. Properly rework the coordinate system to support multi zoom environments. This is not doable for the upcoming release, but should be done for the next release
  2. Only revert 4f60cb6 and 7a04f7a. This would solve the regression, but re-introduce some issues in the multi-zoom environment, when rescaling at runtime. As we will introduce the rescaling feature with the upcoming release via the experimental flag, this would negatively affect it.
  3. Retain the adaptions added 4f60cb6 and 7a04f7a, but properly guard them to differentiate between the scenarios with rescaling at runtime active and inactive.

This PR uses the 3rd approach. The first commit is a squashed commit to revert 4f60cb6 and 7a04f7a. The second commit reintroduces the changes with guards.

One scenario to test

Primary Monitor: 125% (Left)
Secondary Monitor: 175% (Right)

When moving the workbench to the secondary monitor and opening the search window, the window appears on the primary (fallback) monitor instead of the secondary monitor.

Copy link
Contributor

github-actions bot commented Nov 12, 2024

Test Results

   483 files  ±0     483 suites  ±0   8m 11s ⏱️ -11s
 4 095 tests ±0   4 085 ✅ ±0   7 💤 ±0  3 ❌ ±0 
16 173 runs  ±0  16 080 ✅ ±0  90 💤 ±0  3 ❌ ±0 

For more details on these failures, see this check.

Results for commit 428846c. ± Comparison against base commit d9d9bf0.

♻️ This comment has been updated with latest results.

@akoch-yatta akoch-yatta marked this pull request as ready for review November 12, 2024 11:57
Copy link
Contributor

@fedejeanne fedejeanne left a comment

Choose a reason for hiding this comment

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

This PR fixes the issue it describes (I tested).

I only have some minor comments regarding unnecessary else blocks and 1 question regarding what seems to be a change in the original code (which used autoScaleDown).

Other than that, the PR looks good. @akoch-yatta if you can address my comments then I'd like to request PMC approval to merge this one for M3.

@fedejeanne fedejeanne added regression Something that used to work HiDPI Issues related to High resolution monitors labels Nov 12, 2024
@akoch-yatta akoch-yatta force-pushed the fix-coordinate-system branch from b2aed6b to e9da217 Compare November 12, 2024 14:11
This commit reapplies the essence of the reverted commits from 4f60cb6 and 7a04f7a with proper guards to differentiate between the scenario with rescaling active and inactive.

contributes to eclipse-platform#62 and eclipse-platform#127
@akoch-yatta akoch-yatta force-pushed the fix-coordinate-system branch from e9da217 to 428846c Compare November 12, 2024 14:13
@fedejeanne
Copy link
Contributor

Requesting PMC approval to merge this one since we're on "RC1 stabilization" phase: @akurtakov / @merks

@fedejeanne
Copy link
Contributor

BTW the test failures are unrelated

@merks
Copy link
Contributor

merks commented Nov 13, 2024

+1 I will defer to you guys' good judgement.

@fedejeanne fedejeanne merged commit 0e7f799 into eclipse-platform:master Nov 13, 2024
11 of 14 checks passed
@HeikoKlare HeikoKlare added the pmc_approved Issues with PMC approval label Nov 13, 2024
@akoch-yatta akoch-yatta deleted the fix-coordinate-system branch November 15, 2024 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
HiDPI Issues related to High resolution monitors pmc_approved Issues with PMC approval regression Something that used to work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regressions regarding coordinate system change for default scenario
4 participants