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.

Sign up for GitHub

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

Jump to bottom

Translate points in display coordinate space gap #1524

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

Translate points in display coordinate space gap #1524

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 39 additions & 17 deletions bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Display.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3006,7 +3006,8 @@ public Point map (Control from, Control to, int x, int y) {
checkDevice ();
Point mappedPointInPoints;
if (from == null) {
Point mappedPointInpixels = mapInPixels(from, to, getPixelsFromPoint(to.getShell().getMonitor(), x, y));
Point translatedPoint = translatePointIfInDisplayCoordinateGap(x, y);
Point mappedPointInpixels = mapInPixels(from, to, getPixelsFromPoint(to.getShell().getMonitor(), translatedPoint.x, translatedPoint.y));
mappedPointInPoints = DPIUtil.scaleDown(mappedPointInpixels, to.getZoom());
} else if (to == null) {
Point mappedPointInpixels = mapInPixels(from, to, DPIUtil.scaleUp(new Point(x, y), from.getZoom()));
Expand Down Expand Up @@ -3119,7 +3120,8 @@ public Rectangle map (Control from, Control to, int x, int y, int width, int hei
checkDevice ();
Rectangle mappedRectangleInPoints;
if (from == null) {
Rectangle mappedRectangleInPixels = mapInPixels(from, to, translateRectangleInPixelsInDisplayCoordinateSystem(x, y, width, height, to.getShell().getMonitor()));
Point translatedPoint = translatePointIfInDisplayCoordinateGap(x, y);
Rectangle mappedRectangleInPixels = mapInPixels(from, to, translateRectangleInPixelsInDisplayCoordinateSystem(translatedPoint.x, translatedPoint.y, width, height, to.getShell().getMonitor()));
mappedRectangleInPoints = DPIUtil.scaleDown(mappedRectangleInPixels, to.getZoom());
} else if (to == null) {
Rectangle mappedRectangleInPixels = mapInPixels(from, to, DPIUtil.scaleUp(new Rectangle(x, y, width, height), from.getZoom()));
Expand Down Expand Up @@ -3147,19 +3149,21 @@ Rectangle mapInPixels (Control from, Control to, int x, int y, int width, int he
}

Point translateLocationInPixelsInDisplayCoordinateSystem(int x, int y) {
Monitor monitor = getContainingMonitor(x, y);
return getPixelsFromPoint(monitor, x, y);
Point translatedPoint = translatePointIfInDisplayCoordinateGap(x, y);
Monitor monitor = getContainingMonitorOrPrimaryMonitor(translatedPoint.x, translatedPoint.y);
return getPixelsFromPoint(monitor, translatedPoint.x, translatedPoint.y);
}

Point translateLocationInPointInDisplayCoordinateSystem(int x, int y) {
Monitor monitor = getContainingMonitorInPixelsCoordinate(x, y);
Monitor monitor = getContainingMonitorInPixelsCoordinateOrPrimaryMonitor(x, y);
return getPointFromPixels(monitor, x, y);
}

Rectangle translateRectangleInPixelsInDisplayCoordinateSystemByContainment(int x, int y, int width, int height) {
Monitor monitorByLocation = getContainingMonitor(x, y);
Monitor monitorByContainment = getContainingMonitor(x, y, width, height);
return translateRectangleInPixelsInDisplayCoordinateSystem(x, y, width, height, monitorByLocation, monitorByContainment);
Point translatedPoint = translatePointIfInDisplayCoordinateGap(x, y);
Monitor monitorByLocation = getContainingMonitorOrPrimaryMonitor(translatedPoint.x, translatedPoint.y);
Monitor monitorByContainment = getContainingMonitorOrPrimaryMonitor(translatedPoint.x, translatedPoint.y, width, height);
return translateRectangleInPixelsInDisplayCoordinateSystem(translatedPoint.x, translatedPoint.y, width, height, monitorByLocation, monitorByContainment);
}

private Rectangle translateRectangleInPixelsInDisplayCoordinateSystem(int x, int y, int width, int height, Monitor monitor) {
Expand All @@ -3175,8 +3179,8 @@ private Rectangle translateRectangleInPixelsInDisplayCoordinateSystem(int x, int
}

Rectangle translateRectangleInPointsInDisplayCoordinateSystemByContainment(int x, int y, int widthInPixels, int heightInPixels) {
Monitor monitorByLocation = getContainingMonitor(x, y);
Monitor monitorByContainment = getContainingMonitor(x, y, widthInPixels, heightInPixels);
Monitor monitorByLocation = getContainingMonitorOrPrimaryMonitor(x, y);
Monitor monitorByContainment = getContainingMonitorOrPrimaryMonitor(x, y, widthInPixels, heightInPixels);
return translateRectangleInPointsInDisplayCoordinateSystem(x, y, widthInPixels, heightInPixels, monitorByLocation, monitorByContainment);
}

Expand Down Expand Up @@ -5458,18 +5462,32 @@ private boolean setDPIAwareness(int desiredDpiAwareness) {
return true;
}

private Monitor getContainingMonitor(int x, int y) {
private Monitor getContainingMonitorOrPrimaryMonitor(int x, int y) {
return getContainingMonitor(x, y).orElse(getPrimaryMonitor());
}

private Optional<Monitor> getContainingMonitor(int x, int y) {
Monitor[] monitors = getMonitors();
for (Monitor currentMonitor : monitors) {
Rectangle clientArea = currentMonitor.getClientArea();
if (clientArea.contains(x, y)) {
return currentMonitor;
return Optional.of(currentMonitor);
}
}
return getPrimaryMonitor();
return Optional.empty();
}

private Monitor getContainingMonitor(int x, int y, int width, int height) {
private Point translatePointIfInDisplayCoordinateGap(int x, int y) {
// Translate only if the point doesn't lie in the point coordinate space but in
// the pixel coordinate space (the gap between the point coordinate spaces of
// the monitors)
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.

return translateLocationInPointInDisplayCoordinateSystem(x, y);
}
return new Point(x, y);
}

private Monitor getContainingMonitorOrPrimaryMonitor(int x, int y, int width, int height) {
Rectangle rectangle = new Rectangle(x, y, width, height);
Monitor[] monitors = getMonitors();
Monitor selectedMonitor = getPrimaryMonitor();
Expand All @@ -5486,15 +5504,19 @@ private Monitor getContainingMonitor(int x, int y, int width, int height) {
return selectedMonitor;
}

private Monitor getContainingMonitorInPixelsCoordinate(int xInPixels, int yInPixels) {
private Monitor getContainingMonitorInPixelsCoordinateOrPrimaryMonitor(int xInPixels, int yInPixels) {
return getContainingMonitorInPixelsCoordinate(xInPixels, yInPixels).orElse(getPrimaryMonitor());
}

private Optional<Monitor> getContainingMonitorInPixelsCoordinate(int xInPixels, int yInPixels) {
Monitor[] monitors = getMonitors();
for (Monitor current : monitors) {
Rectangle clientArea = getMonitorClientAreaInPixels(current);
if (clientArea.contains(xInPixels, yInPixels)) {
return current;
return Optional.of(current);
}
}
return getPrimaryMonitor();
return Optional.empty();
}

private Rectangle getMonitorClientAreaInPixels(Monitor monitor) {
Expand Down
Loading