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

Ensure windows are visible when recovering a session #2007

Merged
merged 14 commits into from Sep 10, 2020
Merged

Ensure windows are visible when recovering a session #2007

merged 14 commits into from Sep 10, 2020

Conversation

romanroibu
Copy link
Contributor

This PR fixes issues related to windows being outside the display bounds when recovering a session.

@romanroibu
Copy link
Contributor Author

@papr @pfaion The implementation currently in this PR tests if the origin of the window is within any of the connected monitors. This, however, can lead to situations where even windows with as little as 1 pixel (origin) within the monitor keep their position. I was thinking about an alternative implementation where we would calculate an intersection rectangle of the window and each monitor. The rectangle would be valid if it's at least 10x10 pixels (or any other size). If there are no valid rectangles - the position is reset to default. Wanted to see what you think about current and proposed implementations.

Copy link
Contributor

@papr papr left a comment

Choose a reason for hiding this comment

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

I like the idea about the intersection area. But since windows are only movable if their "header" is clickable, I suggest applying your logic such that the header has a minimum visible area before resetting the window to its default position.

pupil_src/shared_modules/glfw.py Show resolved Hide resolved
pupil_src/shared_modules/glfw.py Outdated Show resolved Hide resolved
@romanroibu romanroibu marked this pull request as ready for review September 8, 2020 14:59
pupil_src/shared_modules/glfw.py Outdated Show resolved Hide resolved
pupil_src/shared_modules/glfw.py Show resolved Hide resolved
@papr
Copy link
Contributor

papr commented Sep 9, 2020

@pfaion Please give this recovery logic a try on Windows. Please try it with different multi-screen setups, i.e. disconnected screen to the left/top/bottom/right.

@romanroibu romanroibu requested a review from papr September 9, 2020 09:12
@romanroibu
Copy link
Contributor Author

@papr @pfaion I incorporated all the latest feedback. Please let me know if there's anything else that might need changing.

Copy link
Contributor

@papr papr left a comment

Choose a reason for hiding this comment

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

When I was suggesting to use typing.NamedTuple, I was mostly referring to using the type annotation possibility to define the tuple, making it more readable. Please adopt the type annotation variant accordingly.

@romanroibu romanroibu requested a review from papr September 9, 2020 12:20
Copy link
Contributor

@pfaion pfaion left a comment

Choose a reason for hiding this comment

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

Works fine in different window layouts!

@papr papr merged commit 91cb5f1 into develop Sep 10, 2020
@papr papr deleted the cu-72t9b7 branch September 10, 2020 11:33
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.

None yet

3 participants