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

QMLDemo: improve OrientationToggleButton touch friendlyness #3934

Merged

Conversation

Swiftb0y
Copy link
Member

@Swiftb0y Swiftb0y commented Jun 1, 2021

The previous implementation was hard to use on a touchscreen
because the surface where it was touching was very small.
The new version makes use of a slider internally which allows
the control to be changed more intiutively by swiping.

@github-actions github-actions bot added the skins label Jun 1, 2021
@Swiftb0y Swiftb0y force-pushed the qml-orientation-button-touchfriendlier branch 2 times, most recently from 12957af to f54c1ed Compare June 1, 2021 21:19
@Swiftb0y Swiftb0y requested a review from Holzhaus June 1, 2021 21:19
@Swiftb0y Swiftb0y added this to In progress in QML GUI via automation Jun 1, 2021
Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

Good idea, but this does not work. If I play deck 1, move the crossfader to the right and change the channel orientation, I still can't hear the deck. This is because you never actually set a value on the control proxy, just read it.

stepSize: 1
value: control.value
orientation: Qt.Horizontal
snapMode: Slider.SnapOnRelease
Copy link
Member

Choose a reason for hiding this comment

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

Obviously, you can't add value: orientationSlider.value to the control proxy because that would create an endless binding loop.

Instead, you only want to update the CO value when the slider was moved by the user, not on every value change.

Try this:

Suggested change
snapMode: Slider.SnapOnRelease
snapMode: Slider.SnapOnRelease
onMoved: control.value = value

Didn't test this, but it should work.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I thought I had figured it out but it seems like I didn't. Not sure onMoved will work because it might fire before releasing the control, but I'll try to figure something out.

Copy link
Member Author

Choose a reason for hiding this comment

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

Turns out using onMoved causes some weird issues where the visualposition and the value of the slider get out of sync. I'll have to think of another workaround...

Copy link
Member

Choose a reason for hiding this comment

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

I investigated a bit (using console.warn(value) 😉), and this should work.

Suggested change
snapMode: Slider.SnapOnRelease
snapMode: Slider.SnapOnRelease
onMoved: {
// The slider's `value` is not updated until after the move ended,
// so we need to calculate the value position ourselves here.
control.value = Math.round(visualPosition * (to - from));
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I don't know when I'll be able to get back on this though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should work now.

@Swiftb0y Swiftb0y force-pushed the qml-orientation-button-touchfriendlier branch 2 times, most recently from 5c0dfc9 to 49d4d6c Compare July 14, 2021 19:38
@Swiftb0y Swiftb0y requested a review from Holzhaus July 14, 2021 19:38
@coveralls
Copy link

coveralls commented Jul 14, 2021

Pull Request Test Coverage Report for Build 1033210008

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.004%) to 28.63%

Totals Coverage Status
Change from base Build 1025553951: 0.004%
Covered Lines: 20090
Relevant Lines: 70170

💛 - Coveralls

Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

Thanks, it works. But IMHO util.js makes the much more complicated than it needs to be.

Comment on lines 4 to 8
export function hasChanged(value) {
const changed = value != previous;
previous = value;
return changed;
}
Copy link
Member

Choose a reason for hiding this comment

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

Why is this a utility function? If I use this function for two unrelated values, the result is wrong, so it's not reusable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it is very primitive, I'm aware. It's not supposed to be perfect but just generally make sure that code that gets called repeatedly can debounce a bit so to speak.

onMoved: {
// The slider's `value` is not updated until after the move ended.
const val = valueAt(visualPosition);
if (Util.hasChanged(val))
Copy link
Member

Choose a reason for hiding this comment

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

Why not just val != control.value? This utility function makes it harder to understand what happens. It's also not reusable because it uses a global mutable variable (evil!).

Copy link
Member Author

Choose a reason for hiding this comment

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

Thats a good idea actually. How did I not think of that.

The previous implementation was hard to use on a touchscreen
because the surface where it was touching was very small.
The new version makes use of a slider internally which allows
the control to be changed more intiutively by swiping.
@Swiftb0y Swiftb0y force-pushed the qml-orientation-button-touchfriendlier branch from 49d4d6c to 18b7e69 Compare July 15, 2021 08:27
Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@Holzhaus Holzhaus merged commit 441cfed into mixxxdj:main Jul 15, 2021
QML GUI automation moved this from In progress to Done Jul 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants