Page MenuHomeWildfire Games

Unify load game and savegame screen
ClosedPublic

Authored by elexis on Sep 14 2019, 5:43 PM.

Details

Summary

Currently these two dialogs store shared purposes:

  • loading savegames
  • displaying them in a list
  • sort the list depending on user input
  • displaying details upon selection
  • delete items
  • load or save it

Notice that one of the two dialogs was updated to use the COList with sortable columns while the other dialog hadn't incorporated that feature.
To support the sortable list in both dialogs, the easiest way to do that is by deleting the code (and merging the dialogs).

Notice also that this satisfies a warning about a style that uses a setting that doesnt exist for the object (ModernList style that specifies "olist" settings that dont exist in "list").

Load dialog before:

Save dialog before (= both dialogs afterwards):

Test Plan

Notice class syntax according to plan #5387.
Notice that upon loading the savegame dialog, it should not preselect the first item, because the user by default wants to write a new savestate, not overwrite an existing one (#4668).
Notice that upon loading the loadgame dialog, it should preselect the first item, because the user by default wants to load the most recent item.
Notice there are many more inviting cleanups, red buttons, gui/common/ removal, some ugly HACK, but needs to be declared offtopic.
Notice patch depends on rP22900 / D2289.
Make sure the translation bot is updated correctly (last release there was a big failure already when renaming folders.)

Event Timeline

elexis created this revision.Sep 14 2019, 5:43 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/685/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/170/display/redirect

elexis edited the summary of this revision. (Show Details)Sep 14 2019, 5:45 PM

There is the question how this will scale in the future in case the two dialogs receive different new features.

For example the save dialog could receive a new "Load" button, but the Load dialog cant receive a new Save button (if the game isnt running).
On the other hand an ingame Load dialog would slightly be useful, so the features would be in sync again.

But if there was such a feature difference that would make it a good idea to have it in separate GUI pages, then the common code for the savegame selection could still be externalized, avoiding the duplicate code while gaining the same features.
In order to make this commit as easily auditable as possible, the number of changes are minimized, and existing lines are not touched further than for the purpose of this diff.

Silier added a subscriber: Silier.Sep 14 2019, 6:16 PM
Silier added inline comments.
binaries/data/mods/public/gui/loadgame/load.js
92

is safe to remove selectionChanged?

in all if branches selected item is changed

elexis added inline comments.Sep 14 2019, 6:17 PM
binaries/data/mods/public/gui/loadgame/load.js
92

selectionChanged was called twice before in most cases, because its also triggered upon selected = foo. (due to the onSelection = ... handler).

Its relevant to be called if there are no savegames in order to disable the "Load" button.
And that call was moved up to init.

Stan awarded a token.Sep 16 2019, 12:11 PM

Conceptually this is reasonable, I don't think the purposes will get that different that we need completely different interfaces for these.

It seems to me you should rename LoadGame.xml/js in this diff too, before the followup cleanup diff I assume is planned.

I'm wondering if it wouldn't be better to pass the simulation data as a parameter to SavegameWriter to make it more of a purely 'logical' component instead of depending on other code.

I'm wondering if it wouldn't be better to pass the simulation data as a parameter to SavegameWriter to make it more of a purely 'logical' component instead of depending on other code.

I wondered that too and had written that too, but reverted, because the idea is to move all the savegame logic to that class, in particular obtaining the simstate may not be done in the loading dialog, or rather if the game isn't running (we may want to load Load to the ingame menu too).

It seems to me you should rename LoadGame.xml/js in this diff too, before the followup cleanup diff I assume is planned.

The SavegameWriter should contain all the logic relating to saving a match, the load.js file should contain the rest, i.e. everything but saving the data.
The plan is to split the GUI further into classes (#5387), then load.js would probably become one class that is concerned with loading (SavegameLoader.js) and one class that is concerned with displaying the savegames SavegameListHandler maybe.

(rP19351, rP19471, rP20537 might have been opportunities to notice that feature increase by unifying)

elexis updated this revision to Diff 9836.Sep 18 2019, 2:51 AM

Happily move over gui/common/functions_utility_loadsave.js.
That needed also to change the init call to updateSavegameList, otherwise it'd have broken rP19471 again.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/741/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/230/display/redirect

This revision was not accepted when it landed; it landed in state Needs Review.Sep 18 2019, 4:34 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Sep 18 2019, 4:34 AM