[PATCH] UI Enhancement: Right Click selection removal

[michael]

Currently, when multiple units are selected, the player can remove a unit (or group of like units) from the selection by ctrl+left clicking the icon portrait of that unit in the selection UI (center panel).

An enhancement here would be to also allow removal of units from the selection via simply right-clicking their icons in the center panel (no Ctrl key necessary). This does not replace the ctrl-left click, but adds a redundant method. Currently right-clicking icons in the center panel does nothing.

This is a GUI enhancement only/does not effect ctrl-clicking behavior in the game world.

[gerbilOFdoom]

Working on it... just give me some time.

[gerbilOFdoom]

Patch to add right-click functionality

[gerbilOFdoom]

First time coding C++, although I reused the code from left-clicks... pretty exciting to have gotten something done!

[Ganon]

Replying to gerbilOFdoom:

Why you discharge your solution? Are not good?

[gerbilOFdoom]

Oh, I accidentally closed it before it was reviewed and applied. In order for it to show up in the review query, it has to be new, so I reset it to new. When it's applied, it should be closed by whoever applies it.

[historic_bruno]

Replying to gerbilOFdoom:

First time coding C++, although I reused the code from left-clicks... pretty exciting to have gotten something done!

Thanks, sorry it's taken a while to review :) Your patch seems to work well, I do have a few suggestions:

• changePrimarySelectionGroup in input.js: it probably shouldn't have a parameter called rightPressed, because generally the function doesn't care about the mouse state, we only want to indicate what type of action should be performed. I think it would be more appropriate to call it something like deselectGroup. Then in unit_commands.js when the button callbacks are set up, you can still pass true for right button presses (because no matter whether the hotkey was used or not, it's a deselect action) and then pass the Engine.HotkeyIsPressed(...) for the other press event callback.
• Right-click-press now toggles checkboxes and radio buttons, which is unusual behavior for GUIs, and it could lead to undesired behavior in future GUI objects if they handle the GUIM_PRESSED event expecting only left clicks. So I think you should add e.g. GUIM_PRESSED_MOUSE_RIGHT enums to EGUIMessageType in GUIbase.h, because they should be handled separately than the current press and double press events. (At first I was thinking we would need to likewise refactor the left-click-press events, but it may be possible in the future for buttons to be pressed in response to keystrokes, which would have no logical difference from a standard left-click-press, so it seems best to leave them for now.)
• It's nothing introduced by your script, but the switch blocks in IGUIButtonBehavior.cpp are unconventional in that the break statements for each case are on the same line as the closing "}" bracket. This is a good chance to correct that by moving each of them onto a new line.
• I think the added comment in IGUIButtonBehavior.h should either go before the end of the line (inside the period) or just be removed, it looks strange hanging at the end.
• Usually we update the copyright date at the top of C++ source files when modifying them, though inconsistently.

[Kieran P]

Hey gerbilOFdoom. Any progress with implementing historic_bruno's suggestions? Keep to have this in Alpha 10 if we can.

[gerbilOFdoom]

Sorry, forgot to do anything about his suggestions. I'll hop on it soon, once I wrap up my current web development ticket!

[gerbilOFdoom]

Installing Visual Studio on my new OS.

[gerbilOFdoom]

I haven't fixed up the patch yet, not sure if I ever will. Removing myself from this so someone else can pick up where I left off.

[Jonathan Waller]

Thanks for picking up this patch. Generally it looks ok, a few comments.

guiName == "Selection" should be guiName == SELECTION

Engine.HotkeyIsPressed("selection.remove") should not be used, the hotkey is session.deselectgroup and this is already checked for in changePrimarySelectionGroup.

Also it would be better if you copy what has been done for research, so leave line 535 untouched and then add an if (guiName == SELECTION) {} block afterwards which sets onpress and onrightpress correctly for the selection case. This is so that you are not passing an unexpected boolean to all of the other callbacks.

[kingadami]

Implemented quantumstate's suggestions. Thanks for the input. Picking up this patch is introducing me to the code without being overwelmed (along with making me learn javascript).

[kingadami]

Updated patch file with quantumstate's suggestions. Thanks for the input.

[Jonathan Waller]

In 13040:

Added right click selection removal. Fixes #1028. Thanks to kingadami and GerbilOfDoom for the patch.

[Jonathan Waller]

This is pretty nice to use, thanks for the joint effort on the patch. And of course it also makes things easier for other right click actions we might want to add.

[elexis]

In 22969:

Defragment IGUIButtonBehavior Settings.

Moves the AddSetting sound calls from CButton and CCheckbox to IGUIButtonBehavior, since the latter is the only class to use them.
Moves ChooseColor from IGUIButtonBehavior to CButton, since that's the only class to use it and the only class registering the dependent Settings following rP14476 (refs rP22952 / D2314)

Initialize m_PressedRight in the constructor to prevent undefined behavior in possible future code following rP13040, refs #1028.
Remove unused soundPath variable following rP22756 / D2209.

Differential Revision: ​https://code.wildfiregames.com/D2318
Tested on: clang 8.0.1, Jenkins