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

Remember window position #1751

Merged
merged 22 commits into from
Nov 15, 2023
Merged

Remember window position #1751

merged 22 commits into from
Nov 15, 2023

Conversation

lenemter
Copy link
Member

@lenemter lenemter commented Sep 9, 2023

Fixes #297

@lenemter lenemter marked this pull request as ready for review September 9, 2023 12:30
@lenemter lenemter requested a review from a team September 9, 2023 12:30
@lenemter lenemter marked this pull request as draft September 9, 2023 13:14
@vjr
Copy link
Member

vjr commented Sep 10, 2023

Thanks for this PR @lenemter , looking forward to this feature :-)

According to the comment here: https://github.com/elementary/gala/pull/1751/files#diff-ba3fbdef1da3bc4b5454c2ad54c4a2751d0df4ed0dd64ee57c7c623bc161a570R63

Is there a hurdle tracking multiple windows of the same app - anyone have any ideas to solve this?

@lenemter lenemter marked this pull request as ready for review September 10, 2023 09:26
@lenemter
Copy link
Member Author

lenemter commented Sep 10, 2023

Is there a hurdle tracking multiple windows of the same app - anyone have any ideas to solve this?

I think if an application has two windows (e.g. Files opened with 2 windows), after the first (primary) window is being closed, gala should track the second window. I'll try to implement this now, but it's worth asking @danirabbit for her opinion.

@vjr
Copy link
Member

vjr commented Sep 10, 2023

Is there a hurdle tracking multiple windows of the same app - anyone have any ideas to solve this?

I think if an application has two windows (e.g. Files opened with 2 windows), after the first (primary) window is being closed, gala should track the second window. I'll try to implement this now, but it's worth asking @danirabbit for her opinion.

What about the use case of multiple windows of the same app and not closing any of them (or closing only some of them, still leaving multiple open) and user maybe logs out or restarts?

I mean, say an app like Files or some third party say Firefox if I open 3 windows and move them around to my preferred positions, then close just 1 window (that should have its position saved) then while remaining 2 are still open, I logout/restart?

Is it possible to handle this kind of scenario in this PR? Of course, yes, will leave it to core devs / UX team opinion.

@lenemter
Copy link
Member Author

Is it possible to handle this kind of scenario in this PR? Of course, yes, will leave it to core devs / UX team opinion.

That's interesting. I'll look into this, shouldn't be too hard to implement.

Copy link
Member

@vjr vjr left a comment

Choose a reason for hiding this comment

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

This is not a code review but just wanted to mention I tried this PR and it appears to work well already in its current state.

I was going to ask about also saving apps' sizing information but looks like each app mostly does that for itself already. Tried Files, Music and Firefox.

@vjr
Copy link
Member

vjr commented Sep 10, 2023

@lenemter my quick attempt at tracking all windows: #1752

One thought I had is what to do about potential DB buildup (cleanup old/stale saved entries) over time?

@lenemter
Copy link
Member Author

lenemter commented Sep 10, 2023

One thought I had is what to do about potential DB buildup (cleanup old/stale saved entries) over time?

It shouldn't be a issue. Even if user will launch 20 unique windows everyday for 5 years, it would be only 36k entries which I think isn't too much.

@lenemter
Copy link
Member Author

lenemter commented Sep 10, 2023

@vjr I tried to implement your idea. Can you check if it is what you have in mind please?

To test it you need to remove the old database: rm ~/.local/share/io.elementary.gala-windowstate.db

@lenemter
Copy link
Member Author

lenemter commented Sep 10, 2023

If someone wants to test this PR:

There are two commits (versions) of this branch: b3203ac that implements #1751 (comment) and df0db86 that implements #1751 (comment)

@danirabbit
Copy link
Member

I think what @jeremypw had implemented in Files before was to only track the last closed window, but afaict this is working well to track all windows :) I'm sure he might have some opinions though?

Nice work! Definitely super nice to have every window remembering position <3

@jeremypw
Copy link

I think what @jeremypw had implemented in Files before was to only track the last closed window, but afaict this is working well to track all windows :) I'm sure he might have some opinions though?

Only the last closed window was remembered because only one window was restored. Saving and restoring all app windows was something I have considered but never got around to - I don't think there is a wishlist request for this? The state of each window (open tabs etc) would need to be saved so it would add to the complexity of the schema but is certainly doable if it is something that people want.

Copy link
Member

@jlnr jlnr left a comment

Choose a reason for hiding this comment

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

Even for X11, this looks much nicer than forcing every app to implement it by itself 👍

I wanted to take a look at this PR to see how it handles multi-monitor setups. What macOS seems to do is that it remembers the last screen that a window was actively moved to. Disconnecting a secondary screen collects all windows on the remaining display, re-connecting it restores things as they were before.

I don't mean to ask for extra code in this PR to cover more edge cases, but do you think it makes sense to add a column for storing the preferred screen index(?) now to avoid having to migrate the DB later on?

(Although I guess one could always come up with more fields in the future, like was_fullscreen, workspace_index etc. so maybe YAGNI should win for now?)

Copy link
Member

@danirabbit danirabbit left a comment

Choose a reason for hiding this comment

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

I'm inclined to get this in early so we can get lots of testing and potentially break the DB early if we have to

@danirabbit danirabbit requested review from jlnr and vjr November 9, 2023 21:43
Copy link
Member

@vjr vjr left a comment

Choose a reason for hiding this comment

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

I might not get to testing the PR until later in the weekend but I agree with @danirabbit comment to get this PR in early and work on any other issues , if discovered , as we go along.

@lenemter
Copy link
Member Author

@danirabbit I think currently window position is not saved before shutdown/sleep and doesn't save monitors correctly, but I'm too busy to implement it now. If this is ok for you we can merge this.

@danirabbit
Copy link
Member

Yeah we have several months to iterate! I'm sure we'll find other bugs too :shipit: 🚀

@danirabbit danirabbit merged commit 72b9e01 into master Nov 15, 2023
@danirabbit danirabbit deleted the lenemter/window-state-saver branch November 15, 2023 19:54
@orowith2os
Copy link

Late concern, and I'm not sure if I can make sense of it from the patch - what happens if you store a position and restore it when the setup changes such that the stored coords are out of bounds of the current setup?

@lenemter
Copy link
Member Author

@orowith2os I don't know. As said it's tested very badly and was merged to get more feedback.

@orowith2os
Copy link

I guess I'll be bug testing Elementary at some point in the future then - because this sounds like it could be Not Great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

Add an API to remember window position
8 participants