eclipse.platform.swt icon indicating copy to clipboard operation
eclipse.platform.swt copied to clipboard

Positioning shells correctly for multi zoom level

Open amartya4256 opened this issue 1 year ago • 5 comments

This contribution enforces displays to store x,y coordinates in point as the absolute pixels in the display coordinate system. while scaling the width and height to the points, following this the map methods are reimplemented to do the right conversion between the new display coordinate system and the coordinates within a control.

The top left coordinates of monitors (x, y) are stored in the absolurte pixels coordinate of the display to avoid any overlap of any monitor bounds on scaling down as per their zoom level, i.e., to locate the monitor for a point, we can easily skim and find which monitor it belongs to, hence confirming the zoom needed for scaling.

e.g. Monitor A: 100 x 100 (zoom: 100%, x-coordinates: 0 - 100) - On scaling down: 0 - 100 Monitor B: 100 x 100 (zoom: 100%, x-coordinates: 100 - 200) - On scaling down: 50 - 100 Here, the x-coordinates of A and B overlap, hence for a point at a location between (50, 100), there is ambiguity.

With the new system the scaledDown coordinates will be: Monitor A: 0 - 100 Monitor B: 100 - 150 and scaling up: 0 - 100, 100 - 200, respectively

contributes to #62 and #127

amartya4256 avatar Jul 17 '24 13:07 amartya4256

Test Results

   486 files  ±0     486 suites  ±0   9m 13s :stopwatch: +9s  4 159 tests ±0   4 151 :white_check_mark: ±0   8 :zzz: ±0  0 :x: ±0  16 390 runs  ±0  16 298 :white_check_mark: ±0  92 :zzz: ±0  0 :x: ±0 

Results for commit b88965de. ± Comparison against base commit 8c39dcdf.

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar Jul 17 '24 13:07 github-actions[bot]

Can you elaborate in more detail what this change is supposed to achieve and how to test it? I tried around a bit with multiple shells on different monitors with different scale values and the behavior is still quite broken at the border between the monitors. In particular, a shell spanning multiple monitors does not work as a proper drop target (but maybe that's not even a goal of this PR). And the region preview for a detached shell does not work properly when the new shell spans multiple monitors.

In the following video, there are two monitors next to each other (each using half of the horizontal space): hidpi_monitor_spanning_3

HeikoKlare avatar Jul 18 '24 11:07 HeikoKlare

Can you elaborate in more detail what this change is supposed to achieve and how to test it? I tried around a bit with multiple shells on different monitors with different scale values and the behavior is still quite broken at the border between the monitors. In particular, a shell spanning multiple monitors does not work as a proper drop target (but maybe that's not even a goal of this PR). And the region preview for a detached shell does not work properly when the new shell spans multiple monitors.

Regarding the second issue with the region preview for a detached shell. That is because we are identifying the monitor change only based on x and y, but we should do it by having more than 51% on the monitor, as it aligns with the scaling behavior of windows.

About the PR in general. We want to fix the issues with Shell positioning -> having them positioned on the wrong monitor or even outside of the visible area. But as we are changing the monitor bounds for that, we have a risk of breaking things.

akoch-yatta avatar Jul 19 '24 14:07 akoch-yatta

We have one limitation with the current implementation. If the top right corner of the shell is in another window, it considers the monitor to be the one where this point is and doesn't lay the bounds properly. An idea would be to use the middle of the rectangle to find the monitor. Or to find which monitor has most area of the rectangle (since the center point can also go out of all the monitors, for example when we drag the shell to the bottom, where some part of the shell can go off the screen). @akoch-yatta @HeikoKlare

amartya4256 avatar Jul 24 '24 15:07 amartya4256

@akoch-yatta @fedejeanne @HeikoKlare All the issue that was mentioned earlier after the first implementation has been fixed. After a deep investigation, I found out that the obtained monitor is sometimes faulty and the scaling is done with respect to the zoom level of the primary monitor and this can occur in case of a rectangle if its top left is out of the client area of any monitor and thats why the wrong scaling is used to determine the bounds and its placement. I have fixed this for rectangles by not using its top left corner to find the monitor but by calculating the biggest intersection between the shell and the monitor and in case the shell is completely out of all the monitors (which I can't seem to reproduce - but possible), then it will use primary monitor for scaling.

amartya4256 avatar Aug 12 '24 13:08 amartya4256

@HeikoKlare The behaviour should be more consistent now, although I'm not sure, if I was able to reproduce everything as you had. Would be great, if you could have another look on it.

akoch-yatta avatar Sep 05 '24 07:09 akoch-yatta

I've tested again, but unfortunately the behavior does still not appear to be comprehensible to me. I used the same setup as before:

  • Left monitor (primary) at 100%
  • Right monitor (secondary) at 200%
  • Workbench window spans both monitors with almost equal distribution of its area to both monitors. Area on left (primary, 100%) monitor is slightly higher, so the shell is scaled according to that monitor (100%)

This is how it looks like when I drag around an editor tab: monitor-spanning_dnd

HeikoKlare avatar Sep 05 '24 07:09 HeikoKlare

@akoch-yatta I tested again (this time only the known bug I mentioned before) and the bug is still there. This time you can see it if the window that is on the 100% monitor (on the left) does not occupy the whole monitor.

shell_repositioning_bug_2 shell_repositioning_bug_2

When dragging the view on the border of the 100% monitor, the frame is sometimes off. You can see it clearly when dragging it above the Eclipse window.

FTR this time I didn't check the new code and I did not test other use cases.

shell_repositioning_bug_2.zip

@fedejeanne I don't know, how to say it, but it seems I pushed onto the wrong remote and you just tested an incomplete version. I'm sorry, but I just pushed the changes onto the branch of this PR.

akoch-yatta avatar Sep 23 '24 12:09 akoch-yatta