Fix clicks in Atlas
ClosedPublic
Actions

Authored by causative on May 7 2017, 11:18 PM.

Details

Reviewers

elexis
Stan

Commits

rP19589: Fix atlas clicking following rP19444 / D326 by adding the missing clicks…

Trac Tickets

#4557

Summary

Without this patch in svn, place 3 CCs in Atlas for Player 1, click Play, and try to click on your CC. This fails (selecting all 3) because Atlas does not pass up the clicks attribute of the event to input.js, needed since D326. This patch passes up the clicks attribute from Atlas.

However, note that Atlas relies on wx for mouse events, not SDL, and wx does not actually track clicks. see http://docs.wxwidgets.org/trunk/classwx_mouse_event.html#adc4d39b626fcc62e93d9356a32003273 . Therefore, the workaround is to pass up 1 for single click events, and 2 for double click events.

The caveat is, this means triple clicks do not work in Atlas. Can a triple click be detected from two consecutive double clicks? No, because if you triple click in wx, the double click is registered only on the second click, not on the third. The only way to detect triple clicks in Atlas would be to go back to the old behavior of using our own timer and motion threshold in javascript, which elexis agreed is not preferred.

Another note: wx only registers the double click on mousedown, not on both mousedown and mouseup like SDL does. So it is now necessary to store the number of clicks on mousedown in input.js.

Test Plan

Open Atlas. Place some units for Player 1, and hit Play. Verify that you can select your units by single clicking or double clicking on them.

Close Atlas and open a single player scenario (Rome Sandbox is good). Verify that single clicking, double clicking, and triple clicking still work as expected.

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

causative created this revision.May 7 2017, 11:18 PM

Maybe this http://wxwidgets.10942.n7.nabble.com/Triple-Clicks-td60655.html could help ? I know there is no native support for it.

Well, as that thread suggested, I could pass up wxSYS_DCLICK_MSEC (max time between double clicks), wxSYS_DCLICK_Y (max distance between double clicks), and wxSYS_DCLICK_X to input.js and have that detect double and triple clicks that way (... and either figure out how to pass the same information from SDL, or else have two different ways of detecting triple clicks in input.js). However, this is not necessarily portable, as wxSYS_DCLICK_MSEC remains undocumented and http://trac.wxwidgets.org/ticket/8736 , which added it, indicates it was added only for GTK and MSW. I think the impact of not having triple click in Atlas is very low.

In D452#18291, @causative wrote:

Well, as that thread suggested, I could pass up wxSYS_DCLICK_MSEC (max time between double clicks), wxSYS_DCLICK_Y (max distance between double clicks), and wxSYS_DCLICK_X to input.js and have that detect double and triple clicks that way (... and either figure out how to pass the same information from SDL, or else have two different ways of detecting triple clicks in input.js).

There is GetClickCount(), but that is only implemented on OSX. Which is one of the reasons why that one metric isn't really implemented there...

Sounds like one would have to rely on platform specific behaviour to maybe get nice results (we could try and create an upstream bug report about those being unimplemented on a few platforms), or decide that triple clicks not working in atlas isn't that much of an issue. (One could also do both of those.)

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/1074/ for more details.

Harbormaster completed remote builds in B1543: Diff 1741.May 8 2017, 2:42 PM

I think the best solution is to just not have triple clicks in Atlas, and make a bug report for the future in case wx ever implements GetClickCount on all platforms. It's not worth weird platform specific hacks, considering that triple clicks are barely ever used in a normal game, let alone Atlas.

I guess it is also fair considering hotkeys are a bit broken in atlas.

It would be preferable to keep that workaround (i.e. recalling the nr of clicks in the atlas code (i.e. ScenarioEditor.cpp) and not make it worse for input.js.

binaries/data/mods/public/gui/session/input.js
45 ↗	(On Diff #1741)	If you're already changing the style of this comment, might as well go for JSdoc for consistency with the rest of the JS code
48 ↗	(On Diff #1741)	Since it's not obvious from reading this file that this is a wxWidgets workaround, there should be a comment about this here. The value is correct, because there is always a mousedown event before a mouseup event and since the variable is only read after one of these two events.
49 ↗	(On Diff #1741)	Not the best naming choice, since this variable will be accessible in the entire gui context, so some other file might want to use that name too and override this one unintentionally. Also we use `g_Foo` for globals
source/tools/atlas/AtlasUI/ScenarioEditor/ScenarioEditor.cpp
275 ↗	(On Diff #1741)	Ok, besides being used already, `ButtonDClick` should be supported on every implementation: http://docs.wxwidgets.org/3.0/classwx_mouse_event.html
277 ↗	(On Diff #1741)	Indeed `evt.ButtonDClick()` always returns `false` here. Stupid wxWidgets.
source/tools/atlas/GameInterface/Handlers/MiscHandlers.cpp
170 ↗	(On Diff #1741)	2017

This revision now requires changes to proceed.May 15 2017, 2:42 PM

Track the number of clicks for a mouseup in C++ instead of js, eliminating all js changes in this patch.

That's it, thanks! It's a bit weird to have the third click count as a single click (I'd expect a triple click to behave like a double click), but we can blame those that didn't implement it with wxWdigets.