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

Restore windows position when app is restarted #626

Closed

Conversation

rparrapy
Copy link

@rparrapy rparrapy commented Jun 3, 2016

Closes issue #181.
I am not sure about the cleanness of reading saved state in the main window module, feedback/alternative approaches are more than welcome :)

@rparrapy rparrapy changed the title Restore windows position when app is restarted, resolves #181 Restore windows position when app is restarted Jun 4, 2016
@@ -314,6 +314,8 @@ function dispatch (action, ...args) {
saveStateThrottled()
}
if (action === 'saveState') {
state.saved.window = state.saved.window || {}
state.saved.window.bounds = args[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid dispatching bounds as an argument we could do something like this instead:

{ x: window.screenX, y: window.screenY, width: window.outerWidth, height: window.outerHeight }

Thoughts?

Copy link
Author

Choose a reason for hiding this comment

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

I agree with your suggestion, did not notice that window was a available in the renderer. Dispatching bounds is more cumbersome.

@mathiasvr
Copy link
Contributor

Hi, I added some comments to your PR.

However an alternative to the current approach could be to always load the window with default settings, and then dispatch the bounds back from renderer/main.js during initialization. This way we could avoid adding State in main/window/main.js since it currently belongs in renderer/lib/.

Probably need more opinions on this though! 😅

@rparrapy
Copy link
Author

Thanks for the feedback! I agree the line comments you shared.

Maybe not including State in main/window/main.js would be more appropriate. I tried something along those lines, but I could see the window moving from the default position to the saved one, not really a nice effect. I probably messed up somewhere.

If we decide to go with this PR, I can fix the minor issues you pointed out in your comments.

@mathiasvr
Copy link
Contributor

If the movement is visible I think loading State in main/window/main.js is better 👍
We could also load the state in main/index.js and pass it to windows.main.init().
Either way the state module should probably be moved then, but maybe someone has a better idea?

@Flet Flet mentioned this pull request Aug 24, 2016
@Flet
Copy link
Member

Flet commented Sep 19, 2016

Closing this PR as this was fixed in #827 👍

@Flet Flet closed this Sep 19, 2016
@lock lock bot locked as resolved and limited conversation to collaborators May 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants