Page MenuHomeWildfire Games

Add GUI events for middle mouse click
ClosedPublic

Authored by Imarok on Jan 18 2019, 5:21 PM.

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Owners Package(Owns No Changed Paths)
Commits
rP23505: Add GUI events for middle mouse click
Summary

Add gui events for middle mouse click

Test Plan

just test it ;)

Event Timeline

Imarok created this revision.Jan 18 2019, 5:21 PM
Vulcan added a subscriber: Vulcan.Jan 18 2019, 5:26 PM

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

Link to build: https://jenkins.wildfiregames.com/job/differential/980/

vladislavbelov added inline comments.
source/gui/CGUI.cpp
185

Too many of the same code. Maybe remove the duplication here and below?

for what u need it

Silier added a subscriber: Silier.Jan 19 2019, 8:30 AM

for what u need it

for map ping

elexis added a subscriber: elexis.Jan 19 2019, 12:31 PM

Actually, does this have to be hardlinked to a mouse tripleclick? Why not use hotkeys so people can put it on arbitrary input? There will certainly be people with only 2 buttons (Wasn't that about every mac user?) Which doesn't mean that the GUI hardcoding support can't be completed. I guess the use case for these hardcoded GUI events is to keep some things unchangeable, like the leftclick on a button, the leftclick on a list item. These are conventions that are the same throughout platforms, websites, decades. But there are not many of such things that are so common.

It's already supported, see default.cfg:

pan = MouseMiddle ; Enable scrolling by moving mouse

(I posted the same comment already this week on some other phab page, but not sure where)

source/gui/CGUI.cpp
238

When I saw this in D1751 I also thought to remove this triplicated hunk, but there are too many constants to make this immediately worthwhile.
One could pull the && pNearest condition out of the switch, and maybe the doubleclick timing, but it won't become much better probably.
Maybe one actually could do refactor the mousebutton handling with some function and pass the constant.
I guess mouses are not limited to 3 buttons as they are not limited to 2 buttons, so one could solve it for as much as SDL supports then.

vladislavbelov added inline comments.Feb 16 2019, 6:27 PM
source/gui/CGUI.cpp
238

I guess mouses are not limited to 3 buttons as they are not limited to 2 buttons, so one could solve it for as much as SDL supports then.

It won't be an easy task. We make solve it with keyboard settings to detect any pressed key.

Imarok added a comment.Mar 3 2019, 5:30 PM
In D1752#70523, @elexis wrote:

Actually, does this have to be hardlinked to a mouse tripleclick? Why not use hotkeys so people can put it on arbitrary input? There will certainly be people with only 2 buttons (Wasn't that about every mac user?) Which doesn't mean that the GUI hardcoding support can't be completed. I guess the use case for these hardcoded GUI events is to keep some things unchangeable, like the leftclick on a button, the leftclick on a list item. These are conventions that are the same throughout platforms, websites, decades. But there are not many of such things that are so common.

GUI events are for actions executed onto a specific GUI element. Hotkeys are global events or apply to the focused GUI element.
Polling or only reacting if minimap is focused is not suitable, so we have to use GUI events. For those without a middle mouse button, we could implement a gui button. (Analogous to the unit commands which can be triggered either by key or by buttons)

It's already supported, see default.cfg:

pan = MouseMiddle ; Enable scrolling by moving mouse

There it is used as global Hotkey and checked via polling.

elexis added a comment.Mar 3 2019, 5:57 PM

I wasn't proposing a hotkey that triggers the "middle button action", but a hotkey that triggers the specific minimap action you want for the minimap patch.
The question is whether it's a good idea to assign three actions to buttons in general and thus commit the patch, or wether whether alternate patterns, such as one action per button are more intuitive.
Perhaps there are some fringe examples, I guess the minimap button already is one of the fringiest. So perhaps it's okay to add that support in the engine but advise people to better use a separate button instead for regular buttons, unless they have a really strong use case where it's also a bad idea to use global hotkeys.

Considering the Minimap use case, it's also strange that this event doesn't send the coordinates to the minimap. I guess it works that way: user clicks, code is triggered, code gets the coordinates from the minimap object? I have a revision proposal somewhere to allow sending a JS::Object to the GUI event.
It should really be interchangeable for people without the third mousebutton. Don't OSX hipsters have only one mouse button?

Isn't the ScriptEvent("worldclick", coords); + IsHotkeyPressed("sendping_minimap") enabling that?

Imarok added a comment.Mar 3 2019, 6:27 PM
In D1752#72563, @elexis wrote:

I wasn't proposing a hotkey that triggers the "middle button action", but a hotkey that triggers the specific minimap action you want for the minimap patch.

I know

The question is whether it's a good idea to assign three actions to buttons in general and thus commit the patch, or wether whether alternate patterns, such as one action per button are more intuitive.
Perhaps there are some fringe examples, I guess the minimap button already is one of the fringiest. So perhaps it's okay to add that support in the engine but advise people to better use a separate button instead for regular buttons, unless they have a really strong use case where it's also a bad idea to use global hotkeys.

Considering the Minimap use case, it's also strange that this event doesn't send the coordinates to the minimap. I guess it works that way: user clicks, code is triggered, code gets the coordinates from the minimap object? I have a revision proposal somewhere to allow sending a JS::Object to the GUI event.
It should really be interchangeable for people without the third mousebutton. Don't OSX hipsters have only one mouse button?

Isn't the ScriptEvent("worldclick", coords); + IsHotkeyPressed("sendping_minimap") enabling that?

But when and where do you want to check if the hotkey is pressed?

elexis added a comment.Mar 4 2019, 4:44 PM

I guess worldclick doesn't work since it only reacts after left-click; unless we make that hotkey a modifier (minimap click + hotkey(default shift) for instance).

So the question is how minimap ping should be implemented for the people who don't have a third mouse button.
Isn't the usual way to add a button near the minimap, that if clicked changes the input.js state, and if that state is set, both the minimap "worldclick" events and the clicks on the actual 3D world would equally trigger the 'minimap ping'?
Then said input.js state could be triggered with an arbitrary hotkey.
(Could do the same with patroling.)

Also wondering why this patch changes the button behavior instead of adding the third button case to CMiniMap::HandleMessage.

Perhaps one could make that function parse the middleclick, trigger WorldClick, and have the JS event test for IsHotkeyPressed() to distinguish whether it should move the unit, patrol the unit, or trigger a mapping.
Then default.cfg could be set to some keyboard combination, middleclick. (Possibly switching the JS input state for the duration the hotkey is pressed if going that route).

So needs some exploration, but hardcoding it to the middle mousebutton seems possibly problematic for users without that button for the minimap case.

wraitii added reviewers: Restricted Owners Package, Restricted Owners Package.Apr 22 2019, 9:38 AM
wraitii added a subscriber: wraitii.
In D1752#70523, @elexis wrote:

There will certainly be people with only 2 buttons (Wasn't that about every mac user?)

Nowadays mac users have no buttons at all and touch input, in fact ;)

In D1752#72574, @elexis wrote:

I guess worldclick doesn't work since it only reacts after left-click; unless we make that hotkey a modifier (minimap click + hotkey(default shift) for instance).

Yes.

So the question is how minimap ping should be implemented for the people who don't have a third mouse button.
Isn't the usual way to add a button near the minimap, that if clicked changes the input.js state, and if that state is set, both the minimap "worldclick" events and the clicks on the actual 3D world would equally trigger the 'minimap ping'?

In D1752#72553, @Imarok wrote:

For those without a middle mouse button, we could implement a gui button. (Analogous to the unit commands which can be triggered either by key or by buttons)

Then said input.js state could be triggered with an arbitrary hotkey.

Could, but we are running low on hotkeys ;) (So I'd let it unassigned)

Also wondering why this patch changes the button behavior instead of adding the third button case to CMiniMap::HandleMessage.

Because there is currently no middle button message sent that could be recieved by CMiniMap::HandleMessage.

Ok, seems like I shouldn't try using the private svn for phab ^^

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

Linter detected issues:
Executing section Source...

source/gui/CGUI.cpp
|1693| »   m_ScrollBarStyles[name]·=·scrollbar;
|    | [MAJOR] CPPCheckBear (uninitStructMember):
|    | Uninitialized struct member: scrollbar.m_ScrollWheel

source/gui/CGUI.cpp
|1693| »   m_ScrollBarStyles[name]·=·scrollbar;
|    | [MAJOR] CPPCheckBear (uninitStructMember):
|    | Uninitialized struct member: scrollbar.m_ScrollSpeed

source/gui/CGUI.cpp
|1693| »   m_ScrollBarStyles[name]·=·scrollbar;
|    | [MAJOR] CPPCheckBear (uninitStructMember):
|    | Uninitialized struct member: scrollbar.m_ScrollButtons
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1288/display/redirect

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

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

Imarok updated this revision to Diff 11386.Feb 17 2020, 7:06 PM

Allow reuse of the strings in inherited and friend classes.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 17 2020, 7:17 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.

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

Linter detected issues:
Executing section Source...

source/gui/CGUI.h
|   1| /*·Copyright·(C)·2019·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2019"

source/gui/CGUI.h
|  51| class·CGUI
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCGUI{' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...

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