show a thin separator between view and scrollbar
ClosedPublic

Authored by mart on Jan 14 2020, 10:45 AM.

Details

Summary

This look makes listviews look way better, not having the selected
items look truncated into nothingness.
Always display "big" handles, due to the numerous complaints of the slim handles not being obvious

Test Plan

Before:


After:

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 14 2020, 10:45 AM
Restricted Application added a project: Plasma. · View Herald TranscriptJan 14 2020, 10:45 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
mart requested review of this revision.Jan 14 2020, 10:45 AM
mart edited the test plan for this revision. (Show Details)Jan 14 2020, 10:49 AM
mart edited the test plan for this revision. (Show Details)Jan 14 2020, 10:54 AM
mart added a reviewer: ngraham.
mart edited the summary of this revision. (Show Details)Jan 14 2020, 11:26 AM
mthw added a subscriber: mthw.Jan 14 2020, 2:44 PM

It does look better but looking at this (image) it looks like a step backwards. Wouldn't it be better to have the scrollbar floating above the content?

Hi Notmart,
you need to rebase (you have some unrelated changes in the diff, which revert latest change from Noah)

  • Concerning the separator, it looks great.
  • Concerning reverting the thin bar, I think this is really unfortunate. I expect you will have as many complaints after the revert for people to have it back, as now of people who want the thick one. I would be among the former. Going back and forth in these design issues really gives wrong impression IMHO on our design capabilities, and I think this would be one of VDG's task to avoid this at all cost.

Now, the thin bar might not work very well with the new separator (and both pursue opposite goals: one to make the scrollbar more visible and the other less visible).
But then again, it just means that we, (including our designers and usuability experts), keep going back and forth on which direction to follow (here regarding whether we should make the scrollbars more prominent or less prominent). This does not give a good impressing to the outside world IMHO.

  • In QQC there is an issue of overlap between item selection (and header titles) and scrollbar. Isn't that made even worth with the addition of the thin separator ?

Or is it now fixed ? (I could not see it in your other commit's screenshot. I do see it here in e.g. systemsettings)

mart added a comment.Jan 14 2020, 2:50 PM
In D26655#594135, @mthw wrote:

It does look better but looking at this (image) it looks like a step backwards. Wouldn't it be better to have the scrollbar floating above the content?

in general i would agree, but in the (very long) discussion of D26530 it was decided for this look

mthw added a comment.Jan 14 2020, 3:05 PM
In D26655#594149, @mart wrote:
In D26655#594135, @mthw wrote:

It does look better but looking at this (image) it looks like a step backwards. Wouldn't it be better to have the scrollbar floating above the content?

in general i would agree, but in the (very long) discussion of D26530 it was decided for this look

In that case, this patch does look better. The current state looks like neiher here, neither there. (If that is the correct way to say it).

In QQC there is an issue of overlap between item selection (and header titles) and scrollbar. Isn't that made even worth with the addition of the thin separator ?

That problem is caused by overlapping scrollbars, D26530 makes them no longer overlapping thus fixing that problem.

In QQC there is an issue of overlap between item selection (and header titles) and scrollbar. Isn't that made even worth with the addition of the thin separator ?

That problem is caused by overlapping scrollbars, D26530 makes them no longer overlapping thus fixing that problem.

Clear enough yes. Thanks !

ngraham accepted this revision as: VDG, ngraham.Jan 14 2020, 4:02 PM

Oh man, I think this just looks so good everywhere.

This revision is now accepted and ready to land.Jan 14 2020, 4:02 PM

Regarding the thickened scrollbars, I think it makes sense for a few reasons:

  • Acknowledging user feedback: we've had a bunch of complaints about the thin scrollbars.
  • Usability: even though the click area was the same size for the thin inactive scrollbars, it didn't look as large. This can make people subconsciously position their cursors more precisely than they need to
  • If we draw a separator line but keep the thin scrollbar, then the track looks much too wide because the thin inactive scroll handle looks lost in the wide track. But we can't make the track narrower since it has to accommodate the scroll handle's thicker expanded width too. Much simpler and more visually pleasing to just make it always thicker and not change its size on hover.
  • IMO the overall result just looks good. :)

Regarding the thickened scrollbars, I think it makes sense for a few reasons:

  • Acknowledging user feedback: we've had a bunch of complaints about the thin scrollbars.

With all due respect:
This is irrelevant unless you do an actual poll.
You will have user feedback that want the thin scrollbar back. (I will).

  • Usability: even though the click area was the same size for the thin inactive scrollbars, it didn't look as large. This can make people subconsciously position their cursors more precisely than they need to

my complain here is that this argument was either not made or discarded, when the first switch to thin scrollbar was done.
This is my main concern about this change: the going back and forth using adhoc arguments each time to justify the change, often contradicting each other. It means essentially that we either don't know what we are doing, or dont think our changes enough. This is bad imo

  • If we draw a separator line but keep the thin scrollbar, then the track looks much too wide because the thin inactive scroll handle looks lost in the wide track. But we can't make the track narrower since it has to accommodate the scroll handle's thicker expanded width too. Much simpler and more visually pleasing to just make it always thicker and not change its size on hover.

Matter of taste. It think I would be happy with thin handle and separator line.

  • IMO the overall result just looks good. :)

IMO so does the above (thin handle and separator line). With the advantage that it does not feel like a step backward.

This comment was removed by hpereiradacosta.

Removed previous comment because after testing the patch it turns out that this is not too strong of an issue.
However I also noticed that you not only removed the think handle but also the color blending. This make the handle not only larger but also stronger in terms of colors, especially in the case where the parent list has focus (you get a strong blue handle).
Is this really intended ?
I would at least keep the color blending, making the color stronger only when the scrollbar is actually hovered.

ngraham added a comment.EditedJan 14 2020, 4:46 PM

With all due respect:
This is irrelevant unless you do an actual poll.
You will have user feedback that want the thin scrollbar back. (I will).

Sure, you can't please everyone. However we have gotten bug reports: https://bugs.kde.org/show_bug.cgi?id=396747. In addition, I spend a lot of time interacting with the community through social media and my blog. And I watch a lot of YouTube videos of people reviewing Plasma or using it for the first time. In those venues, I see people expressing this complaint a lot.

I know it's not a scientific poll, but it's my impression. I can't do any better than that, I'm afraid.

my complain here is that this argument was either not made or discarded, when the first switch to thin scrollbar was done.
This is my main concern about this change: the going back and forth using adhoc arguments each time to justify the change, often contradicting each other. It means essentially that we either don't know what we are doing, or dont think our changes enough. This is bad imo

If I had been around back then or noticed the patch, I would have made this argument.

I understand the complaint about going back and forth on things. However I see it another way: it means we have the humility to acknowledge when a change didn't have the effect we were hoping. It means we are flexible and mature enough to try an experiment, see if it worked, and go back to the old thing if we see that it didn't work, or that our users didn't really like it. Personally I see this as a good thing.

Anyway, I respect your opinion, Hugo, and maybe we can find a compromise. Perhaps we could make the always-thick behavior on by default but optional?

mart added a comment.Jan 14 2020, 5:11 PM

Is this really intended ?
I would at least keep the color blending, making the color stronger only when the scrollbar is actually hovered.

ouch, well spotted, i will fix that :)

hpereiradacosta added a comment.EditedJan 14 2020, 5:12 PM

I would start by

  • rebase this patch to apply cleanly
  • split it in two (one for the separator line and one for the think handle removal), because these are really two features, adressing two different issues:

1: the item truncation into nothingness
2: the too small handle that few people dislike.

Adding an option does not really solve anything (especially if it is not the default) in terms of the back and forth.
If everyone is ok with the back and forth, you should go with it.
I can always hack my own breeze clone so that I am happy with it.
Just warning how some users could feel about it.

In D26655#594280, @mart wrote:

Is this really intended ?
I would at least keep the color blending, making the color stronger only when the scrollbar is actually hovered.

ouch, well spotted, i will fix that :)

Great :)
And rebase to latest master, so that the patch appears cleanly :)

ngraham edited the summary of this revision. (Show Details)Jan 14 2020, 6:49 PM

my complain here is that this argument was either not made or discarded, when the first switch to thin scrollbar was done.
This is my main concern about this change: the going back and forth using adhoc arguments each time to justify the change, often contradicting each other. It means essentially that we either don't know what we are doing, or dont think our changes enough. This is bad imo

If I had been around back then or noticed the patch, I would have made this argument.

For the record, you were around for the original commit that made the scrollbar thiner (https://phabricator.kde.org/D9792)

mart updated this revision to Diff 73601.Jan 15 2020, 9:26 AM

rebase on current master

mart updated this revision to Diff 73603.Jan 15 2020, 9:33 AM
  • remove the fat scrollbar part
mart added a comment.Jan 15 2020, 9:38 AM

Thick scrollbar moved to D26685

ngraham edited the summary of this revision. (Show Details)Jan 15 2020, 2:35 PM

For the record, you were around for the original commit that made the scrollbar thiner (https://phabricator.kde.org/D9792)

Heh, how embarrassing. I do remember that patch now, and I remember not really liking the idea it but not feeling like it was my place to object since I was quite new and I should give things a chance. And I wasn't around for D3210 where the scrollbars were first changed.

Hi again Marco,
since you mentionned centering in plasmoids in the other commits, in fact here also the centering is wrong, with the added separator. Probably the offset is the one pixel you take for the separator. Can you double check ? (this appears true for QWidget based scrollbars, not QQC). Likely you need to increase the scrollbar width by 1, and tune the positionning of the groove+handle.

ndavis added a subscriber: ndavis.Jan 15 2020, 4:16 PM

+1 to this change.

kstyle/breezestyle.cpp
6571

Use PenWidth::Frame instead of hardcoding 1.

kstyle/breezestyle.cpp
6574

Another thing:
Color role should be QPalette::Text rather than WindowText, since the vast majority of the scrollbars are rendered on views (color roles then being Base and Text)

mart updated this revision to Diff 73636.Jan 15 2020, 4:39 PM
  • center scrollbars
mart marked 2 inline comments as done.Jan 15 2020, 4:40 PM

comments should be adressed

Alignment again: so you increased the width of the groove rather than increasing the width of the scrollbar. This indeed fixes the alignment of the handle (and groove), but now the scrollbar arrows are off centered. (for the same reason), with both the scrollbar groove and with respect to the line. I would revert to the old handle width and rather increase the scrollbar width by 1 pixel. Then adjust the positioning. Does that make sense ? Increasing the width of the arrows by one pixel might break pixel alignment and create inconsistencies with other places were arrows are rendered ...
Now I realize that the thin scrollbar will still be off centered (and always was so far) while it is well centered with your patch, since it is only 3 pixels while the thick one is 6 pixels ... This was not visible without the separator line. But it is going to dissapear with the other patch anyway. (otherwise one would probably need to increase its width to 4 pixels)

mart updated this revision to Diff 73639.Jan 15 2020, 5:07 PM
  • Revert "center scrollbars"
  • align by growing scrollbar

yeah thanks !
Now, it turns out that the arrows are still ill-aligned (with respect to the handle). Maybe they use the full width for rendering ?

mart updated this revision to Diff 73642.Jan 15 2020, 6:19 PM
  • fix arrows too
mart added a comment.Jan 15 2020, 6:20 PM

now everything is centered.
for some reasons (i didn't fully understand that) the option->rect of scrollBarInternalSubControlRect was influenced by the previous calls of it, if i just substract.
now using absolute values for the rect, it always returns the proper value and everyuthing is centerd

hpereiradacosta accepted this revision.Jan 15 2020, 6:45 PM

Confirmed that the centering is fixed. Now the arrow seem to have moved vertically (for vertical scrollbars) by half a pixel, which is probably a consequence of the 20->21, and make them look somewhat thicker due to antialiasing. It is a minor detail, so feel free to ignore.
Other than that, no more comments !

This revision was automatically updated to reflect the committed changes.
mart added a comment.Jan 15 2020, 7:00 PM

Confirmed that the centering is fixed. Now the arrow seem to have moved vertically (for vertical scrollbars) by half a pixel, which is probably a consequence of the 20->21, and make them look somewhat thicker due to antialiasing. It is a minor detail, so feel free to ignore.
Other than that, no more comments !

moving the points half a pixel in renderArrow seems to fix it, but it may have sideeffects for arrows used in other places..

In D26655#595033, @mart wrote:

moving the points half a pixel in renderArrow seems to fix it, but it may have sideeffects for arrows used in other places..

Indeed, it would. I already made the arrows pixel aligned under normal circumstances, so the alignment fix needs to happen somewhere else.

ndavis added a comment.EditedJan 15 2020, 8:31 PM

I noticed that when hovering on the scrollbar border, while the view area is not focused, the scrollbar handle is light gray (Breeze Dark, dark gray for Breeze) instead of blue. Moving the mouse just slightly more to the right turns the scrollbar handle blue.

Not hovering:

Hovering on border:

Hovering inside border:

mart added a comment.Jan 16 2020, 9:49 AM

I noticed that when hovering on the scrollbar border, while the view area is not focused, the scrollbar handle is light gray (Breeze Dark, dark gray for Breeze) instead of blue. Moving the mouse just slightly more to the right turns the scrollbar handle blue.

That's the difference between thesubControlRect of the handle (that has been moved by one pixel to the right for the line) and the rectangle of the entire scrollbar.

to change this and have the handle always hitting, the subcontrolrects should stay the same and just paint the handle moved by one pixeltough the subcontrol rects not moving.
either is good, probably the latter approach looks a bit dirtier in the code but if you think is better from a design pov, it can be done

In D26655#595314, @mart wrote:

I noticed that when hovering on the scrollbar border, while the view area is not focused, the scrollbar handle is light gray (Breeze Dark, dark gray for Breeze) instead of blue. Moving the mouse just slightly more to the right turns the scrollbar handle blue.

That's the difference between thesubControlRect of the handle (that has been moved by one pixel to the right for the line) and the rectangle of the entire scrollbar.

to change this and have the handle always hitting, the subcontrolrects should stay the same and just paint the handle moved by one pixeltough the subcontrol rects not moving.
either is good, probably the latter approach looks a bit dirtier in the code but if you think is better from a design pov, it can be done

Yeah. I think one has to go this second way, to avoid the 1 pixel color flicker noted by Noah: scrollbar hover without handle hover.
Right now the scrollbar hit area includes the separator, while the handle does not.
Agreed that the code will look slightly less intuitive but so be it.

Hugo

As a side note: I have been using this code for a couple of days now, and while I think the separator looks well for vertical scrollbars, when you have both vertical and horizontal, ... this will need some getting used to.
See:


This is with a thin handle, but having the thick one does not really improve. The view looks like a frame inside a frame, first one being off-centered.
I have no clue how this can be improved

As a side note: I have been using this code for a couple of days now, and while I think the separator looks well for vertical scrollbars, when you have both vertical and horizontal, ... this will need some getting used to.
See:


This is with a thin handle, but having the thick one does not really improve. The view looks like a frame inside a frame, first one being off-centered.
I have no clue how this can be improved

Can the top of the scrollbar go below the column headers instead of next to them?

Like this:

Or maybe we could make the vertical scrollbar take up the bottom right corner:

Vertical scrollbar taking the corner without the column header change:

Can the top of the scrollbar go below the column headers instead of next to them?

Like this:

I don't think it is doable inside Qt no, due to how widgets are nested ( header in viewport, next to scrollbars, in scrollarea)
Also, for the last column this would pose serious problems on centering between column content and column header (for right align column in particular)

mart added a comment.Jan 20 2020, 10:56 AM

Yeah. I think one has to go this second way, to avoid the 1 pixel color flicker noted by Noah: scrollbar hover without handle hover.

D26783