Opened 23 months ago

Closed 19 months ago

Last modified 12 months ago

#3742 closed enhancement (fixed)

[PATCH] Restore selection after replay

Reported by: elexis Owned by: Imarok
Priority: Nice to Have Milestone: Alpha 21
Component: UI & Simulation Keywords: patch
Cc: Patch:

Description

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.

Attachments (6)

restore_selection_v1.patch (7.0 KB) - added by Imarok 20 months ago.
Restores the selected column, selected directory, sorting order and the filter settings
replaymenu.png (221.8 KB) - added by Imarok 20 months ago.
restore_selection_v1.1.patch (7.0 KB) - added by Imarok 20 months ago.
Some whitespace cleanup, also moved the creation of replayMenuData into an own function
restore_selection_v1.2.patch (7.1 KB) - added by Imarok 20 months ago.
Again whitespaces…
restore_selection_v2.patch (12.3 KB) - added by Imarok 20 months ago.
Now stores unique identifiers instead of selection indices. Also some whitespace and typo correction
restore_selection_v2.4.patch (11.4 KB) - added by Imarok 19 months ago.
more checks for existing properties, use variables for GUI objects, rename replayMenuData to replaySelectionData, remove "selected" from every property, use && and again whitespace

Download all attachments as: .zip

Change History (21)

comment:1 Changed 20 months ago by Imarok

I think the filters also should be passed

comment:2 Changed 20 months ago by Imarok

Owner: set to Imarok

Changed 20 months ago by Imarok

Attachment: restore_selection_v1.patch added

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

Changed 20 months ago by Imarok

Attachment: replaymenu.png added

comment:3 Changed 20 months ago by Imarok

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

Changed 20 months ago by Imarok

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

Changed 20 months ago by Imarok

Again whitespaces...

comment:4 Changed 20 months ago by elexis

Keywords: patch added; simple removed
Milestone: BacklogAlpha 21
Summary: Restore selection after replay[PATCH] Restore selection after replay
  • 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.

comment:5 in reply to:  4 ; Changed 20 months ago by 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

  • 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

Last edited 20 months ago by Imarok (previous) (diff)

comment:6 in reply to:  5 ; Changed 20 months ago by 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).

comment:7 in reply to:  6 Changed 20 months ago by 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

Changed 20 months ago by Imarok

Attachment: restore_selection_v2.patch added

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

comment:8 Changed 19 months ago by 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.

comment:9 in reply to:  8 ; Changed 19 months ago by 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

comment:10 in reply to:  9 Changed 19 months ago by 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.

Changed 19 months ago by Imarok

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

comment:11 Changed 19 months ago by Imarok

-existing +existence of

comment:12 Changed 19 months ago by elexis

Resolution: fixed
Status: newclosed

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.

comment:13 Changed 19 months ago by 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

comment:14 Changed 19 months ago by elexis

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

comment:15 Changed 12 months ago by elexis

In 18935:

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

Note: See TracTickets for help on using tickets.