[PATCH] Restore selection after replay

[elexis]

If one replays a game using the replaymenu, the summary screen will appear. If one closes that screen, it will return to the replay menu. It would be ideal if it would not only return there but also restore the selection and sorting order.

Those three variables (selected directory, selected column, sorting order) need to be passed

• from the replaymenu to the loading screen (reallyStartVisualReplay in replay_actions.js)
• from the loading screen to the session
• from the session to the summary screen
• from the summary screen back to the replaymenu.

Those variables need to be added to the argument of the SwitchGuiPage calls.

[Imarok]

I think the filters also should be passed

[Imarok]

Restores the selected column, selected directory, sorting order and the filter settings

[Imarok]

Only thing not working are these arrows not refreshing: Needs #3905 to work.

[Imarok]

Some whitespace cleanup, also moved the creation of replayMenuData into an own function

[Imarok]

Again whitespaces...

[elexis]

• Are you sure it is okay to set the indices on init _before_ loadReplays and displayReplayList?
• "replayMenuData" : g_GameData.replayMenuData one space too much
• Didn't notice before, but g_selectedReplayDirectory should be g_SelectedReplayDirectory

• It were better if you don't store the selection index, as at least dateTimeFilter, mapNameFilter, mapSizeFilter and durationFilter are generated dynamically.

If a replay was added or removed while replaying a game, the contents of the dropdowns would change, thus it would select something different. Not sure if it would crumble if you try to select an index bigger than the number of items.

So better pass a unique identifier there, like popCap 200, mapSize 320, mapName "Ambush" etc.

[Imarok]

Replying to elexis:

• Are you sure it is okay to set the indices on init _before_ loadReplays and displayReplayList?

You mean setting Engine.GetGUIObjectByName("dateTimeFilter").selected = data.replayMenuData.selectedFilters.date; etc in init? That is sure as these values are set in the xml, which is loaded before the js

• It were better if you don't store the selection index, as at least dateTimeFilter, mapNameFilter, mapSizeFilter and durationFilter are generated dynamically.

If a replay was added or removed while replaying a game, the contents of the dropdowns would change, thus it would select something different. Not sure if it would crumble if you try to select an index bigger than the number of items.

So better pass a unique identifier there, like popCap 200, mapSize 320, mapName "Ambush" etc.

That's right... I'll redo it

[elexis]

Replying to Imarok:

Replying to elexis:

• Are you sure it is okay to set the indices on init _before_ loadReplays and displayReplayList?

You mean setting Engine.GetGUIObjectByName("dateTimeFilter").selected = data.replayMenuData.selectedFilters.date; etc in init? That is ok as these values are set in the xml, which is loaded before the js

The contents of the dropdown filters are set after loading the replaylist. For example the map-, duration- and date-filter only contains those entries that are actually present in the replays. If I recall correctly, it was one of those two functions that fills those dropdown elements after the replays are loaded. Before that, the dropdowns should be empty. Thus telling them to select index N if they are empty sounds inappropriate (might work, but not the cleanest solution imo and wouldn't be surprised if it breaks at some point).

[Imarok]

Replying to elexis:

Replying to Imarok:

Replying to elexis:

• Are you sure it is okay to set the indices on init _before_ loadReplays and displayReplayList?

You mean setting Engine.GetGUIObjectByName("dateTimeFilter").selected = data.replayMenuData.selectedFilters.date; etc in init? That is ok as these values are set in the xml, which is loaded before the js

The contents of the dropdown filters are set after loading the replaylist. For example the map-, duration- and date-filter only contains those entries that are actually present in the replays. If I recall correctly, it was one of those two functions that fills those dropdown elements after the replays are loaded. Before that, the dropdowns should be empty. Thus telling them to select index N if they are empty sounds inappropriate (might work, but not the cleanest solution imo and wouldn't be surprised if it breaks at some point).

When loading the site the procedure is like this: load xml -> js init function -> reload some of the gui items inside the init function I e.g. first say selected index is 5 and then fill the dropdown elements. But this doesn't matter as the gui items are not reloaded before the and of init

[Imarok]

Now stores unique identifiers instead of selection indices. Also some whitespace and typo correction

[elexis]

Thanks for moving that translate to the right place in initMapNameFilter!

The patch looks much better, only few style things remain:

• if( should be if (
• If we distrust the code and ask whether data.replayMenuData exists, it might also be questionable whether data.replayMenuData.selectedFilters exists.
• In createReplayMenuData, use variables for the GUI objects. You access each GUI object twice, so it might be justified performance-wise and it makes the code a tiny bit shorter: var mapSizeFilter = Engine.GetGUIObjectByName("mapSizeFilter");

• The variable names could be improved. A good name is as specific as possible as to what the variable contains. So data and replayMenuData (in the replay menu) are to be avoided. I guess you can just pass g_GameData.replayMenuData instead of an object containing that one thing in summary.xml. You could add a variable selection = data && ... to initFilters (see ​https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Logical_Operators for how && works) and pass that selection && selection.filter && selection.filter.foo instead of data. (You will still have to check if a valid argument was given, i.e. if (foo))

• Don't add "selected" to each property of the object of that new function in replay_actions.js, but add it to the function name. Someone might want to pass data other than a selection to that menu. But until then, getSelection or similar would be the most specific name for that function.

If you think there are some good reasons to not change some of these things, let me know. None of these suggestions are carved in stone.

[Imarok]

Replying to elexis:

• If we distrust the code and ask whether data.replayMenuData exists, it might also be questionable whether data.replayMenuData.selectedFilters exists.

I think when data.replayMenuData exists, the rest(data.replayMenuData.selectedFilters etc.) also should exist. When data exists you can not be sure if data.replayMenuData exists as it may be in the future that some data is submitted when you open the replay window out of the main menu

• In createReplayMenuData, use variables for the GUI objects. You access each GUI object twice, so it might be justified performance-wise and it makes the code a tiny bit shorter: var mapSizeFilter = Engine.GetGUIObjectByName("mapSizeFilter");

Ok

• The variable names could be improved. A good name is as specific as possible as to what the variable contains. So data and replayMenuData (in the replay menu) are to be avoided.

What about calling it replaySelectionData?

I guess you can just pass g_GameData.replayMenuData instead of an object containing that one thing in summary.xml.

When someone decides in the future to pass more data to the replaymenu.js init function, all the data will be packed together (to me it seems to be neater)

You could add a variable selection = data && ... to initFilters (see ​https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Logical_Operators for how && works) and pass that selection && selection.filter && selection.filter.foo instead of data. (You will still have to check if a valid argument was given, i.e. if (foo))

Don't get why and for what I should use this. Also this seems like a really special feature, so it could make the code a bit more difficult to understand

• Don't add "selected" to each property of the object of that new function in replay_actions.js, but add it to the function name. Someone might want to pass data other than a selection to that menu. But until then, getSelection or similar would be the most specific name for that function.

That's right

[elexis]

Replying to Imarok:

Don't get why and for what I should use this.

data && data.replayMenuData && data.replayMenuData.selectedFilters && data.replayMenuData.selectedFilters.date returns data.replayMenuData.selectedFilters.date if all those properties exist, otherwise false. So the check could be inlined in some cases.

That suggestion in particular was to add a variable selectedFilters = data && data.replayMenuData && data.replayMenuData.selectedFilters so you can just pass selectedFilters.date, selectedFilters.popCap, etc. there, which might make the code a bit shorter (depending on how much properties you check).

Still not fond of using objects when they contain only one property (as I'm not sure which data besides selection items will be passed to the replaymenu) but if you want it, I will accept it.

[Imarok]

more checks for existing properties, use variables for GUI objects, rename replayMenuData to replaySelectionData, remove "selected" from every property, use && and again whitespace

[Imarok]

-existing +existence of

[elexis]

In 18085:

Restore the previously selected item and filters when returning to the replay menu. Patch by Imarok, fixes #3742.
Also few cleanups and translate the mapnames in the correct place.

[elexis]

Thanks for the patch!

Notice I changed it to also restore the compatibility filter and did these tiny codestyle optimizations:

• for consistency made Engine.GetGUIObjectByName("playersFilter") also a variable
• 1 trailing whitespace in directory": selectedDirectory,
• if( -> if (

(it's hard to do it right all the time without automation)

• missing semicolon after the returned object in createReplaySelectionData
• missing newline after that function
• updated the comment of loadReplays

[elexis]

Notice the GUI/Ordered-List (c++) bug #3905 is still experienced with this and needs separate investigation.

[elexis]

In 18935:

Scroll to the selected element after the selection changes. Fixes #4254, refs #3742.