Page MenuHomeWildfire Games
Differential D312

Mods list doesn't save selected rows and has missed reset
ClosedPublic
Actions

Authored by vladislavbelov on Apr 10 2017, 1:27 PM.

Details

Reviewers
elexis
Commits
rP19470: Restore selected mod and fix reset button in the mod selection screen.
Trac Tickets
#4397
Summary

Few issues with the mods list:

  • If you select some row and then change filter and/or order then selection will disappear, but shouldn't, as it's for the replay list.
  • If you typed something in the filter field, pressed reset, then the field will be cleared, but the list will be filtered like if we'd press apply the filter.
  • Probably the reset button should reset the sort. order too (asc/desc).
Test Plan
  1. Run mod list
  2. Select some row
  3. Change any filter value and check that the row is still selected, if shown

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

vladislavbelov created this revision.Apr 10 2017, 1:27 PM
Vulcan added a subscriber: Vulcan.Apr 10 2017, 1:28 PM

Build has FAILED

Link to build: http://jw:8080/job/phabricator/731/
See console output for more information: http://jw:8080/job/phabricator/731/console

Harbormaster failed remote builds in B1074: Diff 1191!Apr 10 2017, 1:28 PM
vladislavbelov updated this revision to Diff 1192.Apr 10 2017, 1:28 PM
vladislavbelov edited the test plan for this revision. (Show Details)

Fixed EOLN.

Vulcan added a comment.Apr 10 2017, 2:14 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/732/ for more details.

Harbormaster completed remote builds in B1075: Diff 1192.Apr 10 2017, 2:14 PM
elexis requested changes to this revision.Apr 28 2017, 7:40 PM

If you wan't to keep the selected element, then it has to remember the a unique identifier (like mod name) and select that mod again after filtering.
Apparently it does that, but it's not working for me when I test it. If I select a mod then apply a filter again, the selection is lost.

The lobby has g_SelectedGameIP, and perhaps it would be good at this point to add a comment that it is a global because it should reselect the previously selected player if that player went offline and online again, or if the filter was changed back and forth, hiding then showing it again.

Since the latter case can occur here too, it would have to become a global too if we want it to be fancy.

Also found some probably unrelated bug, if I type 0ad it doesn't find the things I typed in, like it doesn't check for the title of that mod but only for folder names.

Those findIndex look equivalent to indexOf without arrow function

This revision now requires changes to proceed.Apr 28 2017, 7:40 PM
vladislavbelov updated this revision to Diff 1519.Apr 29 2017, 1:51 AM
vladislavbelov edited edge metadata.

Clear a description of selected mods, because user saw the description if mod was already selected.

Vulcan added a comment.Apr 29 2017, 2:54 AM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/913/ for more details.

Harbormaster completed remote builds in B1318: Diff 1519.Apr 29 2017, 2:55 AM
elexis accepted this revision.Apr 29 2017, 3:51 AM

Usually should have requested changes.

Also we should really remove those two sorting GUI objects and implement proper column header sorting.

binaries/data/mods/mod/gui/modmod/modmod.js
272 ↗(On Diff #1519)

unneeded comment

275 ↗(On Diff #1519)

Reverting the addition of this line as agreed upon in irc, since either all or none of the sorting order GUI object should be reset

296 ↗(On Diff #1519)

that can still be indexOf.
In IRC you answered me, but the answer should also be in the comments when you update the diff without taking the review remarks into account

This revision is now accepted and ready to land.Apr 29 2017, 3:51 AM
Closed by commit rP19470: Restore selected mod and fix reset button in the mod selection screen. (authored by elexis). · Explain WhyApr 29 2017, 3:55 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
ps/
trunk/
binaries/
data/
mods/
mod/
gui/
modmod/
23 lines

Diff 1522

View Options

ps/trunk/binaries/data/mods/mod/gui/modmod/modmod.js

Loading...
Wildfire Games · Phabricator