Page MenuHomeWildfire Games

Hotkey summary, hotkey tabs and panel/charts remember

Authored by ffffffff on Aug 20 2017, 4:33 PM.



I suggest having hotkey for in-game summary and replaymenu summary as Ctrl + Tab.

Switch to next tab hotkey in general (summary, credits and options page) Alt + S.
and switching to previous tab hotkey Alt + W.

Changing therefore silhoutte hotkey from Alt + S to Alt + Shift + S.

Remember showed panel and showed charts for next opening.

trac #3842

Test Plan


Diff Detail

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

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
bb added inline comments.Nov 14 2017, 4:34 PM
128–136 ↗(On Diff #4165)

maybe is correct, maybe not: why do we need to have g_DefaultPanel? and can't we just set here g_ViewPanel = "scorePanelButton" and then just let the init overwrite that when it wants to (like it already does).

145–146 ↗(On Diff #4165)

Correct since if g_ViewPanel == "" then slicing just remains "" and that is not an element of panels. (Noting for self, no change needed)

146–147 ↗(On Diff #4165)

Aww well (index + direction) % panels.length means (index + direction) modulo panels.length So that returns the remainder of dividing (index + direction) by panels.length, in the range [0, panels.length - 1]. So -1 % panels.length returns panels.length - 1 thus so we can remove the second ternary.

Also nextIndex is used only once so inline.

517 ↗(On Diff #4165)

with removing g_DefaultPanel this would simplify to selectPanel(Engine.GetGUIObjectByName(g_ViewPanel));

ffffffff added inline comments.Nov 15 2017, 9:42 PM
146–147 ↗(On Diff #4165)


-1 % panels.length = -1

ffffffff updated this revision to Diff 4207.Nov 15 2017, 9:44 PM
ffffffff updated this revision to Diff 4208.Nov 15 2017, 9:54 PM
bb accepted this revision.Nov 15 2017, 11:19 PM

That one js rubbish style thing can be done when committing.

87 ↗(On Diff #4208)

A yeller could yell that the tab should get an own line, but not required for me

141–142 ↗(On Diff #4208)

It is a js disgrace to take the remainder as negative for negative values imo, (index + direction + panels.length) % panels.length

This revision is now accepted and ready to land.Nov 15 2017, 11:19 PM
elexis added inline comments.Nov 15 2017, 11:39 PM
87 ↗(On Diff #4208)

but but but

54 ↗(On Diff #4208)

(maybe g_SelectedSummaryPanel and the jsdoc could state that it remembers the name of the panel, but the current part is ok too)

129 ↗(On Diff #3458)


bb added inline comments.Nov 15 2017, 11:41 PM
129 ↗(On Diff #3458)

@elexis reload the page

ffffffff updated this revision to Diff 4218.Nov 16 2017, 7:05 PM
ffffffff added inline comments.
521 ↗(On Diff #4218)

some more juice here.
go next/prev panel on mouse scroll wheel up/down over the tab buttons.

24 ↗(On Diff #4218)

also close summary on cancel. (ESC)

ffffffff updated this revision to Diff 4219.EditedNov 16 2017, 7:12 PM

updated the jsDoc string "* Remember the name of the currently opened view panel." on all three occurences.

elexis added inline comments.Nov 16 2017, 7:15 PM
24 ↗(On Diff #4218)

Closing dialogs with escape is a good idea.
Closing stacked pages with escape like the ingame summary is reasonable.
But switching the GUI page if one can't get back with escape arguably leaves room for user mistakes.
For instance if a lobby game was played and the summary page is seen, one can't go back to that screen after having pressed escape.
Furthermore it would be inconsistent, because all other buttons that switch GUI pages don't show that behavior and for the same reason shouldn't implement it.
The disadvantages in this case are tolerable though.
(But I recall giving the same argument already for closing some other GUI page with escape)

bb added inline comments.Nov 16 2017, 7:21 PM
141–142 ↗(On Diff #4218)

see previous comment

513 ↗(On Diff #4218)

As we use this thing twice now Object.keys(g_ScorePanelsData).concat(["charts"]) make it a global, also do Object.keys(g_ScorePanelsData).push("charts")
And just add the "PanelButton" to each element in that global that can be nuked in both usages

514 ↗(On Diff #4218)

When being in the little space between the buttons, the scrolling doesn't work, probably just meh, since it would run in some inconsistencies when we allow it.

521 ↗(On Diff #4218)

That should be documented too in the intro.txt

Note to self: add things to the wiki when committed

ffffffff added inline comments.Nov 16 2017, 7:22 PM
24 ↗(On Diff #4218)

so whats ur decision

elexis added inline comments.Nov 16 2017, 7:43 PM
87 ↗(On Diff #4208)

Every hotkey on a separate line, meaning two new lines added for the two other new hotkeys.
I don't see why these two hotkeys should not be used on any other GUI page using tabs as well (credits, options).

24 ↗(On Diff #4218)

In the definition of the cancel hotkey it says "Close all dialogs".
A dialog is a GUI object or page that is stacked. Switching a GUI page is not stacking, hence this diff stands in contradiction to the definition of the hotkey.
The user shouldn't accidentally close a page he can't recover (by switching to another page).

If we want a hotkey switching GUI pages as opposed to closing dialogs, then it should be a different hotkey (like shift+escape) with a custom description.

We know that continueButton() closes the current GUI page if we're ingame and switches the GUI pages unrecoverably otherwise.

So the cancel hotkey could close the summary page if isInGame and don't do anything otherwise.

ffffffff added inline comments.Nov 16 2017, 7:59 PM
87 ↗(On Diff #4208)


very true.

lets do it.

/ exec

ffffffff added inline comments.Nov 16 2017, 8:00 PM
24 ↗(On Diff #4218)

away with it

ffffffff updated this revision to Diff 4220.Nov 16 2017, 8:03 PM
ffffffff updated this revision to Diff 4221.
ffffffff added inline comments.Nov 16 2017, 8:06 PM
216 ↗(On Diff #4221)

cant use push when afterwards wanna use another function on return cause push doesnt return an array. can concat stay?

513 ↗(On Diff #4218)

cant use push when afterwards wanna use another function on return cause push doesnt return an array. can concat stay?

Grugnas removed a subscriber: Grugnas.Nov 16 2017, 8:07 PM
elexis added inline comments.Nov 16 2017, 8:24 PM
87 ↗(On Diff #4208)

The script printed something to stderr.

FS introduces three new hotkeys, so every hotkey being mentioned on a separate line means adding thee new lines.
Also I don't see this diff implementing the hotkey in the tabs of the options or credits page. This can be done in a separate diff, but at least the hotkey description should not speak of the summary screen but tabs in general.

Also means renaming summary.nextpanel to something like (and tabs.previous) respectively.

Also this should be located the (only other?) dialog hotkey, not near the ingame hotkeys which are used for moving units around and such.

(Unrelated, but if bb wants to do it in the same go when editing the wiki page: Technically not a hotkey (since those a virtual), but the nick autocomplete key (which is hardcoded in CInput.cpp) isn't found in the wiki page).

514 ↗(On Diff #4221)

Nice, didn't notice so far. Would also be useful in the other GUI pages with tabs and consistent.

ffffffff updated this revision to Diff 4227.Nov 16 2017, 11:28 PM
elexis added inline comments.Nov 16 2017, 11:55 PM
329 ↗(On Diff #4227)


260 ↗(On Diff #4227)

Not convinced that this global function is worth it. We should unify the tabbing code eventually, still this, especially when also looking at the calls to this, looks ugly.

267 ↗(On Diff #4227)

As in return (index + direction + indexArray.length) % indexArray.length; ?
I suspect this function can go too following this simplification and given that indexArray are global constants.

4 ↗(On Diff #4227)


53 ↗(On Diff #4227)

Switch to the next tab and Switch to the previous tab (I'm more a fan of the word Select) should do.
Forward switch to next tab sounds like Backward switch to next tab would also be possible.

163 ↗(On Diff #4227)

selectNextTab and using that name in all gui pages?

62 ↗(On Diff #4227)

currently viewed panel

64 ↗(On Diff #4227)

g_SelectedSummaryPanel or g_SummarySelectedPanel?

221 ↗(On Diff #4227)

g_SelectedPanel IMO

130 ↗(On Diff #4227)

With the exception of typed arrays, there is only one JS number type I'm aware of and that's called number. The comment implies the type, so the type definition could be avoided too.

24 ↗(On Diff #4218)

Not away. but if (g_GameData.gui.isInGame), newline, continueButton();

ffffffff added inline comments.Nov 17 2017, 12:30 AM
53 ↗(On Diff #4227)


ffffffff added inline comments.Nov 18 2017, 8:14 PM
53 ↗(On Diff #4227)

Select doesnt mean switch. or?

ffffffff added inline comments.Nov 18 2017, 8:15 PM
53 ↗(On Diff #4227)

Select doesnt mean necessarily switch. or? could also be only select without switch. IMO not clear.

ffffffff updated this revision to Diff 4264.Nov 18 2017, 8:25 PM


ffffffff marked 3 inline comments as done.Nov 21 2017, 6:38 AM
ffffffff updated this revision to Diff 4282.Nov 21 2017, 6:48 AM

Unified used terms for functions and variables.

Inlining nextIndex.

im not sure but im feelin somehow better with arrow keys up/down (options/credit tabs) left/right (summary tabs) for tab switch. maybe we should change keys.

also tab can be better used and is already used for other things (like showing health bars or tabbing names)

bb added a comment.Nov 27 2017, 8:45 PM

That last comment is spot on, in gamesetup f.e. a tab hotkey won't work (tested in D1027). So I guess we need to use an unique combination for this.

216 ↗(On Diff #4282)

k => key (or panel)

505 ↗(On Diff #4282)

k => button

ffffffff updated this revision to Diff 4568.Dec 5 2017, 6:13 PM
ffffffff retitled this revision from Having some summary in-game hotkey and panel remember to Hotkey summary, hotkey tabs and panel/charts remember.
ffffffff edited the summary of this revision. (Show Details)

Update keys according to bb states.

Remember charts selected to.

ffffffff updated this revision to Diff 4569.Dec 5 2017, 6:15 PM

wrong intro.txt diff.

bb accepted this revision.Dec 11 2017, 7:05 PM

changing that one hotkey in two places and unlooping twice can be done with committing

unrelated @elexis the autocomplete is neither in intro.txt

149 ↗(On Diff #4569)

Here another
(I guess shift+Alt+W will do?)

150 ↗(On Diff #4569)

not used that often so => OK

151 ↗(On Diff #4569)

\0/ that has annoyed me a lot

162 ↗(On Diff #4569)

that is looping twice over the same thing, first in the map then the for. Move the map into the for scope so:

for (let button of g_PanelsButton)
   tab = Engine.GetGUIObjectByName(button); // (or find a better name for tabObject)

Thanks for the patch fpre, that feature will ease access to the summary screen significantly.
And thanks for having it abstracted and implemented it on the other two GUI pages too.
Patch looks very well.

In D810#45749, @bb wrote:

unrelated @elexis the autocomplete is neither in intro.txt

Since it's not a hotkey, but it might become eventually I suppose

53 ↗(On Diff #4227)

Its true though because the Forward in Forward switch qualifies the switch.

53 ↗(On Diff #4569)


138 ↗(On Diff #4569)

= undefined; should be removed (we got a linter rule)

162 ↗(On Diff #4569)


ffffffff updated this revision to Diff 4720.Dec 11 2017, 8:22 PM

refactor as wished.
taking tab as hotkey

i hope remap tab from show status bar to shift+tab and taking tab for summary window is ok, cause it annoys me realy that thers no quick grap for summary window and the combination hot keys are quite limited and not fast to grap. not switching forward backwards with tab key is already hard to get up to. :)

btw are u feeling comfortable with D1058?

bb added inline comments.Dec 11 2017, 10:25 PM
151 ↗(On Diff #4720)

Should be Alt+Shift then (also seeing the web)

486 ↗(On Diff #4720)


ffffffff edited the summary of this revision. (Show Details)Dec 11 2017, 10:34 PM
bb accepted this revision.Dec 11 2017, 10:43 PM

No need for new patch

115 ↗(On Diff #4727)

another one

ffffffff added inline comments.Dec 11 2017, 11:00 PM
115 ↗(On Diff #4727)

ok thx

This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Dec 11 2017, 11:16 PM