Page MenuHomeWildfire Games

Add columns to load window and remove a path
ClosedPublic

Authored by vladislavbelov on Mar 21 2017, 7:59 PM.

Details

Summary

Removed useless paths to maps, removed brackets around dates.
Added colist, which allow to sort savegames by dates/names/descriptions/maps.
Added the checkbox to hide incompatible savegames.

Test Plan

Save/Load games:

  • Create compatible savegames.
  • Create partial compatible savegames (without needed mods).
  • Create incompatible savegames.

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.Mar 21 2017, 7:59 PM
Owners added a subscriber: Restricted Owners Package.Mar 21 2017, 7:59 PM
Vulcan added a subscriber: Vulcan.Mar 21 2017, 9:47 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/564/ for more details.

elexis requested changes to this revision.Mar 22 2017, 12:27 AM

Did you try arcanist already? I heard it bugs with windows though. If it doesn't work, try creating the diffs you upload with 9999 lines of unified context, so we can see the entire file when reviewing.

Nice feature.

I don't think we need to keep the orange and red savegame distinction. The user won't know either why some are red and others orange. Either it's compatible or it's not.
Looks like generateDateString will only contain Engine.FormatMillisecondsIntoDateStringLocal

I always wonder who is interested in the Map Type option. GameMode seems more interesting. Perhaps Map Type becomes more interesting in the future when we have tutorial and campaign mode though.
This is also another GUI page where we could add optional columns for people with higher resolutions than 1024x768.

binaries/data/mods/public/gui/common/functions_utility_loadsave.js
15 ↗(On Diff #882)

so remove the copy from replay_menu

binaries/data/mods/public/gui/savedgames/load.js
15 ↗(On Diff #882)

comment unneeded

28 ↗(On Diff #882)

(1) .id

(2) Those three translate calls could become a single translate call here:
cmpA = translate(a.initAttributes.settings.Name);
cmpB = translate(b.initAttributes.settings.Name);
"mapName": greyout(translate(metadata.initAttributes.settings.Name), isCompatible),
That duplication exists in the lobby too and became a bit annoying when one of fpre's recent lobby patches added another occurance iirc

(3) depending on how many attributes exist, might be shorter to write foo => ([ "prop1": ..., "prop2": ...}), didn't test whether it makes sense here

32 ↗(On Diff #882)

we very rarely use const, maybe let is more in line with the rest of gui/, dont care really

78 ↗(On Diff #882)

can it happen that the properties are undefined? can we remove the || [] checks?

83 ↗(On Diff #882)

those 2 properties don't seem to be used at all for COList, they are inherited from CList, perhaps we can address this sometime. wraitii also was asking about this in D11

87 ↗(On Diff #882)

length always exists, right? so !! wouldn't be needed?

binaries/data/mods/public/gui/savedgames/load.xml
37 ↗(On Diff #882)

MouseLeftDoubleClickItem, I think this is case insensitive

40 ↗(On Diff #882)

The strings were copied from the replay menu. Since they are in the same translation resource (messags.json), they will be reused (instead of adding a copy to another resource). The context seems pointless at first sight, but the Description string definitely should keep it's context so that translators know where it is used, so keeping the context for the neighboring strings is ok too.

Changing the context from "replay" to "savegame" doesn't seem needed, let's reuse the existing strings until someone complains on transifex.

Nothing to change here.

This revision now requires changes to proceed.Mar 22 2017, 12:27 AM
vladislavbelov edited edge metadata.
vladislavbelov marked 5 inline comments as done.

Fixed elexis notes.

binaries/data/mods/public/gui/savedgames/load.js
28 ↗(On Diff #882)

(1) Done.
(2) If we want to cache translate, then metadata should contain duplicates (translated and non-translated), because we use non-translated values too later.
(3) There are many attributes of metadata.

32 ↗(On Diff #882)

We don't change this value, so it has the reason to be const.

78 ↗(On Diff #882)

Look at the replay code, it uses the same logic. I think there could be this case.

83 ↗(On Diff #882)

We use it with all that we used with CList.

elexis requested changes to this revision.Mar 27 2017, 12:43 PM
elexis added inline comments.
binaries/data/mods/public/gui/common/color.js
165 ↗(On Diff #959)

thanks for nuking the copy&pasted function.
As agreed upon last night, this will be named compatibilityColor because it is now in common/, to avoid potential name conflicts.

binaries/data/mods/public/gui/common/functions_utility_loadsave.js
12 ↗(On Diff #959)

since we touch all lines having that name and because its in common/, should be renamed
generateSavegameDateString?

14 ↗(On Diff #959)

spaces

17 ↗(On Diff #959)

red & orange seem pointless on first sight. either its compatible or its not.
instead of showing an unexplained color, we should display the incompatibility reason in the selection details at some point.
IMO it's weird to have the first cell red&orange and the other grey, should be all the same color IMO

25 ↗(On Diff #959)

same as above, generateSavegameLabel?

binaries/data/mods/public/gui/savedgames/load.js
55 ↗(On Diff #959)

trailing whitespace

63 ↗(On Diff #959)

trailing whitespace

This revision now requires changes to proceed.Mar 27 2017, 12:43 PM
vladislavbelov edited edge metadata.
vladislavbelov marked 7 inline comments as done.
elexis accepted this revision.Mar 27 2017, 2:53 PM

These inline comments were fixed in the final version and the changes were reviewed by Vladislav in irc.
The thing also works in 1024x768.
We agreed to fix the delete dialog in the future, not colorizing the title, not showing the filename to the path and nuking the gameSelection.list property from the COList.

binaries/data/mods/public/gui/common/functions_utility_loadsave.js
16 ↗(On Diff #965)

Should _always_ pass a second argument. Should be true or false, not undefined.

This is better as it also avoid the variable and if-statement.

	return compatibilityColor(
		Engine.FormatMillisecondsIntoDateStringLocal(metadata.time * 1000, translate("yyyy-MM-dd HH:mm:ss")),
		isCompatibleSavegame(metadata, engineInfo));
binaries/data/mods/public/gui/savedgames/load.js
83 ↗(On Diff #965)

Adding // list strings used in the delete dialog for clarification. These two inherited list attributes should be removed from OList sometime.

binaries/data/mods/public/gui/savedgames/load.xml
78 ↗(On Diff #965)

checked by default in accordance with the replay menu, also because incompatible ones aren't useful.

binaries/data/mods/public/gui/savedgames/save.js
39 ↗(On Diff #965)

Not your bug, but while at it we should fix it: save.js doesn't pass the mod info to generateSavegameLabel, so displays all savegames as incompatible when trying to save new state.

This revision is now accepted and ready to land.Mar 27 2017, 2:53 PM
This revision was automatically updated to reflect the committed changes.