D29264: New tabs for Breeze

[Manuel Jesús de la Fuente]

manueljlin added a comment.


  Invent link in case it's useful for anyone: 
https://invent.kde.org/plasma/breeze/-/merge_requests/6

REPOSITORY
  R31 Breeze

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

To: abstractdevelop, #vdg, #breeze
Cc: bcooksley, ndavis, niccolove, manueljlin, filipf, ngraham, cblack, 
plasma-devel, dmenig, Orage, LeGast00n, The-Feren-OS-Dev, konkinartem, ian, 
jguidon, Ghost6, jraleigh, zachus, fbampaloukas, squeakypancakes, alexde, 
IohannesPetros, GB_2, trickyricky26, ragreen, crozbo, ZrenBot, firef, skadinna, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, aaronhoneycutt, abetts, 
sebas, apol, ahiemstra, mbohlender, mart

D29264: New tabs for Breeze

[Ben Cooksley]

bcooksley added a comment.


  Please note that should you have a KDE Developer account, then you can use a 
work branch instead of forking the repository.
  For more information on these please see my email to kde-devel and 
kde-cvs-announce.

REPOSITORY
  R31 Breeze

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

To: abstractdevelop, #vdg, #breeze
Cc: bcooksley, ndavis, niccolove, manueljlin, filipf, ngraham, cblack, 
plasma-devel, dmenig, Orage, LeGast00n, The-Feren-OS-Dev, konkinartem, ian, 
jguidon, Ghost6, jraleigh, zachus, fbampaloukas, squeakypancakes, alexde, 
IohannesPetros, GB_2, trickyricky26, ragreen, crozbo, ZrenBot, firef, skadinna, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, aaronhoneycutt, abetts, 
sebas, apol, ahiemstra, mbohlender, mart

D29264: New tabs for Breeze

[Noah Davis]

ndavis added a comment.


  Would you mind moving this to GitLab? You'll just need to fork the breeze 
repo, add your fork as a remote to the existing repo you're working on and push 
your branch to the fork. For instance, this is what I do for myself:
  
git remote add ndavis_fork g...@invent.kde.org:ndavis/breeze.git
git push ndavis_fork
  
  After you push, you can make a Merge Request. A link to make a merge request 
will even show up in the terminal when you push.

REPOSITORY
  R31 Breeze

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

To: abstractdevelop, #vdg, #breeze
Cc: ndavis, niccolove, manueljlin, filipf, ngraham, cblack, plasma-devel, 
dmenig, Orage, LeGast00n, The-Feren-OS-Dev, konkinartem, ian, jguidon, Ghost6, 
jraleigh, zachus, fbampaloukas, squeakypancakes, alexde, IohannesPetros, GB_2, 
trickyricky26, ragreen, crozbo, ZrenBot, firef, skadinna, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, aaronhoneycutt, abetts, sebas, apol, ahiemstra, 
mbohlender, mart

D29264: New tabs for Breeze

[Niccolò Venerandi]

niccolove added a comment.


  +1 for the direction!

REPOSITORY
  R31 Breeze

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

To: abstractdevelop, #vdg, #breeze
Cc: niccolove, manueljlin, filipf, ngraham, cblack, plasma-devel, dmenig, 
Orage, LeGast00n, The-Feren-OS-Dev, konkinartem, ian, jguidon, Ghost6, 
jraleigh, zachus, fbampaloukas, squeakypancakes, alexde, IohannesPetros, GB_2, 
trickyricky26, ragreen, crozbo, ndavis, ZrenBot, firef, skadinna, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, aaronhoneycutt, abetts, sebas, apol, 
ahiemstra, mbohlender, mart

D29264: New tabs for Breeze

[Nathaniel Graham]

ngraham added a comment.


  The tab widget's active tab background color is something that can be played 
with in code--and already is, in fact. With the default Breeze color scheme, 
observe how it has a subtly lightened background color compared to the rest of 
the view background.
  
  Since everybody loves @manualjlin's mockups, faithfully implementing those 
always seems like a good idea to me. :)

REPOSITORY
  R31 Breeze

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

To: abstractdevelop, #vdg, #breeze
Cc: manueljlin, filipf, ngraham, cblack, plasma-devel, dmenig, Orage, 
LeGast00n, The-Feren-OS-Dev, konkinartem, ian, jguidon, Ghost6, jraleigh, 
zachus, fbampaloukas, squeakypancakes, alexde, IohannesPetros, GB_2, 
trickyricky26, ragreen, crozbo, ndavis, ZrenBot, firef, skadinna, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, aaronhoneycutt, abetts, sebas, apol, 
ahiemstra, mbohlender, mart

D29264: New tabs for Breeze

[Abstract Software]

abstractdevelop added a comment.


  Sorry everybody for not seeing all the new comments sooner! :D
  
  Ok, it seems like the some corners being rounded is a common complaint, so we 
should address it. I agree that it looks a little out of place in System 
Settings. The only trouble is doing this in a way which respects the line at 
the top.
  
  As for the inactive tab background, the problem is that I can't seem to find 
a QPalette color which is not too light/dark. The current one 
(QPalette::Disabled, QPalette::Window) is the best I could find, but we can go 
back to using the rather extreme dark one as is used in the current tabs, which 
IMO doesn't look good with this style. We could also make the current tab 
background *lighter* instead, as in manueljlins mockups where it is almost 
white. But that appears to be an issue left to colorshemes and not to the code. 
;)

REPOSITORY
  R31 Breeze

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

To: abstractdevelop, #vdg, #breeze
Cc: manueljlin, filipf, ngraham, cblack, plasma-devel, dmenig, Orage, 
LeGast00n, The-Feren-OS-Dev, konkinartem, ian, jguidon, Ghost6, jraleigh, 
zachus, fbampaloukas, squeakypancakes, alexde, IohannesPetros, GB_2, 
trickyricky26, ragreen, crozbo, ndavis, ZrenBot, firef, skadinna, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, aaronhoneycutt, abetts, sebas, apol, 
ahiemstra, mbohlender, mart

D29264: New tabs for Breeze

[Manuel Jesús de la Fuente]

manueljlin added a comment.


  In D29264#663444 , @ngraham wrote:
  
  > I would suggest to re-add the darkened background for inactive tabs
  
  
  +1
  I think it would still look fine with the 1px color line rounded at the top
  
  F8289464: image.png 
  
  2x
  F8289499: image.png 
  
  Maybe it could even be extended to the top of the outline so it's 2px, but 
that might be slightly hacky and/or inconsistent
  F8289535: image.png 
  
  > Using only color to communicate the "current tab" state would jive with 
that, but then I start to think that we don't have enough color here and that 
it should look more like a Plasma tab, with a thicker highlight bar and the 
whole background colored
  
  Hmm, I think that might look out of place seeing how breeze mostly uses
  1px lines. I also believe that instead of coloring the entire tab background
  with the highlight color, it should just use contrasting colors (selected tab
  and content with view bg, the slightly darker inactive tab bg and the
  outline of the tabs should work)

REPOSITORY
  R31 Breeze

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

To: abstractdevelop, #vdg, #breeze
Cc: manueljlin, filipf, ngraham, cblack, plasma-devel, dmenig, Orage, 
LeGast00n, The-Feren-OS-Dev, konkinartem, ian, jguidon, Ghost6, jraleigh, 
zachus, fbampaloukas, squeakypancakes, alexde, IohannesPetros, GB_2, 
trickyricky26, ragreen, crozbo, ndavis, ZrenBot, firef, skadinna, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, aaronhoneycutt, abetts, sebas, apol, 
ahiemstra, mbohlender, mart

D29264: New tabs for Breeze

[Nathaniel Graham]

ngraham added a comment.


  In general I think this is a good direction to move in. However I don't think 
it's quite ready yet.
  
  Small nitpick: this will result in framed tab views having rounded bottom 
corners, but at least one pointy top corner: F8284555: 
Screenshot_20200504_140532.png 
  
  That should be easily fixable though.
  
  I think the blue line on top is an improvement and brings the tabs closer to 
the Plasma appearance, which is desirable from a #consistency 
 point of view. However I'm 
finding that the current tab just doesn't look distinct enough from un-focused 
tabs anymore. I would suggest to re-add the darkened background for inactive 
tabs, but then we run into T13054: Unify how Breeze communicates 
checked/current/selected states . Using 
only color to communicate the "current tab" state would jive with that, but 
then I start to think that we don't have //enough// color here and that it 
should look more like a Plasma tab, with a thicker highlight bar and the whole 
background colored. But then if we do that, we're fully abandoning @cblack's 
idea to make tab appearance dependent on the type of view (editable/document vs 
non-editable/tool view).
  
  Bottom line, I think we need to discuss what we really want here before we 
move forward with implementation anymore, to avoid wasting your time!

REPOSITORY
  R31 Breeze

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

To: abstractdevelop, #vdg, #breeze
Cc: manueljlin, filipf, ngraham, cblack, plasma-devel, dmenig, Orage, 
LeGast00n, The-Feren-OS-Dev, konkinartem, ian, jguidon, Ghost6, jraleigh, 
zachus, fbampaloukas, squeakypancakes, alexde, IohannesPetros, GB_2, 
trickyricky26, ragreen, crozbo, ndavis, ZrenBot, firef, skadinna, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, aaronhoneycutt, abetts, sebas, apol, 
ahiemstra, mbohlender, mart

D29264: New tabs for Breeze

[Abstract Software]

abstractdevelop updated this revision to Diff 81461.
abstractdevelop marked 2 inline comments as done.
abstractdevelop added a comment.


  Fixed all messages. Sorry for all the emails. :/

REPOSITORY
  R31 Breeze

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29264?vs=81460=81461

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

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

To: abstractdevelop, #vdg, #breeze
Cc: filipf, ngraham, cblack, plasma-devel, dmenig, manueljlin, Orage, 
LeGast00n, The-Feren-OS-Dev, konkinartem, ian, jguidon, Ghost6, jraleigh, 
zachus, fbampaloukas, squeakypancakes, alexde, IohannesPetros, GB_2, 
trickyricky26, ragreen, crozbo, ndavis, ZrenBot, firef, skadinna, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, aaronhoneycutt, abetts, sebas, apol, 
ahiemstra, mbohlender, mart

D29264: New tabs for Breeze

[Abstract Software]

abstractdevelop marked 5 inline comments as done.
abstractdevelop added a comment.


  If we want to do that, then the line will have to go on the bottom.

REPOSITORY
  R31 Breeze

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

To: abstractdevelop, #vdg, #breeze
Cc: filipf, ngraham, cblack, plasma-devel, dmenig, manueljlin, Orage, 
LeGast00n, The-Feren-OS-Dev, konkinartem, ian, jguidon, Ghost6, jraleigh, 
zachus, fbampaloukas, squeakypancakes, alexde, IohannesPetros, GB_2, 
trickyricky26, ragreen, crozbo, ndavis, ZrenBot, firef, skadinna, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, aaronhoneycutt, abetts, sebas, apol, 
ahiemstra, mbohlender, mart

D29264: New tabs for Breeze

[Filip Fila]

filipf added a comment.


  I think we still want to have the edges rounded if it's possible?

REPOSITORY
  R31 Breeze

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

To: abstractdevelop, #vdg, #breeze
Cc: filipf, ngraham, cblack, plasma-devel, dmenig, manueljlin, Orage, 
LeGast00n, The-Feren-OS-Dev, konkinartem, ian, jguidon, Ghost6, jraleigh, 
zachus, fbampaloukas, squeakypancakes, alexde, IohannesPetros, GB_2, 
trickyricky26, ragreen, crozbo, ndavis, ZrenBot, firef, skadinna, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, aaronhoneycutt, abetts, sebas, apol, 
ahiemstra, mbohlender, mart

D29264: New tabs for Breeze

[Abstract Software]

abstractdevelop updated this revision to Diff 81460.
abstractdevelop added a comment.


  Oof silly me.

REPOSITORY
  R31 Breeze

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29264?vs=81459=81460

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

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

To: abstractdevelop, #vdg, #breeze
Cc: ngraham, cblack, plasma-devel, dmenig, manueljlin, Orage, LeGast00n, 
The-Feren-OS-Dev, konkinartem, ian, jguidon, Ghost6, jraleigh, zachus, 
fbampaloukas, squeakypancakes, alexde, IohannesPetros, GB_2, trickyricky26, 
ragreen, crozbo, ndavis, ZrenBot, firef, skadinna, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, aaronhoneycutt, abetts, sebas, apol, ahiemstra, 
mbohlender, mart

D29264: New tabs for Breeze

[Abstract Software]

abstractdevelop updated this revision to Diff 81459.
abstractdevelop added a comment.


  Address comments.
  (Those were just some things I was playing around with, but everybody was 
right and they weren't necessary)

REPOSITORY
  R31 Breeze

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29264?vs=81454=81459

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

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

To: abstractdevelop, #vdg, #breeze
Cc: ngraham, cblack, plasma-devel, dmenig, manueljlin, Orage, LeGast00n, 
The-Feren-OS-Dev, konkinartem, ian, jguidon, Ghost6, jraleigh, zachus, 
fbampaloukas, squeakypancakes, alexde, IohannesPetros, GB_2, trickyricky26, 
ragreen, crozbo, ndavis, ZrenBot, firef, skadinna, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, aaronhoneycutt, abetts, sebas, apol, ahiemstra, 
mbohlender, mart

D29264: New tabs for Breeze

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> cblack wrote in breezestyle.cpp:5608
> mmm, not sure how I feel about this

Yeah we shouldn't be overriding this. `documentMode` is something that the 
widget itself can have set depending on the context. AFAIK the plan is to use 
`documentMode` to determine the style:

- `documentMode: true` means it's an "editable" tab (e.g. an open document in 
Okular, a view in Konsole, or a folder in Dolphin that should look distinctly 
tab-like, as with this new design, etc)

`documentMode: false` means it's a "non-editable" tab used to switch between 
tool views (e.g. the tabbed sidebars in Okular or Gwenview, a tabbed view in a 
settings window, etc)

REPOSITORY
  R31 Breeze

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

To: abstractdevelop, #vdg, #breeze
Cc: ngraham, cblack, plasma-devel, dmenig, manueljlin, Orage, LeGast00n, 
The-Feren-OS-Dev, konkinartem, ian, jguidon, Ghost6, jraleigh, zachus, 
fbampaloukas, squeakypancakes, alexde, IohannesPetros, GB_2, trickyricky26, 
ragreen, crozbo, ndavis, ZrenBot, firef, skadinna, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, aaronhoneycutt, abetts, sebas, apol, ahiemstra, 
mbohlender, mart

D29264: New tabs for Breeze

[Abstract Software]

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

REVISION SUMMARY
  This is a new tab style for breeze.
  This is my first patch to KDE, so tell me if I did anything wrong. ;)

TEST PLAN
  F8269180: NewTabStyle.png 
  
  This is a new style with a top line which should look good with both mutable 
and nonmutable tabs.

REPOSITORY
  R31 Breeze

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

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

To: abstractdevelop, ngraham, niccolove
Cc: plasma-devel, dmenig, manueljlin, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, konkinartem, ian, jguidon, Ghost6, jraleigh, zachus, fbampaloukas, 
squeakypancakes, alexde, IohannesPetros, GB_2, trickyricky26, ragreen, crozbo, 
ndavis, ZrenBot, firef, ngraham, skadinna, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, aaronhoneycutt, abetts, sebas, apol, ahiemstra, mbohlender, mart

D29264: New tabs for Breeze

[Carson Black]

cblack added inline comments.

INLINE COMMENTS

> breezehelper.cpp:1276
>  QRectF frameRect( rect );
> -qreal radius( frameRadius( PenWidth::NoPen, -1 ) );
> +qreal radius( 0 ); //frameRadius( PenWidth::NoPen, -1 ) );
>  

You should remove code rather than commenting it out

> breezestyle.cpp:5447
>  // render focus line
> -_helper->renderFocusLine( painter, textRect, focusColor );
> +//_helper->renderFocusLine( painter, textRect, focusColor );
>  

ditto

> breezestyle.cpp:5546
>  
> -rect.adjust( 0, 1, 0, 0 );
> +rect.adjust( 0, - 1, 0, 0 );
>  if( isFirst ) corners |= CornerBottomLeft;

Excess space, should be `-1`

> breezestyle.cpp:5608
>  const auto tabWidget = ( widget && widget->parentWidget() ) ? 
> qobject_cast( widget->parentWidget() ) : nullptr;
> -documentMode |= ( tabWidget ? tabWidget->documentMode() : true );
> +documentMode = true; //|= ( tabWidget ? 
> tabWidget->documentMode() : true );
>  

mmm, not sure how I feel about this

> breezestyle.cpp:5610
>  
> -color = 
> (documentMode&&!isQtQuickControl&&!hasAlteredBackground(widget)) ? 
> palette.color( QPalette::Window ) : _helper->frameBackgroundColor( palette );
> +color = 
> /*(documentMode&&!isQtQuickControl&&!hasAlteredBackground(widget)) ?*/ 
> palette.color( QPalette::Window ); //: _helper->frameBackgroundColor( palette 
> );
>  

or this

> breezestyle.cpp:5623
>  // outline
> -const auto outline( selected ? _helper->alphaColor( palette.color( 
> QPalette::WindowText ), 0.25 ) : QColor() );
> +const auto outline( true ? _helper->alphaColor( palette.color( 
> QPalette::WindowText ), 0.25 ) : QColor() );
> +

... or this

REPOSITORY
  R31 Breeze

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

To: abstractdevelop, #vdg, #breeze
Cc: cblack, plasma-devel, dmenig, manueljlin, Orage, LeGast00n, 
The-Feren-OS-Dev, konkinartem, ian, jguidon, Ghost6, jraleigh, zachus, 
fbampaloukas, squeakypancakes, alexde, IohannesPetros, GB_2, trickyricky26, 
ragreen, crozbo, ndavis, ZrenBot, firef, ngraham, skadinna, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, aaronhoneycutt, abetts, sebas, apol, 
ahiemstra, mbohlender, mart