KEMBAR78
BACKENDS: Add double-click time feature to support OS-configurable double-click intervals, implement it for Windows. by elasota · Pull Request #4460 · scummvm/scummvm · GitHub
Skip to content

Conversation

@elasota
Copy link
Contributor

@elasota elasota commented Nov 21, 2022

Definitely not sure the changes for the event recorder are in good places, open to suggestions.

This can be implemented on macOS as well with doubleClickInterval, see https://developer.apple.com/documentation/appkit/nsevent/1528384-doubleclickinterval

@sluicebox
Copy link
Member

sluicebox commented Nov 22, 2022

It's been a while since I've played with the EventRecorder, so apologies if I'm missing a detail on this.

It looks like this forces OSystem implementations to cache doubleclick time since EventRecorder is storing it in one place. I would expect OSystem::getDoubleClickTime() to query the platform directly and leave it up to engines if they wants to cache that or not.

If EventRecorder could treat getDoubleClickTime() as an event that it logs the responses of and responds with them during playback, I think you can remove _haveDoubleClickTime, processHaveDoubleClickTime(), and even the kFeatureDoubleClickTime.

If it weren't for EventRecorder, I think this would just be "getDoubleClickTime() queries the platform, returns 0 if not implemented." I don't know how feasible it is to make EventRecorder treat this query as an event, but I would prefer the complexity to be EventRecorder's burden and keep OSystem/backends simpler.

On the Windows side, I recall GetDoubleClickTime() being around in the Win9x days, so no compatibility issues there. =)

@sev-
Copy link
Member

sev- commented Dec 11, 2022

I am a bit lost here. What is the code supposed to do?

After studying it, I see the following:

  1. We read this setting from the OS (currently Win-only)
  2. We store this setting in event recordings
  3. We return it from the recording during playback instead of the current OS setting

What is the intended usage of this feature?

@elasota
Copy link
Contributor Author

elasota commented Dec 12, 2022

I am a bit lost here. What is the code supposed to do?

After studying it, I see the following:

1. We read this setting from the OS (currently Win-only)

2. We store this setting in event recordings

3. We return it from the recording during playback instead of the current OS setting

What is the intended usage of this feature?

On some OS (Windows and macOS for sure) users can set the double-click interval in the operating system's mouse settings. It's meant as an accessibility feature but it is also just good practice to ask the OS what the double-click time is to make it consistent with what the user is used to. Even if the user doesn't change it, the default isn't even the same on all OS (e.g. Windows is 500ms, macOS is 300ms).

So, this adds a way for engines to get the OS double-click time, and there are a few that could use it (I'm going to add it to mTropolis, where MTI can use it).

The time is also stored in event recordings so that when playing back recordings, the double-click time is treated as the same as it was on the recording machine to prevent any inconsistencies.

@criezy
Copy link
Member

criezy commented Dec 13, 2022

I think adding a way for engines that use double-click events to query the Operating System double-click time is a good idea. If the OSystem instance supports the feature, its value can be used, and otherwise the engine can use a default value.

The changes for the event recorder look a bit intrusive though. Since the event recorder adds its setting to the ConfMan transient domain (see EventRecorder::applyPlaybackSettings()) this can be used by the backend to provide the double-click time expected during playback.
See for example criezy@39ff8a3 for a way to implement this in the SDL backend (that is currently the only one that can be used with the event recorder - but if needed the indirection added there could be moved to the base class OSystem). Note that I have not tested that commit; this was only written quickly starting from your commit to show what I had in mind.

@lotharsm
Copy link
Member

I appreciate the idea of this change. Respecting the OS double-click speed would greatly help regarding accessibility in case the user has special requirements regarding the double-click speed.

@criezy
Copy link
Member

criezy commented Dec 17, 2022

Note that when we discussed this on discord, two ideas were raised:

  1. The one implemented here where we expose a double click time in OSystem
  2. Another one where we add double-click detection in backends and add it as a flag in Common::Event. For example SDL 2.0.2 added detection of double-click events in SDL_Event (via a clicks counter, 1 for single-click, 2 for double-click).

Both should hopefully mean that the OS setting is used to detect double-clicks, but one has the logic in the backends and the other in each engine that needs it. I can see some advantages to either approach. For example having double-clicks detection in the backend and identified in Common::Event means that a touchscreen backend (such as iOS or Android) could for example define a gesture for double clicks.

@elasota
Copy link
Contributor Author

elasota commented Dec 17, 2022

Note that when we discussed this on discord, two ideas were raised:

1. The one implemented here where we expose a double click time in `OSystem`

2. Another one where we add double-click detection in backends and add it as a flag in `Common::Event`. For example SDL 2.0.2 added detection of double-click events in `SDL_Event` (via a `clicks` counter, 1 for single-click, 2 for double-click).

Both should hopefully mean that the OS setting is used to detect double-clicks, but one has the logic in the backends and the other in each engine that needs it. I can see some advantages to either approach. For example having double-clicks detection in the backend and identified in Common::Event means that a touchscreen backend (such as iOS or Android) could for example define a gesture for double clicks.

I was going to try the latter route but it ran into numerous problems. The main issue is that if click count is expected to be in Event, then all backends need to support it. SDL1 doesn't produce click counts, and some platforms convert non-mouse inputs into clicks. Intercepting converted mouse events and updating their click count was turning out to be very complicated, especially since converted clicks and actual clicks can coexist on the same platform. At that point, there would need to be a way to determine double-click independent of the SDL event queue anyway, so it doesn't really even avoid the problem.

Just querying the time and having the engine figure it out from click events is much simpler.

Anyway, I'll make some updates to this to address feedback. I've been trying to figure out if such a thing exists on Linux and it sounds like it can be retrieved from the XSETTINGS "Net/DoubleClickTime" key, maybe?

https://www.freedesktop.org/wiki/Specifications/XSettingsRegistry/

@elasota elasota force-pushed the add-double-click-time-2 branch from 1e77821 to 6898b50 Compare December 22, 2022 21:47
@sev-
Copy link
Member

sev- commented Jan 9, 2023

Apparently, there is a misunderstanding, Particularly from people that say "nice change".

Let me elaborate a bit.

I see that this PR is merely exposing the value via the OSystem API. There is no demonstrated usage. This could be fine, as we are careful with changing the OSystem API, however, I'd like to understand what will happen to it later.

Doubleclicks.

The general approach to double-clicks is the following (apologies for explaining it here, just for the record):

  1. We receive a mouse button click.
  2. We remember which button it was and then launch the timer, with the shoot time set to the double-click interval
    Now, while the timer is still not shot.
  3. We receive button up, fine. We note this.
  4. We receive a second button-down event. If it is the SAME button, fine, we note this. it it is a DIFFERENT button, we then generate button up/button down for the previous button AND restart the timer for the possible new double click
  5. We receive a second botton-up event. Since the timer is still ongoing, we generate EVENT_?BUTTONDOUBLECLICK and kill the timer.
  6. Now, if the timer shoots and there are no other events, this is not a double click, we then generate EVENT_?BUTTONDOWN and up to two more events for #3 (up) and #4 (down).

Ugh, that is complicated.

Now the question is, where to detect those?

We have two options:

  1. Detect it in the user space, e.g. engines or GUI
  2. Detect it in the backend and/or EventManager

If we offload this burden of tracking the clicks on every engine, I personally find it suboptimal because you will have to reimplement and test that complicated logic every time.

I suggest implementing this functionality witin the event manager, so it is completely transparent to the engines. The engines will then receive EVENT_LBUTTONDOUBLECLICK or something like that.

Also, if some backend supports double clicks, we may offload this to them, because they will do all this grunt work for us and just generate events.

Of course, some engines have game scripts detecting double clicks. To address this, I would implement double clicks as a turnable feature. E.g. you tell the EventMan to turn on the double clicks, and it will, in turn, tell it to the baclend g_system->setFeatureState(kFeatureTrackDoubleClicks, true), and then you start receiving those new events.

Now, the question is. Would it be possible to implement this as part of this PR, or is this too much to ask? However, if we just merge it without implementing the usage, I am afraid we may end up with just a dead code.

@OMGPizzaGuy
Copy link
Member

OMGPizzaGuy commented Jan 10, 2023

A few more thoughts on this:

  1. If we generate a EVENT_?BUTTONDOUBLECLICK, we would also likely need a EVENT_?BUTTONCLICK event - handling up or down events as click would end up processing as two click even if double was intended and could also alter behavior where dragging was intended. Click would need to generate when the timeout for double expires.
  2. A button click does not need to invalidate the timeout for a different button. The use case for that likely rare, but generating a single click event for a button shouldn't cancel due to this.
  3. If implemented this way, we would need to agree what the default fallback timeout would be and/or have that be able to be set. Multiple engines already have different values for this.

I'm an odd case, but I would end up using this as-is due to how complex mouse handling is in Ultima 8. It even has a case where if left and right mouse are clicked before the timeout, it produces a jump.

@elasota
Copy link
Contributor Author

elasota commented Jan 11, 2023

I see that this PR is merely exposing the value via the OSystem API. There is no demonstrated usage. This could be fine, as we are careful with changing the OSystem API, however, I'd like to understand what will happen to it later.

Muppet Treasure Island has some things that depend on double-clicks, right now it's using a hard-coded 500ms time, and I'd like to get it from the OS instead, so that's the first usage, but there are some other engines with double-click detection too that could also use it.

Now the question is, where to detect those?

We have two options:

1. Detect it in the user space, e.g. engines or GUI

2. Detect it in the backend and/or EventManager

If we offload this burden of tracking the clicks on every engine, I personally find it suboptimal because you will have to reimplement and test that complicated logic every time.

I think we need to support both scenarios because there are possibly quirks in how engines need handle it. e.g. what happens if the user clicks more than twice in the double-click window? In mTropolis, there is a "clickcount" var that increases until the timer from the first click expires, so it can go higher than 2. But, it's possible that a game may instead want to reset the timer as soon as a double-click is detected.

I suggest implementing this functionality witin the event manager, so it is completely transparent to the engines. The engines will then receive EVENT_LBUTTONDOUBLECLICK or something like that.

I think the SDL approach of putting a click count in the mouse event is the best option.

Now, the question is. Would it be possible to implement this as part of this PR, or is this too much to ask? However, if we just merge it without implementing the usage, I am afraid we may end up with just a dead code.

I want to add it to the mTropolis engine already, so it won't be dead code.

I can add some EventManager handling to this if you want, but I think it'd be better to do it separately because I already tried doing that and it more complicated than expected because of remapped events (e.g. gamepad inputs converted to mouse events).

@sev-
Copy link
Member

sev- commented Feb 7, 2023

@elasota Do you have in mind eventually implementing the EventManager change? Because if we just merge it without the established future plans, I'm afraid it could sit just as an unused code complication that is easy to break without a proper test case.

Speaking of the testcase, once implemented, this functionality also must be covered in the testbed engine.

@sev-
Copy link
Member

sev- commented Mar 24, 2023

@elasota ping?

@elasota
Copy link
Contributor Author

elasota commented Mar 26, 2023

I can add it to testbed some time this week. I guess since it doesn't look like there are actually any other event managers than DefaultEventManager that it should be feasible to do multi-click detection there, but as mentioned previously, it needs some further investigation re: how to handle non-mouse-click events converted into mouse click events.

@elasota elasota force-pushed the add-double-click-time-2 branch from 22207f4 to 98f2998 Compare March 26, 2023 17:27
@elasota elasota force-pushed the add-double-click-time-2 branch from 98f2998 to 5fb11cd Compare March 26, 2023 17:29
@sev-
Copy link
Member

sev- commented May 14, 2023

Thank you!

@sev- sev- merged commit 22a21e9 into scummvm:master May 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants