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

Fix touch control #3165

Merged
merged 15 commits into from
Oct 31, 2020
Merged

Fix touch control #3165

merged 15 commits into from
Oct 31, 2020

Conversation

daschuer
Copy link
Member

@daschuer daschuer commented Oct 8, 2020

This fixes the broken touch control, reported here:
https://bugs.launchpad.net/mixxx/+bug/1895431

We rely here on our own touch solution for skin buttons. The rest uses the synthesized Qt events.

This was tested with Ubuntu Bionic

@daschuer daschuer added this to the 2.2.5 milestone Oct 8, 2020
@ehendrikd
Copy link
Contributor

I attempted to test this on a Raspberry Pi with a touchscreen, however as it is based on 2.2 it will not compile due to #2504 not being present.

@Holzhaus
Copy link
Member

Holzhaus commented Oct 9, 2020

@ehendrikd you can pull these changes into 2.3, e. g.:

git fetch upstream
git checkout -b daschuer-touchfix upstream/2.3
git pull https://github.com/daschuer/mixxx.git touchfix

@daschuer
Copy link
Member Author

daschuer commented Oct 9, 2020

unfortunately that conflicts.

I have prepared touchfix24 on top of master for testing:

git pull https://github.com/daschuer/mixxx.git touchfix24

@ehendrikd
Copy link
Contributor

Thank you, however I am perhaps a bit confused, the setAttribute(Qt::WA_AcceptTouchEvents); statement still exists in https://github.com/daschuer/mixxx/blob/touchfix24/src/mixxx.cpp#L219 but does not in https://github.com/daschuer/mixxx/blob/touchfix/src/mixxx.cpp

@daschuer
Copy link
Member Author

daschuer commented Oct 9, 2020

ups...
You are right if have missed to commit the merge.
Now it is up-to-date.

@ehendrikd
Copy link
Contributor

I can confirm https://github.com/daschuer/mixxx/tree/touchfix24 compiles and fixes the touchscreen issues on Raspberry Pis.

@Swiftb0y
Copy link
Member

Swiftb0y commented Oct 9, 2020

I also tested the PR on top of master (so I could use CMake because I don't have scons set up) and it also fixes the same issue I mentioned on my touchscreen laptop (running f32 on wayland). The library is also usable again, though obviously gestures for scrolling and so forth don't work. Also knobs are still unusable on a touchscreen which is a different issue, but other than that, touch works again.

@daschuer
Copy link
Member Author

daschuer commented Oct 9, 2020

Thank you for confirm.

Which gesture is used for scrolling?

Did you map the "touch_shift" control to access the second feature of the buttons?

@daschuer
Copy link
Member Author

daschuer commented Oct 9, 2020

There is a QScroller class that enables kinetic scrolling. Maybe we should introduce this.
https://doc.qt.io/qt-5/qscroller.html

By the way, here I can already scroll with two fingers.

Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

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

I am not able to test this, just some comments about the ugly code that lacks an explanation.

@@ -38,7 +38,7 @@ WWidget::~WWidget() {
}

bool WWidget::touchIsRightButton() {
return (m_pTouchShift->get() != 0.0);
return (m_pTouchShift->toBool());
Copy link
Contributor

Choose a reason for hiding this comment

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

Redundant parantheses

bool MixxxApplication::touchIsRightButton() {
if (!m_pTouchShift) {
m_pTouchShift = new ControlProxy(
"[Controls]", "touch_shift", this);
}
return (m_pTouchShift->get() != 0.0);
return (m_pTouchShift->toBool());
Copy link
Contributor

Choose a reason for hiding this comment

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

Redundant parantheses

QMouseEventEditable* mouseEvent = static_cast<QMouseEventEditable*>(event);
if (mouseEvent->source() == Qt::MouseEventSynthesizedByQt &&
mouseEvent->button() == Qt::LeftButton &&
m_rightPessedButtons > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

VERIFY_OR_DEBUG_ASSERT(m_rightPessedButtons > 0) and early break otherwise?

Using a debug assert for increasing the counter but not for decreasing it would be inconsequent. Both parts of the code should be symmetrical.

Copy link
Member Author

Choose a reason for hiding this comment

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

m_rightPessedButtons is zero when there is no right button pressed. This is an expected case.
The assertion in the other path verifies the assumption that QT synthesizes only one click at a time. I will add a comment.

mouseEvent->button() == Qt::LeftButton &&
touchIsRightButton()) {
mouseEvent->setButton(Qt::RightButton);
m_rightPessedButtons++;
Copy link
Contributor

Choose a reason for hiding this comment

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

VERIFY_OR_DEBUG_ASSERT(m_rightPessedButtons < 2) and early break otherwise?

Copy link
Member Author

Choose a reason for hiding this comment

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

Solved, see above.

src/mixxxapplication.cpp Show resolved Hide resolved
class QMouseEventEditable : public QMouseEvent {
public:
// Inherit constructors from base class
using QMouseEvent::QMouseEvent;
Copy link
Contributor

Choose a reason for hiding this comment

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

This class must never be constructed.

@Holzhaus Holzhaus marked this pull request as draft October 13, 2020 12:18
@Holzhaus
Copy link
Member

I converted this to draft since there are unresolved review comments.

@daschuer daschuer marked this pull request as ready for review October 14, 2020 10:05
@daschuer
Copy link
Member Author

Done.

@Holzhaus
Copy link
Member

There is a merge conflict now.

@Holzhaus
Copy link
Member

Merge conflicts again.

@Holzhaus Holzhaus marked this pull request as draft October 24, 2020 13:06
@daschuer daschuer marked this pull request as ready for review October 25, 2020 11:25
@daschuer
Copy link
Member Author

This is merge able again.

m_fakeMouseWidget(NULL),
m_activeTouchButton(Qt::NoButton),
m_pTouchShift(NULL) {
m_rightPessedButtons(0),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
m_rightPessedButtons(0),
m_rightPressedButtons(0),

src/mixxxapplication.cpp Outdated Show resolved Hide resolved
@@ -219,7 +219,6 @@ void MixxxMainWindow::initialize(QApplication* pApp, const CmdlineArgs& args) {
pConfig->getValue(ConfigKey("[Controls]", "Tooltips"),
static_cast<int>(mixxx::TooltipsPreference::TOOLTIPS_ON)));

setAttribute(Qt::WA_AcceptTouchEvents);
Copy link
Contributor

Choose a reason for hiding this comment

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

I am concerned this hack may prevent us from implementing custom touch gestures. However, we currently have a major bug to fix so we can defer that issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

Any widget can enable touch events individually.

This tells Qt that the MixxxMainWindow itself has a event handler that accept touch events.
This hack was required in early Qt versions to detect that no child widget has consumed the touch and we could resend it as mouse click. Now Qt calculated this forehead, which disables the mouse click synthesizer.

@Be-ing
Copy link
Contributor

Be-ing commented Oct 26, 2020

This fixes touch input for the library but there is still the critical bug that no onscreen keyboard is shown when searching the library.

daschuer and others added 2 commits October 27, 2020 20:33
Co-authored-by: Be <be.0@gmx.com>
@daschuer
Copy link
Member Author

Done.

@daschuer
Copy link
Member Author

CI failures are unrelated.

@Be-ing
Copy link
Contributor

Be-ing commented Oct 28, 2020

Looks good to me. Waiting for CI.

@Be-ing
Copy link
Contributor

Be-ing commented Oct 29, 2020

There is a merge conflict with CHANGELOG now.

@Be-ing
Copy link
Contributor

Be-ing commented Oct 29, 2020

I don't know what's going on with the CI failures on the 2.2 branch but it doesn't seem to be related to the changes in this PR.

@daschuer
Copy link
Member Author

That is actually the fifth time this PR is in conflict due to CHANGELOG
Is there a way to avoid it? It is really anoying.

@daschuer
Copy link
Member Author

https://about.gitlab.com/blog/2018/07/03/solving-gitlabs-changelog-conflict-crisis/

@Holzhaus Holzhaus marked this pull request as draft October 30, 2020 15:43
@daschuer
Copy link
Member Author

Done.

@daschuer daschuer marked this pull request as ready for review October 31, 2020 12:03
@Be-ing Be-ing merged commit 9a3a1e7 into mixxxdj:2.2 Oct 31, 2020
@uklotzde uklotzde modified the milestones: 2.2.5, 2.3.0 May 11, 2021
@daschuer daschuer deleted the touchfix branch September 26, 2021 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants