Skip to content

Conversation

@amartya4256
Copy link
Contributor

The previous scaling with the monitor context in Shell:setBounds would already scale the height and width of the Shell with respect to the monitor on which it is going to be placed. In case there is a monitor change, the monitor will be scaled with respect to the target monitor twice, i.e. in Shell::setBounds and then on the dpi changed message, leading to the effects, such as, shrinking or expanding shells on drag and drop to a different monitor with different zoom.

This PR contributes fixes that by scaling the x,y (top left coordinate) with the context of the whole rectangle but still using the zoom of the Shell for scaling the width and height. In case there's a monitor change the width and height will be scaled with respect to the target monitor on DPI_CHANGED event. Otherwise, the current zoom of the shell obtained from Shell::getZoom is enough for scaling the height and the width.

contributes to #62 and #127

@github-actions
Copy link
Contributor

github-actions bot commented Jan 16, 2025

Test Results

   494 files  ±0     494 suites  ±0   9m 10s ⏱️ +24s
 4 325 tests ±0   4 307 ✅ ±0   18 💤 ±0  0 ❌ ±0 
16 532 runs  ±0  16 384 ✅ ±0  148 💤 ±0  0 ❌ ±0 

Results for commit 2dc97c5. ± Comparison against base commit 6e219e1.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@HeikoKlare HeikoKlare left a 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 do not fully understand the reasons for double-scaling. Can you post a call sequence that produces the problem?

It also seems weird to me that translateToDisplayCoordinates obviously produces a wrong result w.r.t to width and height. So it seems as if translateToDisplayCoordinates is behaving wrong. Doesn't that method take width and height into account to identify the correct monitor? Then if these values are wrong, that identification might not work properly. If these values are wrong anyway, why not call translateToDisplayCoordinates with only a point only containing x and y and just scale width/height independently? Then there wouldn't be a useless scaling of width/height and the risk of treating them in a wrong way.

This commit contributes to the correct scaling of the Shell using its
own zoom on Shell::setBounds call as the scaling of the shell in case of a monitor
change is directly handled by the DPI_CHANGED event.

contributes to eclipse-platform#62 and eclipse-platform#127
@amartya4256
Copy link
Contributor Author

amartya4256 commented Jan 17, 2025

@HeikoKlare I have updated the PR to scale only the top left point, which kind of goes in harmony with the implementation of setLocation. However, I have a few arguments to show why in case of Shell::setBounds and setLocation, it is crucial to scale the point with context to the whole rectangle. And i would like to open a separate issue for that to reevaluate. It is only needed for an edge case but using a rectangle for translating the topLeft point of the shell would make it less error prone. we can continue this discussion on a separate issue since this issue is irrelevant to the edge case it could be effective in.

Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

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

I am still lacking an understanding of the underlying issue, as I don't know why it makes a difference that setBounds is called two times. Can you please describe a flow or, in the best case, an exact example flow with according values to understand what happens?
It currently looks to me as if the rectangle passed to getBounds does not have proper point coordinates in all cases or that translateToDisplayCoordinates does not behave correctly for rectangles. Just scaling the topleft point of the rectangle with that method obviously solves that, but still seems to only hide that the method is actually incorrect. This is also indicated by your statement:

However, I have a few arguments to show why in case of Shell::setBounds and setLocation, it is crucial to scale the point with context to the whole rectangle.

So I would appreciate if we can first get a better understanding of the issue before we can assess whether a solution is good/acceptable or not.

@HeikoKlare
Copy link
Contributor

Closing as supceded by:

If I am mistaken, please reopen or let me know @amartya4256

@HeikoKlare HeikoKlare closed this Jan 20, 2025
@amartya4256
Copy link
Contributor Author

Closing as supceded by:

If I am mistaken, please reopen or let me know @amartya4256

The behaviour still exists. The underlying problem of DPI_CHANGED event rescaling the shell size again is not fixed yet in #1711
I am working up a clearer solution which doesn't let the event override the shell size set by setBounds.

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.

Drag and Drop on an Editor on a different monitor with a different zoom leads to extra Shinking/Expanding of the dropped shell

2 participants