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

[Settings] Restore window size and position #13912

Merged
merged 1 commit into from
Oct 25, 2021

Conversation

davidegiacometti
Copy link
Collaborator

@davidegiacometti davidegiacometti commented Oct 19, 2021

Summary of the Pull Request

What is this about:
Restore settings window size and position

What is include in the PR:

  • Using GetWindowPlacement and SetWindowPlacement to restore settings window size and position
  • Also checking if the restored windows is offscreen and rollback to default
  • Aware of multiple monitors and scaling

Let's discuss about this solution. Should I add try-catch to be more safe?

How does someone test / validate:

  • Validate that settings window is always restored and visibile

Quality Checklist

Contributor License Agreement (CLA)

A CLA must be signed. If not, go over here and sign the CLA.

@jaimecbernardo
Copy link
Collaborator

Hi @davidegiacometti , as you've mentioned, I think catching exceptions on JSON reading/parsing in order to revert to default might be good.
I'm not aware of all the possible error handling here, but checking for errors in order to revert to default is a good starting point here.

Copy link
Collaborator

@jaimecbernardo jaimecbernardo left a comment

Choose a reason for hiding this comment

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

Nice work! 😄

I think a couple of changes are still needed.
Some better error handling, reverting to default, as mentioned in the description.
If the previous window position is partially out of a monitor it still displays the Windows cut. It should revert to default when the Windows is partially offscreen, I think.

@davidegiacometti davidegiacometti force-pushed the users/davidegiacometti/issue-3685 branch from c6a8d53 to af2527d Compare October 24, 2021 12:52
@davidegiacometti davidegiacometti force-pushed the users/davidegiacometti/issue-3685 branch from af2527d to 2ab9c3f Compare October 24, 2021 15:03
Copy link
Collaborator

@jaimecbernardo jaimecbernardo left a comment

Choose a reason for hiding this comment

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

LGTM!
Thank you for the contribution!

@jaimecbernardo jaimecbernardo merged commit db90802 into master Oct 25, 2021
@davidegiacometti davidegiacometti deleted the users/davidegiacometti/issue-3685 branch October 28, 2021 18:04
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