Page MenuHomeWildfire Games
Differential D1046

Give Message Box Confirmation Hotkey Binding
Needs ReviewPublic
Actions

Authored by ffffffff on Nov 18 2017, 8:23 AM.

Details

Reviewers
elexis
bb
historic_bruno
Trac Tickets
#4411
#4985
Summary

Advance msgbox to have confirmation hotkey support.

Bind certain callback from btnCode to confirm hotkey.

Default when only one button both confirmation and cancel hotkey bind to the >one< callback. (Like assume its info box with "Ok")
Default when two buttons first is cancel hotkey (as it was before) second is confirmation hotkey. (Like "No", "Yes")

Mapping of the binding is done by passing array of the callback indexes which are then mapped to [0] = cancel, [1] = confirm keys.

relates: #4411

Test Plan

Test msgboxes with different. (may search through gui folder for messageBox)

Actualy i didnt find message box with three buttons where it may would be necessary to set confirmation hotkey to last callback if last button is confirmation button.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

ffffffff created this revision.Nov 18 2017, 8:23 AM
ffffffff removed a reviewer: Polakrity.
ffffffff retitled this revision from Give Message Box Confirmation Callback Hotkey (Enter) to Give Message Box Confirmation Hotkey Binding.
ffffffff edited the summary of this revision. (Show Details)
ffffffff added a subscriber: Polakrity.
ffffffff edited the summary of this revision. (Show Details)Nov 18 2017, 8:36 AM
ffffffff edited the test plan for this revision. (Show Details)
ffffffff updated this revision to Diff 4253.Nov 18 2017, 8:38 AM
ffffffff updated this revision to Diff 4254.
ffffffff edited the summary of this revision. (Show Details)Nov 18 2017, 8:42 AM
ffffffff updated the Trac tickets for this revision.
elexis added a comment.Nov 18 2017, 10:20 AM

You are lucky that since few alphas, the order of Yes/No dialogs became consistent.

Even more lucky (the kind of luck we shouldn't rely on though) that all message boxes currently have only an Ok or a Yep/Nope buttons.
(The worst case currently is [translate("I will return"), translate("I resign")], which still can be mapped to cancel/confirm hotkeys)

The message box code supports 3 buttons (msgbox.xml) (and has no good reason to support 3 at most).
So there ought to be some logic to it making sure that the hotkeys are assigned to the correct buttons.

The hotkey names could be passed as an array like the other button data too.

ffffffff added a comment.EditedNov 18 2017, 10:27 AM
In D1046#41324, @elexis wrote:

You are lucky that since few alphas, the order of Yes/No dialogs became consistent.

Even more lucky (the kind of luck we shouldn't rely on though) that all message boxes currently have only an Ok or a Yep/Nope buttons.
(The worst case currently is [translate("I will return"), translate("I resign")], which still can be mapped to cancel/confirm hotkeys)

The message box code supports 3 buttons (msgbox.xml) (and has no good reason to support 3 at most).
So there ought to be some logic to it making sure that the hotkeys are assigned to the correct buttons.

The hotkey names could be passed as an array like the other button data too.

im so lucky

i think its No/Yes dialog

The hotkey names could be passed as an array like the other button data too.

yes but then we duplicate params with callbacks, maybe better idea mapping/binding them?

[translate("I will return"), translate("I resign")]

this maybe must be settable like something no hotkey..

ffffffff edited the summary of this revision. (Show Details)Nov 18 2017, 10:34 AM
ffffffff updated this revision to Diff 4258.Nov 18 2017, 11:08 AM

better impli

also covering case > [translate("I will return"), translate("I resign")] < where we can now hit esc closing dialog without action by passing null to bindings.

ffffffff updated this revision to Diff 4270.Nov 18 2017, 8:39 PM

contextdiff

ffffffff updated this revision to Diff 4273.Nov 19 2017, 8:12 AM
ffffffff updated this revision to Diff 4731.Dec 12 2017, 12:14 AM

better impli

ffffffff updated this revision to Diff 4732.Dec 12 2017, 12:40 AM
ffffffff updated this revision to Diff 4736.Dec 12 2017, 1:36 AM
ffffffff updated this revision to Diff 4844.Dec 21 2017, 6:35 PM

cleans

ffffffff updated this revision to Diff 4847.Dec 21 2017, 6:59 PM

elexis pls decide maybe better code

elexis added a comment.Dec 21 2017, 7:04 PM

Default values can be considered an anti-pattern.

If a value is an argument, then by definition it having one value is not universally true.

Not making the caller specify all values that the reader has to lookup the default value.
Without lookup, the caller also isn't aware that there is a value passed to begin with (whose value might be wrong).

The only use case of a default vlaue is when both the relative amount and the absolute amount of repetition of the default value is inacceptable (for instance if there 60 files with 20 repeated defaults and 10 calls not using the defaults).

But in this case we only have to add one line per message box and we have like 20 message boxes, right?

In this patch the responsability to specify hotkeys is passed to the ones that don't want to use them.
But it code is more straight-forward if it's positive, i.e. in order to do something, something should be stated.

(So I would type the one or two dozen lines and see how the diff looks. But I don't plan to review this so consider this comment random babbling).

binaries/data/mods/public/gui/session/menu.js
196

should contain the same number of items everywhere, i.e. twice undefined.

Perhaps it would be nice to eventually have this:

{

"Title": "far",
"Text": "bar",
 "Buttons:"
      [
       "Text": "button 1",
       "Hotkey": "gui.messagebox.ok"
        "Function": function() {
              ....
         },
     ],
     [
             ....button2
     ]

}

ffffffff added inline comments.Dec 21 2017, 7:18 PM
binaries/data/mods/public/gui/session/menu.js


196
jea true now its like the pattern in the previous parameters like:



messageBox(



		400, 200,
		translate("Are you sure you want to quit 0 A.D.?"),
		translate("Confirmation"),
		[translate("No"), translate("Yes")],
		[null, Engine.Exit],
		[],
		["cancel", "confirm"]



);



or just for 3 button example



messageBox(



		400, 200,
		translate("Are you sure you want to quit 0 A.D.?"),
		translate("Confirmation"),
		[translate("No"), translate("Maybe"), translate("Yes")],
		[null, null, Engine.Exit],
		[],
		["cancel", "", "confirm"]



);



but this would be also of course the default behaviour, when this parameter arary is not specified.

Confirmation hotkey is always binded to the latest button callback function and cancel always to the first button callback function in the order.



So as most messages boxes in the code uses this default pattern, its not needed to changes something to them in parameters, but this one occasion, where from context no bindings are wanted. Didn't find more occasions from context, but if there is more, it's not realy servere damage as they have only bindings then to the buttons.



But this implementation is realy adaptive, so as you can also make a messagebox like:



messageBox(



		400, 200,
		translate("Are you sure you want to quit 0 A.D.?"),
		translate("Confirmation"),
		[translate("Maybe"), translate("No"), translate("Yes")],
		[null, Engine.Exit, null],
		[],
		["", "cancel", ""]



);



where only cancel key is to the second (middle) button binded and "confirm" is not binded.



So this better so? hope:)
ffffffff added a comment.Dec 21 2017, 7:20 PM
sory u wrote much i need to reread later all, or irc if online. till then
ffffffff mentioned this in D813: Go back to previous gui page hotkey or exit ingame or exit game (or continue).Jan 2 2018, 8:06 PM
ffffffff updated the Trac tickets for this revision.Jan 18 2018, 6:55 AM
historic_bruno added a reviewer: historic_bruno.Jul 20 2019, 7:10 PM
Revision Contents
Changeset List
PathSize
binaries/
data/
mods/
public/
gui/
common/
13 lines
manual/
2 lines
msgbox/
31 lines
1 line
session/
4 lines
Diff 4847
View Options
binaries/data/mods/public/gui/common/functions_global_object.js
Loading...
View Options
binaries/data/mods/public/gui/manual/intro.txt
Loading...
View Options
binaries/data/mods/public/gui/msgbox/msgbox.js
Loading...
View Options
binaries/data/mods/public/gui/msgbox/msgbox.xml
Loading...
View Options
binaries/data/mods/public/gui/session/menu.js
Loading...
Wildfire Games · Phabricator