keep window positions #1432
keep window positions #1432
Conversation
libs/s25main/Game.cpp
Outdated
| @@ -32,15 +30,7 @@ void Game::Start(bool startFromSave) | |||
| if(startFromSave) | |||
| CheckObjective(); | |||
| else | |||
| { | |||
| if(ggs_.objective == GameObjective::EconomyMode) | |||
There was a problem hiding this comment.
we probably do not want that the addon is removed?
There was a problem hiding this comment.
It was moved to GameClient, but it should stay here
There was a problem hiding this comment.
First of all thank you for looking at it thoroughly and for all your suggestions.
I moved the Econhandler initialization to the GameClient because Game::Start() is called too late for the dskGameInterface to construct the economic progress window when the game interface is constructed.
I also think it makes more sense to initialize this with the gameclient already as that is also where the gameworld setup is being done and where we read savegame snapshots which can also lead to creation of the econhandler.
There was a problem hiding this comment.
I now moved this back here and construct the windows after game start as suggested by @Flamefire here.
libs/s25main/Game.cpp
Outdated
| @@ -32,15 +30,7 @@ void Game::Start(bool startFromSave) | |||
| if(startFromSave) | |||
| CheckObjective(); | |||
| else | |||
| { | |||
| if(ggs_.objective == GameObjective::EconomyMode) | |||
There was a problem hiding this comment.
It was moved to GameClient, but it should stay here
|
I now finished taking care of your kind suggestions for now. I didn't change the moving of the setup of the EconHandler. My reasoning for that change is found in my comment above. |
(clang-tidy suggestion)
There was a problem hiding this comment.
On the econ Handler:
I moved the Econhandler initialization to the GameClient because Game::Start() is called too late
I see you call ShowPersistentWindowsAfterSwitch in the ctor. This is actually not the right place, because the action order is: createWindow, switch, i.e. 2 separate actions. This is why Game.Start is called on activate. I'd suggest you move the call to ShowPersistentWindowsAfterSwitch into the same if-condition as the game->Start call.
I also think it makes more sense to initialize this with the gameclient already as that is also where the gameworld setup is being done and where we read savegame snapshots which can also lead to creation of the econhandler.
The GameClient does to much already. I lately moved more and more out of it into better places. The changes you needed to do to the tests IMO show that Game::Start is the better place for this. That deserialization leads to creation of the econhandler is IMO independent as due to the design of the econhandler it needs to be able to receive events and hence is part of the game state.
Bottomline: Checking for victory conditions is part of the Game, not the GameClient (which should in an ideal world just relay messages from network and windowing system), hence I'd rather see that in Game::Start and move that new function so that it works :)
| } catch(const std::out_of_range& oor) | ||
| { // Window is not persistent, no problem | ||
| } catch(std::runtime_error& err) | ||
| { // SETTINGS was probably destroyed already, don't save but print a warning |
There was a problem hiding this comment.
Is this actually possible? It should not...
god, I hate singletons...
There was a problem hiding this comment.
I added the try catch block because clang-tidy doesn't like it if destructors can throw exceptions (and using SETTINGS can at least in theory throw an exception). I don't know if it's actually possible in practice, but I guess it doesn't hurt to catch it here.
There was a problem hiding this comment.
I see. However in that case the exception should really be thrown, as it means we need to rethink that case and fully avoid it instead of just logging it (which likely no one will ever notice...).
Thinking about alternatives...
We need to set the position and the open flag. So I guess it should work to set the position in the SetPos function and isOpen in the ctor and reset in Close. Yes it means a few more calls to this code (maybe make it a function?) but I think doing bookkeeping in the destructor is not "clean" as shown by the need to handle this exception.
What do you think?
There was a problem hiding this comment.
I'm not the biggest fan, because it's easier to forget if someone adds another function that changes those values in the future. But I see your point and don't see a better alternative without using SETTINGS from the destructor, so I just changed it the way you suggested anyway.
There was a problem hiding this comment.
I know... But it's a good compromise IMO and in theory there should be tests for that new function which should also tests this... But yeah.
Thanks for the changes, LGTM
|
@Flow86 Some issue with the runner on jenkins? |
|
@Flamefire yes somethings weird with the jenkins runner. It closes the child container suddenly sometimes. Don't know why yet |
This makes ingame window positions persistent between game sessions. When an ingame window class is destructed the position and whether it was still open is written to the settings. At the opening of the GameInterface for the next game each status is checked and relevant windows opened.
Also the options whether to show build-quality, building names and productivity are saved.
All these ingame settings are stored in a new ini file to keep the main config file clutter free and so main settings are not reset when more ingame options are added.
Closes #1380.
PS: After moving halfway across the country and starting a new job I finally found time and peace of mind to turn this feature into a state in which I think it could be merged into master... that's why it took so long