FIX: Send Touch actions to the proper window #2485

Merged
merged 1 commit into from Apr 2, 2013

Projects

None yet

4 participants

@koying
Member
koying commented Mar 22, 2013

This allows:

...
  <FullScreenVideo>
    <touch>
      <swipeleft>StepBack</swipeleft>
      <swiperight>StepForward</swiperight>
      <swipeup>BigStepForward</swipeup>
      <swipedown>BigStepBack</swipedown>
    </touch>
  </FullScreenVideo>
...
@Montellese
Member

Cool, I actually tried to get that to work once but didn't succeed but I didn't have the time to look into it more. That was actually one of the goals of my touch refactor so that users can add these kind of gestures. I'll give it a try and report back.

@Montellese
Member

OK finally managed to fix my android build env with the new depends and gave this a spin. I didn't see any regressions so should be fine.

[ot]
I tried to map swipe left/right in the music visualization window to SkipNext/SkipPrevious but it didn't work because CGUIMusicVisualization doesn't handle SkipPrevious/SkipNext. Didn't try it with fullscreen video though.
[/ot]

@koying koying merged commit 4ee5de2 into xbmc:master Apr 2, 2013
@koying koying deleted the koying:fixtouchactions branch Apr 2, 2013
@jmarshallnz

What's the reasoning here? AFAICT the usual CApplication::OnAction() route will send the action to the appropriate windowId anyway, right?

See #3978

Member

I guess my state-of-mind was that there is no point using WINDOW_INVALID when we already know the windowid ?

Member

Right, but calling CApplication::OnAction() will get to the correct window through calling CGUIWindowManager::OnAction() which will (eventually) wind through to the appropriate window. Assuming some dialog or other block doesn't get in the way...

Though if it did work, I don't think the changes in this PR would have been needed at all, so something must be getting in the way.

It would be useful to know where things are going wrong, so we can push the actions through the correct chain that other actions go through.

@jmarshallnz

Do some windows "handle" an action they're not supposed to? Why do you need certain actions to go via CApplication::OnAction() while others go directly to the current window, given that CApplication::OnAction() will call into the current window?

Member

And here I reset the windowid because an action for this specific windowid could not be found ?

@koying
Member
koying commented on b891c24 Jan 6, 2014

Arf... Old stuff.
See #2485 for backstory.

@Memphiz
Member
Memphiz commented Jan 6, 2014

Well if we found an action for a windowid and call onaction of this window but it doesn't handle that action because it is a global action (like volumeup/down) we have to ensure to pass it through the CApplication::OnAction chain if SpecificWindow::OnAction failed handling it. As a fallback so to say.

I have no insight why this doesn't work in the first place.

@jmarshallnz
Member

Yup, problem is that CApplication::OnAction() is supposed to be the entry point of all actions - it calls into CGUIWindowManager::OnAction() to pass it to the window should it want it.

ATM with the code in your PR it calls the window's OnAction() twice (not that that's really a problem, but it'd be nice if it was done consistently with all others).

i.e. the process should be:

  1. Resolve the action using the current window ID (as per CApplication::OnKey)
  2. Call CApplication::OnKey(). This handles the global actions as well as passing the action through the windowmanager to the current window.

If this isn't working (which seems to be the reason why #2485 was merged) then we need to figure out why as it might indicate an issue with other action handling as well.

i.e. you should be able to just switch the windowID -> WINDOW_INVALID in the CApplicationMessenger::SendAction() call so that it goes directly via CApplication::OnAction() and everything should work (tm).

@koying
Member
koying commented Jan 7, 2014

Ok, I'll revise that.
Le 7 janv. 2014 00:08, "jmarshallnz" notifications@github.com a écrit :

Yup, problem is that CApplication::OnAction() is supposed to be the entry
point of all actions - it calls into CGUIWindowManager::OnAction() to pass
it to the window should it want it.

ATM with the code in your PR it calls the window's OnAction() twice (not
that that's really a problem, but it'd be nice if it was done consistently
with all others).

i.e. the process should be:

  1. Resolve the action using the current window ID (as per
    CApplication::OnKey)
  2. Call CApplication::OnKey(). This handles the global actions as well as
    passing the action through the windowmanager to the current window.

If this isn't working (which seems to be the reason why #2485https://github.com/xbmc/xbmc/pull/2485was merged) then we need to figure out why as it might indicate an issue
with other action handling as well.

i.e. you should be able to just switch the windowID -> WINDOW_INVALID in
the CApplicationMessenger::SendAction() call so that it goes directly via
CApplication::OnAction() and everything should work (tm).


Reply to this email directly or view it on GitHubhttps://github.com/xbmc/xbmc/pull/2485#issuecomment-31697452
.

@koying
Member
koying commented Jan 7, 2014

The problem is that the gesture triggers the OSD and that there doesn't seem to be a "fallback" to pass the action to WINDOW_FULLSCREEN_VIDEO, where the ACTION_STEP_FORWARD et al actions are handled.

AFAICT, the triggering of the OSD is accidental, but I'm not sure and it would be another issue anyway, as the functionality is actually fine, imo.

A possible solution would be to duplicate the handling of ACTION_STEP_FORWARD et al (and maybe others) in CGUIDialogVideoOSD::OnAction() ?

BTW, apparently CApplication::OnAction() does not systematically fallthrough to CGUIWindowManager::OnAction() if it doesn't handle the action. Is that right? Or is it Application.cpp:2542 ?

@koying
Member
koying commented Jan 7, 2014

Reverting +

`--- a/xbmc/guilib/GUIWindowManager.cpp
+++ b/xbmc/guilib/GUIWindowManager.cpp
@@ -459,7 +459,8 @@ bool CGUIWindowManager::OnAction(const CAction &action) const
     { // we have the topmost modal dialog
       if (!dialog->IsAnimating(ANIM_TYPE_WINDOW_CLOSE))
       {
-        bool fallThrough = (dialog->GetID() == WINDOW_DIALOG_FULLSCREEN_INFO);
+        bool fallThrough = (dialog->GetID() == WINDOW_DIALOG_FULLSCREEN_INFO  ||
+                            dialog->GetID() == WINDOW_DIALOG_VIDEO_OSD);
         if (dialog->OnAction(action))
           return true;
         // dialog didn't want the action - we'd normally return false

seems to do the trick.
Is that ok?

@jmarshallnz
Member

The trick is to figure out why the gesture triggers the OSD, and whether it should do so. I don't know if it should or not - there may be arguments both for and against. I suspect this will be due to a mouse move event or similar during the gesture begin?

Nonetheless, there is some merit in handling a fallthrough from the OSD, so I'm not particularly opposed to doing that. You'll notice that in the VideoOSD section of the keymap there are duplicated a bunch of actions for this reason.

@koying
Member
koying commented Jan 7, 2014

Indeed, the OSD is probably accidental, but I would nonetheless be in favor of implementing it that way, so whether we should fall through the OSD or not still stands.
I guess the question is whether that can have adverse side-effects?

@jmarshallnz
Member

I don't know, though I do notice that, contrary to my memory, the keymap for VideoOSD doesn't contain any duplicate actions, so is likely intended for those actions to be fullscreen window only. With that said, if the above was adequately tested I wouldn't necessarily be opposed.

Regarding the popping up of the OSD, you may wish to play with this line in CGUIWindowFullScreen::OnMouseEvent:

  if (event.m_id != ACTION_MOUSE_MOVE || event.m_offsetX || event.m_offsetY)

I believe you want to eliminate ACTION_GESTURE_BEGIN and possibly also ACTION_GESTURE_END from there by popping them in the if statement above this one?

@koying
Member
koying commented Jan 8, 2014

You're probably indirectly right.
My bet on the root cause is on CGenericTouchActionHandler::focusControl. That'd lead to an actual mouse move event with non-zero offsets, thus triggering the OSD.
CGenericTouchActionHandler::focusControl is called as soon as a finger touches the screen.

There must be a better way to force focus than sending a mouse move event, is there?

OTOH, might be that if (event.m_id != ACTION_MOUSE_MOVE || event.m_offsetX || event.m_offsetY) is wrong, actually, in that, from how it is written, I'm not sure the OSD is intended to be popped up on a mouse move at all.

@Memphiz
Member
Memphiz commented Jan 8, 2014

OSD popup on mouse move is intended afaik.

@koying
Member
koying commented Jan 8, 2014

If so, I guess the OSD popping up when touching the screen is consistent.

Strange "if", though, if intended ;)

@Montellese
Member

IMO OSD popping up makes sense when tapping on the screen but not when doing a (swipe) gesture. With a mouse this is probably different.

@koying
Member
koying commented Jan 8, 2014

We should strive for consistency.
Doing an ACTION_STEP_FORWARD with, e.g., a remote also pops up the OSD, so it should do likewise on touch, one way or another (I suppose the actual action triggers the osd).
And once the OSD is on, we should still be able to do ACTION_STEP_FORWARD with a swipe, so it's back to wether we pass through from OSD to FULLSCREEN.

Note that only popping up the OSD on tap should take care of leaving the possibility to assign an action to it. I can imagine some wanting to assign play/pause to it, for example...

@Memphiz
Member
Memphiz commented Jan 8, 2014

Passthrough from OSD to Fullscreen seems valid then imo - but i am far from the man with overwview here - lets wait for another comment from jm :)

Thx for tackeling this btw @koying && @Montellese

@jmarshallnz
Member

As highlighted there's 2 things going on:

  1. All touch's act as a mouse move initially. The mouse move as per the line quoted above (if the action is not previously handled, and it's not a mouse move or it's a mouse move and the mouse has moved) is designed for the mouse. As @Montellese suggests, this makes sense for a mouse and for a single tap (or some other unhandled touch event?) but doesn't make sense for a swipe action that does something.
  2. When the Video OSD is up, currently a bunch of actions don't work, as they're not explicitly mapped in VideoOSD (and further, the code to handle them isn't there either). On the remote and keyboard this is likely not too much of a problem, as most of the common ones (play/pause/stop etc.) are in the <global> section of the keymap anyway, and are handled in CApplication::OnAction(). You'll find you won't be able to do ACTION_STEP_FORWARD while the VideoOSD is up, regardless of device.

So there's 2 things to fix. The second could be done by the proposed changes. I don't see any issue with doing it like this, but I would note that it would only affect the remote/keyboard/mouse input (i.e. everything except touch) if the user actually maps said actions in the <VideoOSD> section of their keymap. It affects touch directly due to issue number 1 above.

For the first we need to figure out which bit is popping up the VideoOSD and then see whether or not that can be altered without breaking other stuff :)

@koying
Member
koying commented Jan 8, 2014

For 1), and from code comment in CGenericTouchActionHandler::focusControl (´´´// Send a mouse motion event for getting the current guiitem selected```), the mouse move is only meant for focus, so is there another way to attain the same result?

For 2), you're right. I've mistaken the OSD with the progressbar GUI. (Which window/dialog id is that? WINDOW_DIALOG_FULLSCREEN_INFO?)

So if 1) solved, i.e. focusing without mouse event, 2) isn't an issue anymore.

@koying
Member
koying commented Jan 9, 2014

See #3996

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment