[QQC2 Slider Style] Fix wrong handle positioning when initial value is 1
ClosedPublic

Authored by filipf on May 7 2019, 10:34 PM.

Details

Summary

Old code was not reading value properly when initial value was 1, misplacing the handle as a result.

BUG: 405471

Test Plan

Tested by running qmlscene with:

import QtQuick.Controls 2.5

Slider {
    from: 1
    to: 5
    stepSize: 1
    //orientation: Qt.Vertical
}

Tested both in horizontal and vertical orientation.

You can also test this in the Mouse KCM, with the Pointer Speed slider.

Diff Detail

Repository
R858 Qt Quick Controls 2: Desktop Style
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
filipf created this revision.May 7 2019, 10:34 PM
Restricted Application added a project: Plasma. · View Herald TranscriptMay 7 2019, 10:34 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
filipf requested review of this revision.May 7 2019, 10:34 PM
filipf edited the test plan for this revision. (Show Details)May 7 2019, 10:37 PM
filipf added reviewers: mart, Plasma, ngraham.
filipf edited the test plan for this revision. (Show Details)May 7 2019, 10:50 PM
ngraham accepted this revision.May 8 2019, 2:58 AM

Very nice.

@mart, does this look like the right fix?

This revision is now accepted and ready to land.May 8 2019, 2:58 AM
broulik added a subscriber: broulik.May 8 2019, 7:39 AM

-1
This breaks the slider in right-to-left mode. Run qmlscene -reverse to see

broulik requested changes to this revision.May 8 2019, 7:39 AM
This revision now requires changes to proceed.May 8 2019, 7:39 AM
filipf added a comment.May 8 2019, 9:20 AM

-1
This breaks the slider in right-to-left mode. Run qmlscene -reverse to see

Hmm seems to be behaving the same as before, just that the handle is now on the tickmark.

But the problem with reverse is that if I drag it right it goes left? And we see proper behavior with XDG_CURRENT_DESKTOP=QT qmlscene SliderTest.qml -reverse then?

Hm, right, seems the slider behaves incorrectly without this patch, too :/ Still I think visualPosition is what we want to be using

filipf added a comment.EditedMay 8 2019, 9:45 AM

Hm, right, seems the slider behaves incorrectly without this patch, too :/ Still I think visualPosition is what we want to be using

About Qt's unmodified slider, isn't that one wrong as well ie. shouldn't the values be reversed? So that 1 is on the right, 5 is on the left? But when we fire it up it behaves exactly the same as if there is no reverse flag.

So from my understanding our slider should fix both that and then the drag behavior once we figure that out.

filipf updated this revision to Diff 57752.May 8 2019, 11:02 AM

fix handle behavior for right to left layouts

mart accepted this revision.May 8 2019, 2:05 PM
filipf added a comment.May 8 2019, 7:57 PM

So I looked up Google's documentation and it turns out sliders do have to be mirrored:
https://material.io/design/usability/bidirectionality.html#mirroring-elements
https://material.io/design/components/sliders.html#usage

As for the implementation, @broulik mentioned it would be better to use controlRoot.mirrored for mirroring and controlRoot.visualPosition for value. I couldn't come up with something that would work well, but if anyone has ideas I'll change the current code.

This revision was not accepted when it landed; it landed in state Needs Review.May 15 2019, 4:34 PM
This revision was automatically updated to reflect the committed changes.