Skip to content

Save logical position when restoring on macos #4061

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
Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Save logical position when restoring on macos #4061

wants to merge 6 commits into from

Conversation

clouds56
Copy link
Contributor

@clouds56 clouds56 commented Apr 29, 2025 •
edited
Loading

On macOS, it behaves strange with multiple monitor physical location. That is to say

Consider we have 2 monitor main and extra which physical size is 2560x1440 (scale: 2.0) and 1920x1080 (scale: 1.0)

+-----------+---------+
|     .<-A  |  .<-B   |
|   main    |  extra  |
| 2560x1440 |1920x1080|
|           |---------+
+-----------+

The physical position of A is (1200, 100), and the logical position of it would be (600, 50) due to scale factor.
The code before this PR would save (1200, 100) to "window-state.json" and everything just works.

But for point B, which "relative" physical position is (100, 100) (offset from left-top of "extra" monitor),
In this case, winit would tell the physical position is (1380, 100), which is the same as logical position (1380, 100).
If we save (1380, 100) to state file, and when restore, the winit would think we are at main screen and position to (640, 50) in logical position.

Point name p1 = window.outer_position() p2 = p1.to_logical(window.scale_factor()) restored p1 (in logical) restore p2
A PhysicalPosition(1200, 100) LogicalPosition (600, 50) LogicalPosition (600, 50) LogicalPosition (600, 50)
B PhysicalPosition(1380, 100) LogicalPosition (1380, 100) LogicalPosition (640, 50) LogicalPosition (1380, 100)

TL;DR, in macOS the position system is consist in "logical" coordinates.

Edit:
This seems only macOS related, so I restrict patch only on cfg!(target_os = "macos")

There're several related issues you could reference

@clouds56 clouds56 requested a review from a team as a code owner April 29, 2025 20:53
@clouds56 clouds56 marked this pull request as draft April 29, 2025 21:54
@clouds56
Copy link
Contributor Author

clouds56 commented Apr 29, 2025 •
edited
Loading

I found let adjustment = 56 failed on extra monitor.

Update:
It should be LogicalSize(0, 28) empirical.
(though I cannot find any documentation, the original code introduced in e923c64, could @jkelleyrtp kindly explain why it used to be 56 here?)

Tested on my macOS (15.4.1) and windows (11 24H2)

@clouds56 clouds56 marked this pull request as ready for review April 30, 2025 04:07
@clouds56 clouds56 changed the title Save logical position on restore Save logical position when restoring on macos Apr 30, 2025
@clouds56
Copy link
Contributor Author

What's blocking here?

@jkelleyrtp
Copy link
Member

jkelleyrtp commented Jun 19, 2025 •
edited
Loading

Thanks for the PR!

Should we implement LogicalPosition for all platforms, not just macos? Does it make sense to maintain both?

I believe the 56 offset is meant to handle the thickness of the topbar, otherwise it marches down on a reload.

Also CI not passing is blocking this PR, might be worth merging main to see if the failures go away.

@clouds56
Copy link
Contributor Author

On macos, there's 1-1 mapping from points on screen to points in Logical Position, (and there's 2 points on screen have the same expression in Physical Position, like (1380, 100))
While on windows, there's 1-1 mapping from points on screen to points in Physical Position.

I once tried use logical position on windows but it failed to restore to the same location if 2 monitors have different scale. So I revert it in 0fbd886

For CI failure, I think there's some network issue, can re rerun it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@clouds56 @jkelleyrtp