Skip to content

Revert "FIX: Send Touch actions to the proper window" and properly solve #3996

Merged
merged 2 commits into from Feb 9, 2014

4 participants

@koying
Team Kodi member
koying commented Jan 9, 2014

No description provided.

@Memphiz
Team Kodi member
Memphiz commented Jan 9, 2014

Nice will test it on ios in a min...

@koying
Team Kodi member
koying commented Jan 9, 2014

Thanks.
I didn't seem to find an equivalent need for XBMC_SETFOCUS on ios, but I might have missed it.

@Memphiz
Team Kodi member
Memphiz commented Jan 9, 2014

ios uses CGenericTouchActionHandler::focusControl aswell - no worries :)

@Memphiz
Team Kodi member
Memphiz commented Jan 9, 2014

Feels nice already. One issue:

I can bring up the osd by single tap now - before your change i was able to 2finger swipe left to get rid of it - this doesn't work anymore now. (2finger swipe left is mapped to right click)...

@koying
Team Kodi member
koying commented Jan 9, 2014

Okido, will check that out.
Is the 2finger swipe mapping [global], from a keymap?

@Memphiz
Team Kodi member
Memphiz commented Jan 9, 2014

yep it is in global < tap pointers="2" > RightClick < /tap >

@Memphiz Memphiz closed this Jan 9, 2014
@Memphiz Memphiz reopened this Jan 9, 2014
@Memphiz
Team Kodi member
Memphiz commented Jan 9, 2014

ahh sorry ... i mean it is mapped to "Back" ..

in global
< swipe direction="left" pointers="2" >Back< /swipe >

the other one was 2 finger tap which is mapped to mouse rightclick and that works for hiding osd!

@koying
Team Kodi member
koying commented Jan 9, 2014

So 2 fingers tap for right-click is ok, but 2 fingers left swipe for back is not, right?

@Memphiz
Team Kodi member
Memphiz commented Jan 9, 2014

yep that is correct.

Though this is only for videoosd ... 2 fingers left swipe for back works in the menus as usual...

@Memphiz
Team Kodi member
Memphiz commented Jan 9, 2014

basically before this change the "back" action also closed the osd - it doesn't do that anymore ... could also be a feature imo ;)

@koying
Team Kodi member
koying commented Jan 9, 2014

Hehe... No, Back is meant to close the OSD. Works like that on my remote anyway...

@koying
Team Kodi member
koying commented Jan 9, 2014

@Memphiz Could you retry, please.
JM might have pointed out the issue, already ;)

@Memphiz
Team Kodi member
Memphiz commented Jan 9, 2014

Nope ... there is a mapping in fullscreenvideo

< swipe direction="left" pointers="2" >SmallStepBack < /swipe >

This one works (e.x. when the osd is not visible the 2 finger swipe left will do the smallstepback) ...

here is my complete touchscreen.xml for testing:

http://pastebin.com/vpwbXigj

If i remove that smallstepback mapping it is working and the 2 finger swipe left closes the osd. So basically it seems that the mapping in fullscreenvideo somehow prevents the global mapping in osd window to happen.

@jmarshallnz
Team Kodi member

This is because the mapping is done in the handler for touch using GetActiveWindow() as the windowID. This is the active window - dialogs go on top of this.

Instead, it should ideally be using the mapping defined in CApplication::OnKey, though I suspect you can get away with a simpler implementation by ignoring some of the cases there.

https://github.com/xbmc/xbmc/blob/master/xbmc/Application.cpp#L2319

You want the hunk from line 2319-2358 that is determining the window ID. Probably if you just have the very start of that (i.e. HasModalDialog bit) it'll be good enough for touch?

@Memphiz
Team Kodi member
Memphiz commented Jan 9, 2014

http://pastebin.com/ifsKCdVQ <- that fixes it ...

@jmarshallnz
Team Kodi member

Yup, that'll fix the OSD stuff. However, I think you'll find it'll break actions while fullscreen info is up (this might not be possible on touch, so may not be an issue). That's what all that other code in OnKey() is taking care of. Could REALLY do with a solid refactoring :)

@Memphiz
Team Kodi member
Memphiz commented Jan 9, 2014

yep you are right ... now the actions mapped in fullscreenvideo are not working anymore when the osd is up but act like the one from the global section ;)

@jmarshallnz
Team Kodi member

Yes, that was point 2 back on the original PR - it happens on the remote and keyboard as well, so in that respect is by design.

There's basically 2 things at work:
1. Which window context to use to map from input -> action.
2. Which window to send the action to.

By changing the iWindow you change number 1 - i.e. when the video OSD is up, you don't read the actions in the fullscreen section of keymap.

By changing number 2, the action is sent to the dialog, rather than the window.

There's fallbacks in place for one specific window in the code: The fullscreen info dialog (in Confluence, it looks like the OSD without the play/pause buttons - bring it up via I on keyboard or INFO on remote).

The code in OnKey handles getting the correct context for number 1 above. The code in GUIWindowManager::OnAction handles re-routing the action to the fullscreen window if the dialog doesn't handle it.

@koying
Team Kodi member
koying commented Jan 10, 2014

I think I now apply to gestures the same window logic as OnKey.
Hard to test most use cases on a plain tablet, though.

@Memphiz
Team Kodi member
Memphiz commented Jan 10, 2014

Looks good from the code side to me - i will give this a shot after work :)

@jmarshallnz
Team Kodi member

Yup, code looks fine.

@Memphiz
Team Kodi member
Memphiz commented Jan 10, 2014

Something is still strange. If OSD is up i still don't get my swipe up mapping ing from fullscreenvideo ...

Still using this touchscreen map

http://pastebin.com/vpwbXigj

@jmarshallnz
Team Kodi member

That's exactly as designed. The VideoOSD receives the action, and in addition is used for the mapping. Exactly the same as DPAD on remote when the VideoOSD is up.

@Memphiz
Team Kodi member
Memphiz commented Jan 10, 2014

But there is no mapping in the videoosd for swipeup ... something i get wrong i think?

@jmarshallnz
Team Kodi member

Exactly, so nothing is done, right?

In addition, even if there was an action there, it would need to be handled by the video OSD.

@Memphiz
Team Kodi member
Memphiz commented Jan 10, 2014

correct :)

@Memphiz
Team Kodi member
Memphiz commented Jan 10, 2014

I take it - we have a winner? :)

@Memphiz
Team Kodi member
Memphiz commented Jan 12, 2014

i testet it together with your other 2 pull requests (volume up/down and the swipe vs. intertial scroll thingy) - somehow each gesture brings up the osd again - their was a state in between a couple of days ago where the osd stayed hidden ... not sure what was the intended behaviour here finally.

@koying
Team Kodi member
@Memphiz
Team Kodi member
Memphiz commented Jan 13, 2014

When i say OSD i normally mean the player controls and option buttons (like video options, audio options). This includes a progress aswell of course - but yeah i mean OSD ;)

@koying
Team Kodi member
koying commented Jan 13, 2014

Strange. Still fine by me (ie OSD stays hidden) and I haven't touched this one.
Does only including this one and not the others make a difference?

Also, could you send me your latest "test" touchscreen.xml, please.

@koying
Team Kodi member
koying commented Jan 13, 2014

Updated to only generate "detailed" actions for mouse/touch/gestures.
Superseeds #4015

@Memphiz
Team Kodi member
Memphiz commented Jan 13, 2014

Well seems i had a wonky build. The touch mapping on videofllscreen is fine: this is my latest xml:

http://pastebin.com/xkesr5XM

@jmarshallnz
Team Kodi member

Code looks good. Squash it down and ready to go once tested on iOS.

@koying
Team Kodi member
koying commented Jan 14, 2014

@Memphiz Looking at the Obj-C code (I have zero knowledge about ;) ), postMouseMotionEvent() is never called, afaict.
CGenericTouchActionHandler::Get().OnTouchGestureStart is called instead, which does the focusing on the "generic" side.
Do you still want to keep it?

@Memphiz
Team Kodi member
Memphiz commented Jan 14, 2014

Ignore it for now ... i am working for a replacement patch which handles focus on touchdown for ios.

@Memphiz
Team Kodi member
Memphiz commented Jan 14, 2014

@koying wip

Memphiz@6798693

I know get a proper focus whenever my finger goes down (that was not the case before - but its an uninteded bugfix ;) ).

Problem is now that i still trigger the swipe when i start scrolling through a list (an now i am using basically the same code as on android - i don't know why this doesn't happen on your android device :(

Beside that i have another issue which i need to track down (out of time for today though). I go into a list and do a longtap for bringing up the context menu. Now i hit the "x" of the context menu - when i do that the context menu vanishes and at the same time the stupid left hand slide out dialog slides out :(

All in all - its getting better on one side and worse on the other :( - i don't really want to hold off this PR - it seems ios related.

@koying
Team Kodi member
@koying
Team Kodi member
koying commented Jan 15, 2014

... but it is related to #4017 and not this one.
Let's first properly close this one. Could you retest with only this, excluding #4017 , please.

Expected results:

  • Swipe on fullscreen properly trigger mapped actions (e.g. play/pause/stepforward, ...) and doesn't trigger the OSD
  • Swipe with a mapping of vol up/vol down in [global] works
  • Obviously, no regression

Known issue (not regression):

  • a swipe on a inertial scrolling enabled control triggers both the action and intertial scrolling (to be solved by #4017 )
@koying
Team Kodi member
koying commented Feb 6, 2014

@Memphiz @jmarshallnz Ok to merge this one?

@Memphiz
Team Kodi member
Memphiz commented Feb 6, 2014

@koying please cherry-pick Memphiz@6798693 for this also i can confirm the behaviour as you described it in your last comment 22 days ago (which i have missed at that time sadly). Also you seem to have fixed the issue where tapping "x" in the context menu triggered the slide out menu - cool thing :)

@jmarshallnz
Team Kodi member

jenkins build this please

@jmarshallnz jmarshallnz merged commit a75ebdd into xbmc:master Feb 9, 2014

1 check passed

Details default Merged build #159 succeeded in 1 hr 21 min
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.