-
Notifications
You must be signed in to change notification settings - Fork 185
GetLocation as Point.WithMonitor to ensure correct monitor association #2531
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
Conversation
3a2a2b2 to
b0c96c5
Compare
Test Results 118 files ± 0 118 suites ±0 10m 10s ⏱️ -46s For more details on these failures, see this check. Results for commit 87101b7. ± Comparison against base commit d10d7e4. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change does not work for me yet. It seems incomplete, as MultiZoomCoordinateSystemMapper#translateFromDisplayCoordinates() does not take the monitor of the point into account. If I adapt the mapper method to consider the monitor (like in all other translate...()) methods, it works fine.
@ShahzaibIbrahim maybe you had some other changes that we tested still in place when doing your final testing? Otherwise I don't know how this change alone may have solved the issue.
I have tested with two monitors, both at 150%, primary to the left. Then having an IDE on the right monitor (with x >> 0) and a dialog places at the border to the left monitor.
|
Yes, somehow I missed it in the testing. It's not using the monitor that's passed through the Points. I will fix it. |
00a2df5 to
e59588e
Compare
|
Failing test are unrelated, see #2516 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change works as expected for me. I just found that we now have an asymmetry of implementations for getBounds() and getLocation and actually, the part that for processing the monitor we add to the coordinate system mapper for the Point case (getLocation()) with this change is already present for the Rectangle case (getBounds()) but no monitor seems to be passed there as well.
Also, can you please add an according regression test to the coordinate system mapper so that we do not into such an issue again?
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Shell.java
Show resolved
Hide resolved
...g.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/MultiZoomCoordinateSystemMapper.java
Outdated
Show resolved
Hide resolved
30d0715 to
f3e5480
Compare
f3e5480 to
9c2f660
Compare
...eclipse.swt/Eclipse SWT Tests/win32/org/eclipse/swt/widgets/CoordinateSystemMapperTests.java
Outdated
Show resolved
Hide resolved
...eclipse.swt/Eclipse SWT Tests/win32/org/eclipse/swt/widgets/CoordinateSystemMapperTests.java
Outdated
Show resolved
Hide resolved
...eclipse.swt/Eclipse SWT Tests/win32/org/eclipse/swt/widgets/CoordinateSystemMapperTests.java
Outdated
Show resolved
Hide resolved
c32612f to
fb54c21
Compare
There was a problem hiding this 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. And also the tests seem to properly regression-test the behavior. I just have smallremarks on improving their design/coverage.
And would be nice to extend the commit message with an information that this is win32-specific.
...eclipse.swt/Eclipse SWT Tests/win32/org/eclipse/swt/widgets/CoordinateSystemMapperTests.java
Outdated
Show resolved
Hide resolved
...eclipse.swt/Eclipse SWT Tests/win32/org/eclipse/swt/widgets/CoordinateSystemMapperTests.java
Outdated
Show resolved
Hide resolved
324f6e0 to
9d6bea9
Compare
7e98183 to
324f6e0
Compare
|
The failing test should be resolved with the merging of: #2547 |
ad93129 to
bc83397
Compare
|
Test failures are unrelated: #2516 |
…sociation Previously, dialog positioning could be incorrect if the monitor was misidentified when restoring location. By returning the shell's location as a Point.WithMonitor (including monitor reference), we ensure dialogs and secondary shells open on the intended monitor, even in multi-monitor setups. This fixes issues where dialogs could appear on the wrong monitor after moving the parent shell.
bc83397 to
87101b7
Compare
Previously, dialog positioning could be incorrect if the monitor was misidentified when restoring location. By returning the shell's location as a Point.WithMonitor (including monitor reference), we ensure dialogs and secondary shells open on the intended monitor, even in multi-monitor setups. This fixes issues where dialogs could appear on the wrong monitor after moving the parent shell.
Steps to Reproduce
Ctrl + Shift + TThere are three cases which you could test and see if it preserves the previous state:
Case 1:
1 monitor, dialog should open where it was last closed as per parent shell. if child dialog is 10 px to the right to the parent shell, it should be 10 px from the parent shell doesnt matter where parent shell is or what size it is.
Case 2:
2 monitor, dialog moved to the left monitor, for e.g. -324 close the dialog. Reopen it should be -324 px from the parent shell.
Case 3:
2 monitor, parent shell is moved to the left monitor, dialog was closed to right monitor. Now it should follow parent shell.