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

Refactor PersistentWindowSettings and persist window minimized state #1609

Conversation

falbrechtskirchinger
Copy link
Contributor

@falbrechtskirchinger falbrechtskirchinger commented Jul 18, 2023

  • Remove unneeded PersistentWindowSettings constructors and move default initialization in class.
  • Look up PersistentWindowSettings only once in IngameWindow (assumes pointer stability). This will make more of a difference when more settings are saved in these PRs:
  • Save and restore the minimized state of in-game windows. This required some more changes to avoid calling unqualified virtual Resize() from the constructor.

@Flamefire
Copy link
Member

Any chance you can come up with a test especially for the minimized state restore? Should be enough to use a dummy window with a CGI ID matching an "enabled" window

Just want to be sure we'd catch e.g. the virtual call bug in the ctor you found.

@falbrechtskirchinger
Copy link
Contributor Author

Any chance you can come up with a test especially for the minimized state restore? Should be enough to use a dummy window with a CGI ID matching an "enabled" window

Done. Let me know if this is what you had in mind.

Just want to be sure we'd catch e.g. the virtual call bug in the ctor you found.

That's rather difficult to test. I couldn't call SetMinimized() from the constructor, because that ultimately calls Resize(), and clang-tidy would rightfully warn, that this doesn't call the most derived Resize() as one might expect/want.
Instead, I'm calling Window::Resize() myself, which is already called from the constructor, so I wouldn't expect any unanticipated side effects.

Flamefire
Flamefire previously approved these changes Jul 19, 2023
auto-merge was automatically disabled July 20, 2023 15:36

Head branch was pushed to by a user without write access

@falbrechtskirchinger
Copy link
Contributor Author

@Flamefire Rebased and resolved the one-line merge conflict in IngameWindow.h. You can re-enable auto-merge.

@Flow86 Flow86 enabled auto-merge (rebase) July 22, 2023 06:54
@Flow86 Flow86 merged commit 5bf5c92 into Return-To-The-Roots:master Jul 22, 2023
20 checks passed
@falbrechtskirchinger falbrechtskirchinger deleted the persistent-window-settings branch July 22, 2023 11:16
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.

3 participants