even slimmer scrollbars
ClosedPublic

Authored by mart on Jan 10 2018, 2:15 PM.

Details

Summary

the scrollbar handle will be very slim normally, and grow a bit on mouse
over, this makes it work better with QtQuickControls2 listviews where
the contents overlap the scrollbar (as other platform are going in
this direction since they all have scrollbars visible only on scroll
or mouse over)
this makes scrollbars appear more light but still always visible to
convey information

right now follows animations enabled to use this, as the scrollbar groove already did that, tough a "slim scrollbars" settings may make sense

Test Plan

works both as qwidget and with the qstyle-based qqc2-desktop-style

Diff Detail

Repository
R31 Breeze
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mart created this revision.Jan 10 2018, 2:15 PM
Restricted Application added projects: Plasma, Kirigami. · View Herald TranscriptJan 10 2018, 2:15 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
mart requested review of this revision.Jan 10 2018, 2:15 PM
mart edited the summary of this revision. (Show Details)
mart added a comment.Jan 10 2018, 2:18 PM

left sidebar: QML, main area: QWidgets

on mouse over

Hi,

That looks nice. I don't think it needs a separate settings, since once again the hit area is the same as before.
Then: would be nice to have VDG oppinion on that, and see if we can make the code less hackish (on the mouse-over detection side).

Hugo

kstyle/breezestyle.cpp
5048

This whole code feels hackish.
In principle we know (elsewhere in the code), if any part of the scrollbar is hovered, since it is used to make the groove appear.
I checked that this is done in a separate method though, namely Breeze::Style::drawScrollBarComplexControl, which calls the base class method in the end, which in turn ends up calling this method.
I wonder if we should not simply move the code from drawScrollbarSliderControl to the ComplexControl method, thus making the hacking above unnecessary. (makes sense ?)

kstyle/breezestyle.cpp
5048

One possibility, maybe, would be to call drawScrollbarSliderControl explicitely in drawScrollBarComplexControl rather than letting the parent class method do that, and just pass it a modified (smaller) rectangle when not mouseovered.

hpereiradacosta added a comment.EditedJan 10 2018, 3:10 PM

Checking the patch in action, it also indirecly changes color handling. In the previous implementation, focus color was used to render the 'non-mouse-over' scrollbar for the widget with focus. This is not the case anymore with this patch, because "disabled" is used unconditionally, when not mouse over. I am not sure this is a good thing. Maybe focus color should be preserved on the thin scrollbar.

abetts accepted this revision.Jan 10 2018, 3:24 PM
abetts added a subscriber: abetts.

I like it. I think it falls in line with a less obtrusive bar and one that demands attention.

This revision is now accepted and ready to land.Jan 10 2018, 3:24 PM
mart added inline comments.Jan 11 2018, 10:01 AM
kstyle/breezestyle.cpp
5048

you mean making drawScrollBarSliderControl a complete noop?

mart added inline comments.Jan 11 2018, 10:03 AM
kstyle/breezestyle.cpp
5048

i like the second option, calling by hand drawScrollBarSliderControl, tough it would still need to call the superclass drawScrollBarComplexControl? which then would have to know about not drawing the handle?

kstyle/breezestyle.cpp
5048

Yeah this needs some thinking.

Maybe not call the base class drawComplexControl and implement all the subcontrol calls ourselves ? Disable the call for slider subcontrol (CE_ScrollBarSlider), as we already do for CE_ScrollBarAddPage and SubPage ?

The second has the disadvantage that applications that call this control element directly (for say a custom scrollbar), will get broken.

mart updated this revision to Diff 25151.Jan 11 2018, 11:49 AM
  • use the same slider, animate its size
mart updated this revision to Diff 25152.Jan 11 2018, 11:51 AM
  • const opacity
mart added a comment.Jan 11 2018, 11:52 AM

so, a slightly different approach now, maybe it's a tad cleaner: don't try to draw the handle twice and cross fade it, but draw it smaller and then animate the handle size (and opacity) on mouse over

kstyle/breezestyle.cpp
5043

This generates some warnings here, because 0.7 + grooveAnimationOpacity can become larger than unity.

mart updated this revision to Diff 25240.Jan 12 2018, 3:21 PM

fix the math

mart marked an inline comment as done.Jan 12 2018, 3:21 PM

Looks great!
My only concern would be that in the mouseover state the contrast between the grip and groove is quite low. This could make the two difficult to distinguish, especially for color blind users. Could perhaps the contrast in terms of brightness be increased a bit?

Looks great!
My only concern would be that in the mouseover state the contrast between the grip and groove is quite low. This could make the two difficult to distinguish, especially for color blind users. Could perhaps the contrast in terms of brightness be increased a bit?

+1; maybe we could make the groove lighter?

Looks great!
My only concern would be that in the mouseover state the contrast between the grip and groove is quite low. This could make the two difficult to distinguish, especially for color blind users. Could perhaps the contrast in terms of brightness be increased a bit?

+1; maybe we could make the groove lighter?

But that should be a different patch. Mouse-over and scrollbar background have not been changed by this patch, and have been like that since day one.
No objection against changing them but

  • in a different patch
  • consistently with the rest of breeze. (the same shade of grey is used elsewhere, such as sliders, and progressbars, possibly also inactive tabs in tabs background.

As for the mouse-over color, for the handle itself, I think it comes from the palette unmodified.

Best,

Hugo

Dunno why I only noticed it now, but of course then this is not the right place to fix it. Sorry for the noise.

kstyle/breezestyle.cpp
5016

Ok. Just checked: this whole code should still work if replaced by:

const bool widgetMouseOver( _animations->scrollBarEngine().isHovered( widget, QStyle::SC_ScrollBarGroove ) );

(used elsewhere also)
it relies on the fact that hover state is properly updated, which seems to be the case, at least with QWidget.
Can you double check with QtQuick ? If yes, that would be a nice replacement.

mart added inline comments.Jan 15 2018, 10:22 AM
kstyle/breezestyle.cpp
5016

it works ok for the qwidget case, unfortunately in the case of qml, _animations will always be empty, so i still have to check option->styleObject anyways

mart updated this revision to Diff 25385.Jan 15 2018, 10:36 AM
  • shorter form
mart added inline comments.Jan 15 2018, 10:38 AM
kstyle/breezestyle.cpp
5016

this is the shortest form i could come up which works with both qwidgets and qml

hpereiradacosta accepted this revision.Jan 15 2018, 10:39 AM

Ship it ! Thanks !

Please merge after 5.12 branches.

This revision was automatically updated to reflect the committed changes.
mart added a comment.Jan 18 2018, 1:13 PM

landed just in master post 5.12

In Neon and Kubuntu CI with Qt 5.9.3

https://build.neon.kde.org/job/xenial_unstable_plasma_breeze_bin_amd64/186/console

17:55:15 /workspace/build/kstyle/breezestyle.cpp: In member function ‘virtual bool Breeze::Style::drawScrollBarSliderControl(const QStyleOption*, QPainter*, const QWidget*) const’:
17:55:15 /workspace/build/kstyle/breezestyle.cpp:5013:136: error: ‘const class QStyleOption’ has no member named ‘styleObject’
17:55:15 const bool widgetMouseOver( widget ? _animations->scrollBarEngine().isHovered( widget, QStyle::SC_ScrollBarGroove ) : option->styleObject->property("hover").toBool());
17:55:15 ^
17:55:15 kstyle/CMakeFiles/breeze.dir/build.make:910: recipe for target 'kstyle/CMakeFiles/breeze.dir/breezestyle.cpp.o' failed

In Neon and Kubuntu CI with Qt 5.9.3

https://build.neon.kde.org/job/xenial_unstable_plasma_breeze_bin_amd64/186/console

17:55:15 /workspace/build/kstyle/breezestyle.cpp: In member function ‘virtual bool Breeze::Style::drawScrollBarSliderControl(const QStyleOption*, QPainter*, const QWidget*) const’:
17:55:15 /workspace/build/kstyle/breezestyle.cpp:5013:136: error: ‘const class QStyleOption’ has no member named ‘styleObject’
17:55:15 const bool widgetMouseOver( widget ? _animations->scrollBarEngine().isHovered( widget, QStyle::SC_ScrollBarGroove ) : option->styleObject->property("hover").toBool());
17:55:15 ^
17:55:15 kstyle/CMakeFiles/breeze.dir/build.make:910: recipe for target 'kstyle/CMakeFiles/breeze.dir/breezestyle.cpp.o' failed

Thanks for reporting.
Looking at the log in more detail, this is in fact the qt4/kde4 built of breeze that fails.
And indeed styleObject is not defined for qt4.
Will add the necessary #ifdef asap.

Hugo

rikmills added a comment.EditedJan 20 2018, 1:25 PM

Thanks for reporting.
Looking at the log in more detail, this is in fact the qt4/kde4 built of breeze that fails.
And indeed styleObject is not defined for qt4.

Ah, I forgot that our builds were still making the Qt4 style. Also explains why the main KDE CI build did not fail!

Thanks

zzag added a subscriber: zzag.EditedApr 9 2018, 11:12 PM

I'm sorry, but these scrollbars have too thin sliders.

Slider occupies only 15% of the scrollbar in horizontal direction, that's a "big" waste of space.