D26655: show a thin separator between view and scrollbar

[Marco Martin]

mart added a comment.


  In D26655#595380 , 
@hpereiradacosta wrote:
  
  > 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 

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham, hpereiradacosta
Cc: ndavis, ahiemstra, hpereiradacosta, mthw, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  > Can the top of the scrollbar go below the column headers instead of next to 
them?
  > 
  > Like this:
  >  F7886305: Screenshot_20200116_083629.png 

  
  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)

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham, hpereiradacosta
Cc: ndavis, ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Noah Davis]

ndavis added a comment.


  Vertical scrollbar taking the corner without the column header change: 
F7886334: Screenshot_20200116_083629.png 

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham, hpereiradacosta
Cc: ndavis, ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Noah Davis]

ndavis added a comment.


  In D26655#595381 , 
@hpereiradacosta wrote:
  
  > 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: 
  >  F7886206: Screenshot_20200116_083629.png 

  >  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:
  F7886305: Screenshot_20200116_083629.png 

  
  Or maybe we could make the vertical scrollbar take up the bottom right corner:
  F7886321: Screenshot_20200116_083629.png 


REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham, hpereiradacosta
Cc: ndavis, ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  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: 
  F7886206: Screenshot_20200116_083629.png 

  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

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham, hpereiradacosta
Cc: ndavis, ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  In D26655#595314 , @mart wrote:
  
  > In D26655#595102 , @ndavis 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

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham, hpereiradacosta
Cc: ndavis, ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Marco Martin]

mart added a comment.


  In D26655#595102 , @ndavis 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

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham, hpereiradacosta
Cc: ndavis, ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Noah Davis]

ndavis added a comment.


  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.
  F7884963: Screenshot_20200115_152752.PNG 


REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham, hpereiradacosta
Cc: ndavis, ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Noah Davis]

ndavis added a comment.


  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.

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham, hpereiradacosta
Cc: ndavis, ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Marco Martin]

mart added a comment.


  In D26655#595022 , 
@hpereiradacosta wrote:
  
  > 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..

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham, hpereiradacosta
Cc: ndavis, ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Marco Martin]

This revision was automatically updated to reflect the committed changes.
Closed by commit R31:df1340617147: show a thin separator between view and 
scrollbar (authored by mart).

REPOSITORY
  R31 Breeze

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26655?vs=73642&id=73646

REVISION DETAIL
  https://phabricator.kde.org/D26655

AFFECTED FILES
  kstyle/breeze.h
  kstyle/breezehelper.cpp
  kstyle/breezehelper.h
  kstyle/breezestyle.cpp

To: mart, #plasma, #breeze, #vdg, ngraham, hpereiradacosta
Cc: ndavis, ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Hugo Pereira Da Costa]

hpereiradacosta accepted this revision.
hpereiradacosta added a comment.


  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 !

REPOSITORY
  R31 Breeze

BRANCH
  arcpatch-D26655

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham, hpereiradacosta
Cc: ndavis, ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Marco Martin]

mart added a comment.


  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

REPOSITORY
  R31 Breeze

BRANCH
  arcpatch-D26655

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: ndavis, ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Marco Martin]

mart updated this revision to Diff 73642.
mart added a comment.


  - fix arrows too

REPOSITORY
  R31 Breeze

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26655?vs=73639&id=73642

BRANCH
  arcpatch-D26655

REVISION DETAIL
  https://phabricator.kde.org/D26655

AFFECTED FILES
  kstyle/breeze.h
  kstyle/breezehelper.cpp
  kstyle/breezehelper.h
  kstyle/breezestyle.cpp

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: ndavis, ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  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 ?

REPOSITORY
  R31 Breeze

BRANCH
  arcpatch-D26655

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: ndavis, ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Marco Martin]

mart updated this revision to Diff 73639.
mart added a comment.


  - Revert "center scrollbars"
  - align by growing scrollbar

REPOSITORY
  R31 Breeze

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26655?vs=73636&id=73639

BRANCH
  arcpatch-D26655

REVISION DETAIL
  https://phabricator.kde.org/D26655

AFFECTED FILES
  kstyle/breeze.h
  kstyle/breezehelper.cpp
  kstyle/breezehelper.h
  kstyle/breezestyle.cpp

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: ndavis, ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  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)

REPOSITORY
  R31 Breeze

BRANCH
  arcpatch-D26655

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: ndavis, ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Marco Martin]

mart updated this revision to Diff 73636.
mart added a comment.


  - center scrollbars

REPOSITORY
  R31 Breeze

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26655?vs=73603&id=73636

BRANCH
  arcpatch-D26655

REVISION DETAIL
  https://phabricator.kde.org/D26655

AFFECTED FILES
  kstyle/breeze.h
  kstyle/breezehelper.cpp
  kstyle/breezehelper.h
  kstyle/breezestyle.cpp

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: ndavis, ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Marco Martin]

mart marked 2 inline comments as done.
mart added a comment.


  comments should be adressed

REPOSITORY
  R31 Breeze

BRANCH
  arcpatch-D26655

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: ndavis, ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Hugo Pereira Da Costa]

hpereiradacosta added inline comments.

INLINE COMMENTS

> breezestyle.cpp:6570
> +
> +_helper->renderScrollBarBorder( painter, separatorRect, 
> _helper->alphaColor( option->palette.color( QPalette::WindowText ), 0.1 ));
> +

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)

REPOSITORY
  R31 Breeze

BRANCH
  mart/phab/scrollbarseparator2

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: ndavis, ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Noah Davis]

ndavis added a comment.


  +1 to this change.

INLINE COMMENTS

> breezestyle.cpp:6567
> +Qt::AlignLeft,
> +QSize(1, option->rect.height()), 
> option->rect);
> +}

Use PenWidth::Frame instead of hardcoding 1.

REPOSITORY
  R31 Breeze

BRANCH
  mart/phab/scrollbarseparator2

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: ndavis, ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  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.

REPOSITORY
  R31 Breeze

BRANCH
  mart/phab/scrollbarseparator2

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Nathaniel Graham]

ngraham added a comment.


  In D26655#594415 , 
@hpereiradacosta wrote:
  
  > 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.

REPOSITORY
  R31 Breeze

BRANCH
  mart/phab/scrollbarseparator2

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Marco Martin]

mart added a comment.


  Thick scrollbar moved to D26685 

REPOSITORY
  R31 Breeze

BRANCH
  mart/phab/scrollbarseparator2

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Marco Martin]

mart updated this revision to Diff 73603.
mart added a comment.


  - remove the fat scrollbar part

REPOSITORY
  R31 Breeze

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26655?vs=73601&id=73603

BRANCH
  mart/phab/scrollbarseparator2

REVISION DETAIL
  https://phabricator.kde.org/D26655

AFFECTED FILES
  kstyle/breezehelper.cpp
  kstyle/breezehelper.h
  kstyle/breezestyle.cpp

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Marco Martin]

mart updated this revision to Diff 73601.
mart added a comment.


  rebase on current master

REPOSITORY
  R31 Breeze

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26655?vs=73503&id=73601

BRANCH
  mart/phab/scrollbarseparator2

REVISION DETAIL
  https://phabricator.kde.org/D26655

AFFECTED FILES
  kstyle/breezehelper.cpp
  kstyle/breezehelper.h
  kstyle/breezestyle.cpp

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  >> 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)

REPOSITORY
  R31 Breeze

BRANCH
  phab/scrollbarseparator

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  In D26655#594280 , @mart wrote:
  
  > In D26655#594257 , 
@hpereiradacosta 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 :)

REPOSITORY
  R31 Breeze

BRANCH
  phab/scrollbarseparator

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  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 floating bar
  2: the two 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.

REPOSITORY
  R31 Breeze

BRANCH
  phab/scrollbarseparator

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Marco Martin]

mart added a comment.


  In D26655#594257 , 
@hpereiradacosta 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 :)

REPOSITORY
  R31 Breeze

BRANCH
  phab/scrollbarseparator

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Nathaniel Graham]

ngraham added a comment.


  In D26655#594250 , 
@hpereiradacosta wrote:
  
  > 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 video of people reviewing Plasma. 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 that 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?

REPOSITORY
  R31 Breeze

BRANCH
  phab/scrollbarseparator

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  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.

REPOSITORY
  R31 Breeze

BRANCH
  phab/scrollbarseparator

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  Also, concerning the separator line: how does it look with hovered 
scrollbars, when the handle grove is also rendered. 
  Isn't there some redundancy between the handle groove and the separator ? 
(essentially a frame inside a frame)
  should the handle groove be removed entirely ?

REPOSITORY
  R31 Breeze

BRANCH
  phab/scrollbarseparator

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  In D26655#594240 , @ngraham wrote:
  
  > 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.
  
  > F7883097: Screenshot_20200114_091952.png 

  > 
  > F7883099: Scrollers.png 
  > 
  > F7883100: Scroller 2.png 

REPOSITORY
  R31 Breeze

BRANCH
  phab/scrollbarseparator

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Nathaniel Graham]

ngraham added a comment.


  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. :)
  
  F7883097: Screenshot_20200114_091952.png 

  
  F7883099: Scrollers.png 
  
  F7883100: Scroller 2.png 

REPOSITORY
  R31 Breeze

BRANCH
  phab/scrollbarseparator

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Nathaniel Graham]

ngraham accepted this revision as: VDG, ngraham.
ngraham added a comment.
This revision is now accepted and ready to land.


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

REPOSITORY
  R31 Breeze

BRANCH
  phab/scrollbarseparator

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  In D26655#594173 , @ahiemstra 
wrote:
  
  > > 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 !

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Arjen Hiemstra]

ahiemstra added a comment.


  > 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.

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: ahiemstra, hpereiradacosta, mthw, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D26655: show a thin separator between view and scrollbar

[Matej Mrenica]

mthw added a comment.


  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).

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: hpereiradacosta, mthw, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26655: show a thin separator between view and scrollbar

[Marco Martin]

mart added a comment.


  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

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: hpereiradacosta, mthw, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26655: show a thin separator between view and scrollbar

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  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 ?

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: hpereiradacosta, mthw, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26655: show a thin separator between view and scrollbar

[Matej Mrenica]

mthw added a comment.


  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?

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D26655

To: mart, #plasma, #breeze, #vdg, ngraham
Cc: mthw, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26655: show a thin separator between view and scrollbar

[Marco Martin]

mart created this revision.
mart added reviewers: Plasma, Breeze, VDG.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
mart requested review of this revision.

REVISION SUMMARY
  This look makes listviews look way better, not having the selected
  items look truncated into nothingness.

REPOSITORY
  R31 Breeze

BRANCH
  phab/scrollbarseparator

REVISION DETAIL
  https://phabricator.kde.org/D26655

AFFECTED FILES
  kstyle/breezehelper.cpp
  kstyle/breezehelper.h
  kstyle/breezestyle.cpp

To: mart, #plasma, #breeze, #vdg
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart