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

Persist the logged-in window state #534

Merged
merged 1 commit into from
Sep 29, 2018
Merged

Conversation

sindresorhus
Copy link
Contributor

If the user quits the app or logs out, the next time they log in, the window is restored to the same position.

If the user quits the app or logs out, the next time they log in, thewindow is restored to the same position.
@lukechilds
Copy link
Member

Very cool

setWindowBounds(windowState);
const hasPositionState = Reflect.has(windowState, 'x');
if (!hasPositionState) {
win.center();
Copy link
Member

@lukechilds lukechilds Sep 28, 2018

Choose a reason for hiding this comment

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

Why do you set both setWindowBounds(windowState); and then conditionally overwrite it with win.center();. Seems expensive. Wouldn't it be more efficient to just do one or the other?

Or does setWindowBounds(windowState); do some other stuff that we need in both boolean branches so it's just easier to call that first and then overwrite x/y coords.

Or is the perf just negligible or is it not doing anything because nothing's updated between the two functions in Electron's own UI loop that it really doesn't matter and all gets bundles into a single UI action?

Copy link
Contributor Author

@sindresorhus sindresorhus Sep 29, 2018

Choose a reason for hiding this comment

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

The first time the app launches, the windowState only has width/height, not x/y. That's what hasPositionState checks. So if there's no position set, we center the window as it's nice for it to be at the center for the first launch. For the first launch it does two calls, yes, but they're very cheap and setWindowBounds only sets the dimensions, while win.center() sets the position.

@lukechilds
Copy link
Member

lukechilds commented Sep 28, 2018

Also, not directly related to this PR but slightly relevant.

I think we should increase the auto resize dimensions after login. The UI looks much better when the swap list is full width and most users should probably use the app maximised.

If you think about centralised exchanges, most users would have it open in a maximised browser tab. We're trying to fit a similar amount of information in quite a small amount of screen space.

Thoughts @atomiclabs/core?

@sindresorhus
Copy link
Contributor Author

I think we should increase the auto resize dimensions after login. The UI looks much better when the swap list is full width and most users should probably use the app maximised.

I agree. I'll do this next week. Needs more testing than I have time for this weekend.

@sindresorhus sindresorhus merged commit 420a443 into master Sep 29, 2018
@sindresorhus sindresorhus deleted the persist-window-bounds branch September 29, 2018 17:22
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.

2 participants