Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Touch control for pushbuttons #76

Merged
merged 25 commits into from
Feb 22, 2014
Merged

Conversation

daschuer
Copy link
Member

@daschuer daschuer commented Oct 4, 2013

this patch allows simultaneous control of multiple pushbuttons.
It introduces [Controls]", "touch_shift", to rate touch events as right click.
This can be mapped to a "shift" keyboard key (single touch) or to an additional skin button (multi touch)
https://bugs.launchpad.net/mixxx/+bug/1217733

@esbrandt
Copy link
Contributor

esbrandt commented Oct 5, 2013

Can you give example how this could be utilized on a non-touch device within the Mixxx GUI, please?

@daschuer
Copy link
Member Author

daschuer commented Oct 5, 2013

This should effect touch devices only. I have no experiences how to test it without one.
Maybe something like this: https://github.com/vi/virtual_touchscreen helps here.

@esbrandt
Copy link
Contributor

esbrandt commented Oct 5, 2013

Ok, i`ll ask more precisely ;-)
What would you use the new control for, given a track-pad is available for testing?
Could you give an code example, how to utilize this in a skin.xml?

Thanks.

@daschuer
Copy link
Member Author

daschuer commented Oct 6, 2013

A track-pad makes no use of this feature, because it usually has two dedicated mouse buttons.
It should currently effect only touch screen devices.

Do we need a general solution for Mac single button mice as well?


I have converted the touch_shift to a ControlPushButton, so that it can be placed as an additional button in touch skins.

@rryan
Copy link
Member

rryan commented Oct 11, 2013

The way this is implemented only widgets that descend from WWidget will get the fake touch support. Other widgets (such as the library and a few other skin widgets like the waveforms / spinnies) won't have the support (and things like the touch_shift feature won't work with them which will be quite confusing).

Perhaps it could be implemented as a global event filter (see MixxxApp::eventFilter for how we do that for tooltips) though I'm not sure if all touch/mouse events go through that handler.

I tend to agree with Albert -- we should use a Qt-provided solution rather than trying to build the support ourself. When we re-write with QML touch will be supported natively everywhere, right?

@daschuer
Copy link
Member Author

Hi RJ,

thank you for review. Yes, you are right, it currently effects only WWidget.

But the issue is slightly different then Albert pointed out. First of all it is no option for me to wait for a QML skin, because IMHO we need a touch enabled GUI in 1.12.

Mixxx 1.11 currently has the default Qt touch support. But this suffers some problems that made Mixxx useless on a touch only device. These are pointed out in https://bugs.launchpad.net/mixxx/+bug/1152572 and in a significant attached video.

Two main problems of the Qt touch solution are :

  • You cannot use two buttons at the same time -> solution, save the "pressed" state per widget instance
  • Right click buttons are not supported -> fake right click by modifier control

My first Idea was fixing these issue at WPushButton Layer, but it turns out that if you call setAttribute(Qt::WA_AcceptTouchEvents); in one widget, the default Qt touch support is disabled for all widgets.
So if we need to override the Qt Touch support in one widget we need to implement our own touch support for all widgets.

For me, the library was out of focus when starting the patch because we have the Context menu key on keyboard.
But you are right, the new control should effect the library as well.

But lets clarify some things first:
Should we continue with a "[Controls]", "touch_shift" control?
Or is there an other possible solution for Mixxx 1.12?
Do we need a right click solution for all pointing devices like mouse and track-pad?
(I think no, because we have no user requests for it.)

@rryan
Copy link
Member

rryan commented Oct 11, 2013

I'm not against having touch capability, I just don't want us to rush head-first into a solution that we will have to support for years to come. I'm particularly wary of touch_shift because we will have to support it in the future even if our underlying touch support changes (e.g. we switch to QML).

What device are you using to test this?

I also don't see touch support as a blocker for Mixxx 1.12.0 -- it's not a top-requested user feature. I think the % of users who would be able to take advantage is quite low at this point as we're mostly talking about Windows 8 laptops with touch-screens and the Microsoft Surface.

BTW Mac OS X supports right-click by two-finger tap. It's a common right-click gesture for trackpads. I don't think touch_shift would be necessary for trackpads as they either have a real right-click button or they support a right-click gesture.

@daschuer
Copy link
Member Author

Ok, we have to be sure that our solution is not a quick hack. I have a "samsung ultrabook serie 5" for testing.

In Mixxx we cannot use a two finger tap. It is because the Mixxx pushbuttons behaves different to button controls in other applications. A normal Button accepts a click after releasing the mouse button on it. If you click it and move the mouse away from it the click is ignored. In Mixxx, the click command is executed as early as possible on the first push. This is the reason a two finger tab will not work. it will always execute the left click command as well, because it is nearly impossible to tab two fingers at ones. By the way: Some Mixxx buttons are to small for two of my big fingers :-)

I do not expect big changes in this with QML, because as far as I now the Qt touch support will be the same in QML. Or do you know a special document that describes the different in QML touch support?

Compared to DJ-Controllers the "touch_shift" control makes good points, because Mixxx uses the same button behavior and so it is natural to use the same solution for the secondary button function, a dedicated SHIFT button.

What could be an alternative solution?

@rryan
Copy link
Member

rryan commented Oct 12, 2013

Ah, yes I wasn't suggesting adding two-finger right-click support. Ideally the OS or GUI system should handle all this for us. (And OS X handles detecting the gestures and passing them to Mixxx as right-click events)

// pushbutton so "RightClickIsPushButton" is obsolete
m_rightPressed = true;
emit(valueChangedRightDown(1.0f));
update();

// Do not allow right-clicks to change button state other than when
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm kind of bummed about the removal of RightClickIsPushButton (and generally making right-click "lesser" than a left-click). My comments on this line were saying that we were keeping the pre-1.8.0 behavior but I always wanted to get rid of that behavior and allow right-click to change the state of multi-state push buttons if the skin author chose for it to be that way.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have decided to remove it, because a left click toggle button was not working (well). Since we currently do not use it nor have request for a fix, it was the easiest way to remove it. Do you think it worth to spend some time and reintroduce the left toggler? If you have a valid use case for it as test scenario, I can try to fix it.
.. or should we just move the "is obsolete" warning and fix it later on demand?

@daschuer
Copy link
Member Author

Ideally the OS or GUI system should handle all this for us. (And OS X handles detecting the gestures
and passing them to Mixxx as right-click events)

Right, that was my first idea as well, so I have started to collect the right click solutions at
https://bugs.launchpad.net/mixxx/+bug/1217733.

  • To finger tab -> will issue the left click as well
  • Drag out -> will issue the left click as well
  • Hold (Long press) -> will issue the left click as well
  • Context Menu button Or Shift+F10 -> you cannot focus a button without left click
  • Ctrl+Click/Tab -> works :-)

Unfortunately only the modifier key + tap works for our "exotic" buttons in Mixxx.
IMHO it is unlikely that the Oss will provide a Mixxx compatible right click solution soon.

With this patch, the user is able to configure the Ctrl+tap on all systems.
Or a Shift+Tap" to be in line with many midi devices.
Or a Skin button if no (on screen) keyboard is available.

Any other ideas?

@daschuer
Copy link
Member Author

From http://who-t.blogspot.de/2011/12/multitouch-in-x-pointer-emulation.html

Pointer emulation triggers motion events and, more importantly, button events. The button number for touches is > hardcoded to 1 (any more specific handling such as long-click for right buttons should be handled by touch-aware > clients instead), so the detail field of an emulated button event is 1 (unless the button is logically mapped).

@daschuer
Copy link
Member Author

This are my results from digging though the Qt code:

In Mixxx 1.11 we see the X11 pointer emulation on touch devices (not from Qt). Once we request a touch event by setting setAttribute(Qt::WA_AcceptTouchEvents) for a single widget, Mixxx becomes a multitouch application and the X11 pointer emulation is disabled.

Conclusion: Mixxx needs its own global pointer emulation, by overwriting bool QApplication::notify(QObject *receiver, QEvent *e)

@daschuer
Copy link
Member Author

To be much intuitive as possible, we should allow two finger tab and long tab gestures for calling the context menu library like in win 7/8 as well:
http://msdn.microsoft.com/en-US/library/windows/desktop/dd940543%28v=vs.85%29.aspx

Conflicts:
	src/mixxx.cpp
	src/mixxx.h
@daschuer
Copy link
Member Author

In Qt5 Qt does the Mouse pointer emulation of the touch event is not accepted by any widget.

By default, QGuiApplication translates the first touch point in a QTouchEvent into a QMouseEvent.

So as long Mixxx uses Qt4 it needs its own Mouse Pointer emulation.

@rryan
Copy link
Member

rryan commented Oct 26, 2013

Hey Daniel -- when I'm talking about native Qt support for gestures, I'm talking about QGestureRecognizer and the associated classes.

This is also a good slide deck on the topic: http://www.slideshare.net/qtbynokia/using-multitouch-and-gestures-with-qt

@rryan
Copy link
Member

rryan commented Feb 19, 2014

http://pastebin.com/fjsVzbtG

With this patch I get:

Stat("MixxxApplication::notify","count=55395,sum=4.76187e+06ns,average=85.9621ns,min=21ns,max=9102ns,variance=16078.9ns^2,stddev=126.802ns")

Stat("MixxxMainWindow::event","count=1213,sum=105504ns,average=86.9777ns,min=26ns,max=8885ns,variance=65442.2ns^2,stddev=255.817ns")

The max for notify dropped by an order of magnitude.

@rryan
Copy link
Member

rryan commented Feb 19, 2014

@daschuer could you report what you see on your netbook?

@rryan
Copy link
Member

rryan commented Feb 19, 2014

(Again with that patch -- mouse release events are not part of the measurement)

@daschuer
Copy link
Member Author

Hey we are on GitHub, why do you post patches ;-)

@daschuer
Copy link
Member Author

Ether I misread you patches, or you missed the line https://github.com/mixxxdj/mixxx/pull/76/files#diff-34f48af851ef6ea01364f1834e2b2d1fR113
For me it looks like
bool ret = QApplication::notify(target, event);
is included in the measurement.

}
case QEvent::MouseButtonRelease:
{
bool ret = QApplication::notify(target, event);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ^ line must not be included in the performance measurement.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right -- this block ends in return ret; on line 129 so this is not included in the timer. Note I am using Timer, not ScopedTimer. You have to manually tell it to record by calling elapsed and I do not call elapsed in this block.

@rryan
Copy link
Member

rryan commented Feb 19, 2014

Hey we are on GitHub, why do you post patches ;-)

Haha good question. I guess I should use a gist. Stuffing it in a pastebin was quickest in the moment.

@daschuer
Copy link
Member Author

A fist draft: http://www.mixxx.org/wiki/doku.php/touch

@daschuer
Copy link
Member Author

Here the result fom my i5 notebook:
Stat("MixxxApplication::notify","count=125750,sum=3.27885e+07ns,average=260.743ns,min=94ns,max=58001ns,variance=118623ns^2,stddev=344.417ns")

Stat("MixxxApplication::notifyRef","count=125750,sum=3.57041e+07ns,average=283.929ns,min=106ns,max=54543ns,variance=99238.4ns^2,stddev=315.021ns")

@daschuer
Copy link
Member Author

Here the results on my Atom notebook:
Stat("MixxxApplication::notify","count=62823,sum=2.55846e+08ns,average=4072.49ns,min=2793ns,max=1.25924e+06ns,variance=9.15079e+07ns^2,stddev=9565.98ns")
Stat("MixxxApplication::notifyRef","count=62823,sum=2.50449e+08ns,average=3986.58ns,min=2654ns,max=1.64169e+06ns,variance=1.31183e+08ns^2,stddev=11453.5ns")

@daschuer
Copy link
Member Author

Cool, my new device is 14 time faster :-)
latency drawback due to this patch:
avg: 86 ns Atom / -32 ns i5

Conclusion: the effect is somehow below the noise level of the scheduler.

@rryan
Copy link
Member

rryan commented Feb 20, 2014

Hm, interesting. This is with no touchscreen, right? (So it should be doing essentially no work at all).

The atom takes a max of >1ms :(. That feels like an eternity.

@daschuer
Copy link
Member Author

The vast majority of delay is produced due to the timer itself. MixxxApplication::notifyRef is just the timer without any code to measure. I also think that the max value is not that significant because we measure on the GUI thread and if we get suspended by the scheduler e.g, by the PA callback, we have an offset of a full PA cycle on on the measurement.

@daschuer
Copy link
Member Author

I think it save to merge now, because of no measurable timing regression. Thank you for review.

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.

4 participants