-
Notifications
You must be signed in to change notification settings - Fork 139
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
Translate points in display coordinate space gap #1524
base: master
Are you sure you want to change the base?
Translate points in display coordinate space gap #1524
Conversation
Test Results 483 files ±0 483 suites ±0 8m 45s ⏱️ +54s For more details on these failures, see this check. Results for commit 452141c. ± Comparison against base commit ce2795a. ♻️ This comment has been updated with latest results. |
aceb68c
to
43f71a8
Compare
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Shell.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Display.java
Outdated
Show resolved
Hide resolved
9c0c469
to
8a157cb
Compare
8a157cb
to
47e80cb
Compare
} | ||
|
||
private Monitor getContainingMonitor(int x, int y, int width, int height) { | ||
Point translatePointIfInDisplayCoordinateGap(int x, int y) { |
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.
Point translatePointIfInDisplayCoordinateGap(int x, int y) { | |
private Point translatePointIfInDisplayCoordinateGap(int x, int y) { |
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.
Done
} | ||
|
||
private Monitor getContainingMonitor(int x, int y, int width, int height) { | ||
Point translatePointIfInDisplayCoordinateGap(int x, int y) { | ||
if(getContainingMonitor(x, y).isEmpty() && getContainingMonitorInPixelsCoordinate(x, y).isPresent()) { |
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.
I do not understand this line of code. x
and y
are point coordinates. They are passed to one method (getContainingMonitor()
) that expects these point coordinates and another (getContainingMonitorInPixelCoordinate()
) which expects coordinates in pixels. Also the subsequent call with these coordinates to translateLocationInPointInDisplayCoordinateSystem()
actually expects points.
Can you please elaborate on why this is correct and maybe also adapt the code so that it reflects via according variables and their assigments that this behavior is intended?
@akoch-yatta maybe you have some insight here?
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.
Please correct me if I am wrong @amartya4256 , but the second call is testing, if the point (if treated as pixel = as point in 100%) would be inside the bounds (black area) of a monitor. So this conditition does 1.) check this point is not in the red bounds of any monitor, but is 2.) in the black bounds of a monitor.
We should) add that information as comment here
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.
Yes @akoch-yatta is right. We need to check if the point's location provided lies in the black area and not red; in that case we need to translate.
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.
I have added a comment before the if block explaining the clause @HeikoKlare @akoch-yatta
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.
I have to admit that I still don't get this. Won't this result in "false positives", i.e, some coordinate actually lying outside of the coordinate systems but then erroneously being mapped to an actual coordinate inside it?
Imagine two monitors, left is 100% primary (800x600), right is 200% secondary (800x600). We are at the bottom right end of the point coordinate system of the right monitor (i.e., Point(800, 300)), which we move programmatically 1 point to the right. This is outside the point coordinate system, but using the proposed PR it will be mapped to pixel (1, 300) within the right monitor, won't it? That does not make any sense to me.
In general, being at the right end of a monitor which has one of these gaps to its right and adding something to the x coordinate will usually not land somewhere in the monitor right to it. E.g. consider two monitors, left is 200% primary (800x600), right is 100% secondary (800x600). I am at the bottom right end of the primary monitor (i.e., Point(400,300)) and programmatically move it 1 point to the right. This point is inside the gap, so the coordinate is treated as pixels, resulting at pixel coordinate (401, 300) within the left monitor (i.e., in the middle of that monitor) even though I moved from the bottom right end of the monitor to the right. That sounds undesired to me.
In any case, we definitely need some better documentation of this behavior. Maybe we could have an inner class of Display encapsulating all this coordinate calculation and mapping stuff to better separate it from the rest and make it easier to understand? It is already hard for me to follow what is being done inside this PR, so I am afraid that it will be pretty hard for everyone to understand what happens here without the context of this PR.
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.
If I understand your two scenarios, you are correct about the effect.
In general, we will not be able to solve all scenarios. As long as e.g. Point or Rectangle don't have its corresponding zoom as context and as long as there are methods like setLocation(x, y), you will always have to guess what to do.
This change is improving the behavior in one scenario, but will have the mentioned side effect that you can consider as unexpected. Without this PR the shell would probably positioned outside of your monitors completely.
The scenario where we noticed the gap as a problem is not something I would count as blocker, so perhaps postponing this PR to have time to rediscuss the scenarios is a valid possibility.
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.
I understand that it is hard/impossible to address all cases with the limitations of the coordinate system. However, with this change we perform some guessing anyway. In this case we simply treat point coordinates as pixel coordinates and then land at some (rather random) position, of which I do not see a reasonable relation to any coordinate that might be intended. You can even construct scenario where you move something to the right outside of a right monitor, which will be translated to a coordinate in the left monitor.
I do not completely understand why a conceptually better solution based on these facts (assuming they are correct):
- We can calculate the "gap area", i.e., those areas that are "empty space" between two monitors. In the example of the original post, that would be the area between the second and third monitor, which is 1/3 of the black bordered region (the top right part of it).
- We can calculate whether a coordinate of interest is in exactly such an area (and only inside such an area, not the over-approximation that this PR proposes to do)
- In case we have a coordinate that is inside such a gap area, we can derive the intent that the caller wanted to add something to a coordinate from a monitor to the left/top or subtract something from a monitor to the right/bottom.
- The gap regions are rather large. With 25% scaling steps they are at least 20% of a monitor's width/height. The cases we have seen so far will usually calculate coordiantes slightly outside of one monitor.
Based on these points (particularly 4): can't we calculate for a coordinate in a gap which the ' farthest` of the involved monitors is and translate the coordinate to that monitor (based on the offset within the gap to the other monitor)? In my opinion, that would best cover the actual intent behind such an operation and may also produce the most accurate/intended result.
47e80cb
to
b265c75
Compare
This contribution fixes the current behaviour of the display coordinate system of eclipse where a point can be manually set to somewhere in the gap between the 2 monitor in the coordinate space (in the points system). This fix translates those points to inside the scaled down space of a monitor providing it the right coordinates in the display coordinate space. contributes to eclipse-platform#62 and eclipse-platform#127
b265c75
to
452141c
Compare
This contribution fixes the current behaviour of the display coordinate system of eclipse where a point can be manually set to somewhere in the gap between the 2 monitor in the coordinate space (in the points system). This fix translates those points to inside the scaled down space of a monitor providing it the right coordinates in the display coordinate space.
e.g.:
If the client sets a shell's location like:
It would be trasnlated to the proper display coordinate space so that it's scaled in the right way.
contributes to #62 and #127