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

Translate points in display coordinate space gap #1524

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

amartya4256
Copy link
Contributor

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.:
image

If the client sets a shell's location like:

Point p = shell.getLocation();
p.x -= 20;
shell.setLocation(p);

It would be trasnlated to the proper display coordinate space so that it's scaled in the right way.

contributes to #62 and #127

Copy link
Contributor

github-actions bot commented Oct 9, 2024

Test Results

   483 files  ±0     483 suites  ±0   8m 45s ⏱️ +54s
 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 452141c. ± Comparison against base commit ce2795a.

♻️ This comment has been updated with latest results.

@amartya4256 amartya4256 force-pushed the shell_repositioning_fix branch 2 times, most recently from 9c0c469 to 8a157cb Compare October 16, 2024 14:48
}

private Monitor getContainingMonitor(int x, int y, int width, int height) {
Point translatePointIfInDisplayCoordinateGap(int x, int y) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Point translatePointIfInDisplayCoordinateGap(int x, int y) {
private Point translatePointIfInDisplayCoordinateGap(int x, int y) {

Copy link
Contributor Author

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()) {
Copy link
Contributor

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?

Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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):

  1. 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).
  2. 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)
  3. 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.
  4. 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.

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
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.

Tackle limitation of point coordinate system
3 participants