Skip to content

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 falbrechtskirchinger force-pushed the persistent-window-settings branch from d354f9f to 67b6ab0 Compare July 19, 2023 13:38
@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.

@falbrechtskirchinger falbrechtskirchinger force-pushed the persistent-window-settings branch from 67b6ab0 to a386dd1 Compare July 19, 2023 13:46
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 falbrechtskirchinger force-pushed the persistent-window-settings branch from a386dd1 to 0578d39 Compare July 20, 2023 15:36
@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
@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