Page MenuHomeWildfire Games

Fix missing tooltip function in replay menu following loadgame and savegame unification rP22923 / D2290
ClosedPublic

Authored by elexis on Sep 20 2019, 1:41 PM.

Details

Summary

In rP22923 / D2290 the gui/common/functions_utility_loadsave.js file was moved to the gui/load/ page folder,
because the file originally was introduced to remove duplication for code used in both save and load GUI pages.

However then the replay menu came along and used the savegame delete hotkeys for the replay menu in rP17054,
and rP18751 introduced the deleteTooltip() function in this file for replay menu, savegame and loadgame dialog equally.

So the replay menu now needs a substitute.
So either we reintroduce an ugly global function in gui/common/ loaded by every GUI page but only used by 2 pages (savegame dialog and replay menu),
or we duplicate these < 5 lines and obtain the chance to specialize the strings for the replay menu.

Test Plan

Check whether the patch is complete (wink wink). Check the phrasing of the tooltip and Intro.txt advertizement of the hotkeys to reflect that they are used in both pages.

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

elexis created this revision.Sep 20 2019, 1:41 PM

Successful build - Chance fights ever on the side of the prudent.

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

Successful build - Chance fights ever on the side of the prudent.

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

elexis added a subscriber: Nescio.Sep 20 2019, 5:13 PM
elexis added inline comments.
binaries/data/mods/public/gui/manual/intro.txt
157 ↗(On Diff #9878)

@Nescio ok with the phrasing? Anything else that could be forgotton while performing this change?

Check whether the patch is complete (wink wink). Check the phrasing of the tooltip and Intro.txt advertizement [sic] of the hotkeys to reflect that they are used in both pages.

It's probably just a typo, but to advertise is written with -ise in all variaties of English, including American, never with -ize. (Likewise, to capsize is always written with -ize, never -ise. The -ise/-ize UK/US difference only applies to words going back to Greek -ιζ- verbs.)

binaries/data/mods/public/gui/manual/intro.txt
157 ↗(On Diff #9878)

No, actually not: “saved game” is proper English, “savegame” and “saveame” [sic] are understandable but less correct.
I don't object to inserting “or replay”, but wouldn't it be better to omit the gui/manual/intro.txt string changes from this patch and include them in D2305?

elexis added a comment.EditedSep 20 2019, 5:56 PM
In D2309#96528, @Nescio wrote:

Check whether the patch is complete (wink wink). Check the phrasing of the tooltip and Intro.txt advertizement [sic] of the hotkeys to reflect that they are used in both pages.

It's probably just a typo, but to advertise is written with -ise in all variaties of English, including American, never with -ize. (Likewise, to capsize is always written with -ize, never -ise. The -ise/-ize UK/US difference only applies to words going back to Greek -ιζ- verbs.)

Nope, I wrote it wrong about 50 times:
So thanks -.-

irclogs.wildfiregames.com/2015-02/2015-02-12-QuakeNet-#0ad-dev.log:20:53 < elexis> didnt change default.cfg, i changed local.cfg as advertized
irclogs.wildfiregames.com/2016-02/2016-02-10-QuakeNet-#0ad-dev.log:17:53 < elexis> maybe the other ships (trireme) also shoot one more arrow than advertized in the tooltip
irclogs.wildfiregames.com/2016-02/2016-02-28-QuakeNet-#0ad-dev.log:18:27 < elexis> also ships shot 1 more arrow than advertized
irclogs.wildfiregames.com/2016-03/2016-03-20-QuakeNet-#0ad-dev.log:15:41 < elexis> scythetwirler: care to restart the lobby to remove the game of the banned player? and the subject could be changed to advertize alpha 20
irclogs.wildfiregames.com/2016-05/2016-05-26-QuakeNet-#0ad-dev.log:19:14 < elexis> niektb_: this way we need only one image for arbitrary colors (if that works as advertized)
irclogs.wildfiregames.com/2016-07/2016-07-12-QuakeNet-#0ad-dev.log:22:39 < elexis> if its outdated and completely broken it shouldnt be advertized
irclogs.wildfiregames.com/2016-07/2016-07-14-QuakeNet-#0ad-dev.log:22:30 < elexis> if that code works as advertized, and once we decided on a soundfile, Ill commit that
irclogs.wildfiregames.com/2016-07/2016-07-26-QuakeNet-#0ad-dev.log:14:16 < elexis> I'm ok with the patch if it works as advertized
irclogs.wildfiregames.com/2016-07/2016-07-31-QuakeNet-#0ad-dev.log:18:49 < elexis> there should be some advertizements of that patch
irclogs.wildfiregames.com/2016-08/2016-08-12-QuakeNet-#0ad-dev.log:15:08 < elexis> that merge="" works as advertized
irclogs.wildfiregames.com/2016-08/2016-08-13-QuakeNet-#0ad-dev.log:12:10 < elexis> but its well hidden in code that seems to work as advertized
irclogs.wildfiregames.com/2016-08/2016-08-21-QuakeNet-#0ad-dev.log:14:44 < elexis> simple keyword only if you want it to be advertized to new contributors
irclogs.wildfiregames.com/2016-08/2016-08-31-QuakeNet-#0ad-dev.log:09:24 < elexis> if you want the bot to advertize the games on the lobby
irclogs.wildfiregames.com/2016-09/2016-09-11-QuakeNet-#0ad-dev.log:12:07 < elexis> also if you guys find another place to advertize this feature, that would be nice. not sure where to put that hint though
irclogs.wildfiregames.com/2016-11/2016-11-03-QuakeNet-#0ad-dev.log:16:12 < elexis> looks like FileExists is advertized as a Vfs function, in the same file that handles Vfs, but actually doesn't use the Vfs
irclogs.wildfiregames.com/2016-11/2016-11-06-QuakeNet-#0ad-dev.log:13:26 < elexis> if it works as advertized
irclogs.wildfiregames.com/2017-01/2017-01-01-QuakeNet-#0ad-dev.log:16:04 < elexis> simple is used to advertize tickets to new contributors
irclogs.wildfiregames.com/2017-02/2017-02-23-QuakeNet-#0ad-dev.log:19:18 < elexis> I dont mind stuff generating income as long as it doesnt influence the game too far, dont like advertizements, optional micropayment f.e.
irclogs.wildfiregames.com/2017-02/2017-02-23-QuakeNet-#0ad-dev.log:19:18 < elexis> (ingame or forum advertizements)
irclogs.wildfiregames.com/2017-03/2017-03-23-QuakeNet-#0ad-dev.log:18:12 < elexis> totally got a central sea as advertized in atlas few generations ago
irclogs.wildfiregames.com/2017-04/2017-04-15-QuakeNet-#0ad-dev.log:17:47 < elexis> how to make a game 1. advertize its features 2. implement it
irclogs.wildfiregames.com/2017-05/2017-05-03-QuakeNet-#0ad-dev.log:15:14 < elexis> its a packet that is sent to the lobby to advertize the hosted game
irclogs.wildfiregames.com/2017-05/2017-05-22-QuakeNet-#0ad-dev.log:16:29 < elexis> Phormio: if you have an idea how we could advertize this to the player so that he doesn't have to ask this question would be appreciated
irclogs.wildfiregames.com/2017-06/2017-06-10-QuakeNet-#0ad-dev.log:13:44 < elexis> user1: can you test whether D617 works as advertized?
irclogs.wildfiregames.com/2017-07/2017-07-17-QuakeNet-#0ad-dev.log:12:59 < elexis> explaining people how they can reproduce the issue that you're proposing to fix and how we can see that the patch works as advertized and is complete
irclogs.wildfiregames.com/2017-08/2017-08-20-QuakeNet-#0ad-dev.log:20:08 < elexis> projectile speed isnt advertized though
irclogs.wildfiregames.com/2017-08/2017-08-26-QuakeNet-#0ad-dev.log:12:56 < elexis> testing that theres a deleted entity (on a map where no other entities become deleted due to gaia fights or anything) would confirm that it works as advertized
irclogs.wildfiregames.com/2017-08/2017-08-27-QuakeNet-#0ad-dev.log:15:17 < elexis> s/correct/works as advertized
irclogs.wildfiregames.com/2017-12/2017-12-25-QuakeNet-#0ad-dev.log:22:31 < elexis> we could advertize the full list of playernames
irclogs.wildfiregames.com/2017-12/2017-12-27-QuakeNet-#0ad-dev.log:20:15 < elexis> the defect that it doesnt advertize an assigned hotkey if some other hotkey is unassigned has some cost here already, but it will be the same cost in any other use case that is the targetted use case of this patch
irclogs.wildfiregames.com/2017-12/2017-12-27-QuakeNet-#0ad-dev.log:23:21 < elexis> and the reduction should be advertized
irclogs.wildfiregames.com/2017-12/2017-12-27-QuakeNet-#0ad-dev.log:23:27 < elexis> because it doesnt advertize one key if the other key is unassigned
irclogs.wildfiregames.com/2018-02/2018-02-18-QuakeNet-#0ad-dev.log:16:43 < elexis> since as far as I know one should advertize the good things
irclogs.wildfiregames.com/2018-02/2018-02-21-QuakeNet-#0ad-dev.log:23:18 < elexis> this way it would be more advertized, I believe half of the players won't notice the presence of the feature
irclogs.wildfiregames.com/2018-03/2018-03-06-QuakeNet-#0ad-dev.log:20:38 < elexis> chat with other players? did we want to advertize that as a feature or remove it?
irclogs.wildfiregames.com/2018-05/2018-05-12-QuakeNet-#0ad.log:11:16 < elexis> I'll send the link over and let you know if it works as advertized
irclogs.wildfiregames.com/2018-05/2018-05-13-QuakeNet-#0ad-dev.log:20:55 < elexis> common form of overadvertizement is presenting features that arent part of the deal, but those siege towers look way too cool to not add them
irclogs.wildfiregames.com/2018-05/2018-05-20-QuakeNet-#0ad-dev.log:17:59 < elexis> false advertizement fg
irclogs.wildfiregames.com/2018-05/2018-05-28-QuakeNet-#0ad-dev.log:17:43 < elexis> (apparently the dropping on gamestart problem is so bad that "4v4 game with lag" counts as an advertizement gamename)
irclogs.wildfiregames.com/2018-07/2018-07-31-QuakeNet-#0ad-dev.log:13:19 < elexis> "Turn on trading & patronage to support creators & make mods a part of your games success.", "If you require a private in-house mod platform for  your games that accelerates your time to market with a best-in-class  product, reach out to discuss our licensed whitelabel solution." that's all information I can find to their advertizement to have players monetize their mods
irclogs.wildfiregames.com/2018-07/2018-07-31-QuakeNet-#0ad-dev.log:13:21 < elexis> so perhaps we should rather discuss this first before we help them advertize
irclogs.wildfiregames.com/2019-06/2019-06-20-QuakeNet-#0ad-dev.log:21:24 < elexis> then the campaign goes online, advertized on all media, donors can donate to that, and then the according funds can be used for that and only that
irclogs.wildfiregames.com/2019-07/2019-07-04-QuakeNet-#0ad-dev.log:23:04 < elexis> just because it's advertized and the first thing one comes into contact with doesn't mean it's the right choice to support it, it most often means the opposite
irclogs.wildfiregames.com/2019-07/2019-07-22-QuakeNet-#0ad-dev.log:15:52 < elexis> and often the "Continue" button shouldnt be advertized
irclogs.wildfiregames.com/2019-07/2019-07-31-QuakeNet-#0ad-dev.log:16:48 < elexis> but its not a leak if it works as advertized

Advertizement -> Here it's deemed "alternative spelling", but indeed I couldn't find it on https://leo.org/.
https://en.wiktionary.org/wiki/advertizement

binaries/data/mods/public/gui/manual/intro.txt
157 ↗(On Diff #9878)

It seemed logically related to this one.
I don't mind if you want to incorporate this change.
Was there something missing in (or rather outside of) this patch otherwise?
Perhaps I forgot to update something else? :|

Nescio added inline comments.Sep 20 2019, 6:08 PM
binaries/data/mods/public/gui/manual/intro.txt
157 ↗(On Diff #9878)

Line 154 is already under discussion over there, so I think it would be better to keep it in one place, rather than change it twice.
For line 157, see lines 41 and 68.
(Also, I'd like to avoid redoing things because of file conflicts.)

Nescio added inline comments.Sep 20 2019, 6:10 PM
binaries/data/mods/public/gui/replaymenu/replay_menu.js
202 ↗(On Diff #9878)

Hold or Press? What do similar strings is similar js files use?

elexis added inline comments.Sep 20 2019, 6:13 PM
binaries/data/mods/public/gui/manual/intro.txt
157 ↗(On Diff #9878)

I was hoping to get asked to update default.cfg alongisde:

[hotkey.session.savedgames]
delete = Delete               ; Delete the selected saved game asking confirmation
noconfirmation = Shift        ; Do not ask confirmation when deleting a game

Im afraid one of us has to update this to include replays at the very least in the decription.

I also considered renaming [hotkey.session.savedgames] to [hotkey.gui], since the load dialog and replay menu are not available during the session (ingame), nor is it only used for savegames.
On the other hand I didnt find a name that encompasses precisely those two windows.

Nescio added inline comments.Sep 20 2019, 6:46 PM
binaries/data/mods/public/gui/manual/intro.txt
157 ↗(On Diff #9878)

I was hoping to get asked to update default.cfg alongisde:

If you think that's necessary, go ahead, and include it here.

since the load dialog and replay menu are not available during the session (ingame), nor is it only used for savegames.

Yeah, that complicates things (also for D2305); save game and load game are functionally similar, but save game is only available in game session and load game and replay are only available from the main menu.
What do you think is best?

elexis added inline comments.Sep 21 2019, 5:33 AM
binaries/data/mods/public/gui/manual/intro.txt
157 ↗(On Diff #9878)

If you think that's necessary, go ahead, and include it here.

No, it is necessary even if I think that it is unnecessary, because the information is still wrong and the people who will come after me reading this default.cfg will be misinformed. I want you and everyone else to inform people working on intro.txt that the information is duplicated across files and not keeping it in sync will result in bugs unless one is careful, as opposed to being negligent and not even looking at default.cfg!

Well if you prefer to update it in your D2305 okay, but then you will update default.cfg too with that change. This change should be performed... in sync.

What do you think is best?

The hotkey prefix is a different question than what should be shown in Intro.txt. I suppose I can postpone the former question due to absence of discovered solution (I guess the "session" part should be GUI at least, but then the other hotkeys like "tab" should also be in GUI.), and the latter question is for your diff if you want to perform this change in intro.txt and default.cfg.

binaries/data/mods/public/gui/replaymenu/replay_menu.js
202 ↗(On Diff #9878)

In my understanding, and many contexts in the code, press refers to the one-time event, holding to the repeated event.

So Press shift+delete to skip would work. Hold is chosen because it refers to the modification of the hotkey.

I suppose it doesnt really matter whether its in a JS, XML, TXT, config file, on the website, since all of these places will be consumed by the same reader.

elexis updated this revision to Diff 9893.Sep 21 2019, 5:36 AM

Exclude intro.txt change.
Add "while deleting".

Successful build - Chance fights ever on the side of the prudent.

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

Successful build - Chance fights ever on the side of the prudent.

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

Nescio added inline comments.Sep 21 2019, 9:24 AM
binaries/data/mods/public/gui/manual/intro.txt
157 ↗(On Diff #9878)

I want you and everyone else to inform people working on intro.txt that the information is duplicated across files and not keeping it in sync will result in bugs unless one is careful, as opposed to being negligent and not even looking at default.cfg!

Point taken. Until a few days ago I was unaware of the default.cfg file; I rarely look outside the public folder.

If a patch changes or introduces new hotkey behaviour, then yes, all occurrences (code, config, manual, wiki, etc.) ought to be updated.
If only text strings are changed, then I think it's acceptable to edit only one file in a patch (hence why I decided to have D2307 separate from D2305).

Please correct me if I'm mistaken, but my understanding of this patch (D2309) is that it improves the code but maintains the same behaviour.
With or without this patch, Delete works in the replay page regardless, therefore that should be reflected in the manual anyway, hence my preference to include it in D2305.

binaries/data/mods/public/gui/replaymenu/replay_menu.js
202 ↗(On Diff #9878)

In my understanding, and many contexts in the code, press refers to the one-time event, holding to the repeated event.

Yes, that's my understanding too, although I would have phrased it a bit differently (press: press and release; hold: press and keep pressed).
In this case Hold seems correct.

Thanks for the feedback, I suppose I can call it a review?

The patch doesnt change code, only reintroduces the one from functions_utility_loadsave.js which was moved out of common/ in rP22923.

I suppose a reviewer might also question whether this code is reintroduced correctly or could be improved.
In particular whether that "\n" + must be inside the colorizeHotkey function:
It must, because the entire stringmay fall away and string hidden if the hotkey is not assigned - then the \n must vanish too.
(In case the first hotkey is unassigned, it will result in empty string too.)

binaries/data/mods/public/gui/manual/intro.txt
157 ↗(On Diff #9878)

While the patch doesn't make it worse, we discovered the text actually being wrong, and the cost to fix the defect is editing the line in default.cfg and intro.txt.

This revision was not accepted when it landed; it landed in state Needs Review.Sep 21 2019, 11:04 PM
This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Sep 21 2019, 11:04 PM