D25814: [KColorScheme] Add SeparatorColor

[Noah Davis]

ndavis added a comment.


  How would we add the separator role to color scheme files if separator color 
was added to upstream Qt? Wouldn't we still need to add a separator role to 
KColorScheme so that we could map the color in the color scheme file to the 
equivalent QPalette ColorRole?

REPOSITORY
  R265 KConfigWidgets

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

To: ndavis, #frameworks, #vdg
Cc: ahiemstra, broulik, manueljlin, alexde, ngraham, davidedmundson, filipf, 
cfeck, hpereiradacosta, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

D25814: [KColorScheme] Add SeparatorColor

[Noah Davis]

ndavis abandoned this revision.
ndavis added a comment.


  Abandoning this since others would rather have this in QPalette

REPOSITORY
  R265 KConfigWidgets

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

To: ndavis, #frameworks, #vdg
Cc: ahiemstra, broulik, manueljlin, alexde, ngraham, davidedmundson, filipf, 
cfeck, hpereiradacosta, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

D25814: [KColorScheme] Add SeparatorColor

[Noah Davis]

ndavis added a comment.


  OK, I can agree with contributing upstream. I'll try talking on that bug 
report.

REPOSITORY
  R265 KConfigWidgets

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

To: ndavis, #frameworks, #vdg
Cc: ahiemstra, broulik, manueljlin, alexde, ngraham, davidedmundson, filipf, 
cfeck, hpereiradacosta, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

D25814: [KColorScheme] Add SeparatorColor

[David Edmundson]

davidedmundson added a comment.


  Relevant link on that last comment: 
https://bugreports.qt.io/browse/QTBUG-63331
  
  They actively seeked our opinion on colour roles

REPOSITORY
  R265 KConfigWidgets

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

To: ndavis, #frameworks, #vdg
Cc: ahiemstra, broulik, manueljlin, alexde, ngraham, davidedmundson, filipf, 
cfeck, hpereiradacosta, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

D25814: [KColorScheme] Add SeparatorColor

[Christoph Feck]

cfeck added a comment.


  There is QPalette::Button, but I don't see any hover/focus colors in QPalette.
  
  If we want more roles, we seriously need to contribute them upstream. Qt 6 is 
a chance to avoid diverging more.

REPOSITORY
  R265 KConfigWidgets

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

To: ndavis, #frameworks, #vdg
Cc: ahiemstra, broulik, manueljlin, alexde, ngraham, davidedmundson, filipf, 
cfeck, hpereiradacosta, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

D25814: [KColorScheme] Add SeparatorColor

[Arjen Hiemstra]

ahiemstra added a comment.


  I actually ran into this with the new Card designs I implemented in Kirigami. 
Kirigami right now does a lot of `tint(backgroundColor, textColor)` to generate 
separator colours. However, this leads to potential differences when the color 
set is View vs. Window or other cases, making separators looks slightly 
different based on where they are used. I don't think that's a good idea 
personally and in any case, having an explicit separator color would make it an 
explicit design decision instead of an implicit technical artifact. So +1 from 
my side for adding this.

REPOSITORY
  R265 KConfigWidgets

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

To: ndavis, #frameworks, #vdg
Cc: ahiemstra, broulik, manueljlin, alexde, ngraham, davidedmundson, filipf, 
cfeck, hpereiradacosta, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

D25814: [KColorScheme] Add SeparatorColor

[Nathaniel Graham]

ngraham added a comment.


  I have to agree with @ndavis here. Adding a separator color to the color 
scheme seems sensible enough to me, for the reasons previously provided.

REPOSITORY
  R265 KConfigWidgets

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

To: ndavis, #frameworks, #vdg
Cc: broulik, manueljlin, alexde, ngraham, davidedmundson, filipf, cfeck, 
hpereiradacosta, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

D25814: [KColorScheme] Add SeparatorColor

[Noah Davis]

ndavis added a comment.


  In D25814#598380 , @cfeck wrote:
  
  > > Why don't focus and hover colors belong?
  >
  > Because an application cannot know if and how a style does indicate focus 
or hovering.
  
  
  I don't understand this objection. What would allow an application to know 
about the button background from the QStyle? It could be a PNG, and yet setting 
the button background color is perfectly fine.

REPOSITORY
  R265 KConfigWidgets

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

To: ndavis, #frameworks, #vdg
Cc: broulik, manueljlin, alexde, ngraham, davidedmundson, filipf, cfeck, 
hpereiradacosta, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

D25814: [KColorScheme] Add SeparatorColor

[Christoph Feck]

cfeck added a comment.


  > Why don't focus and hover colors belong?
  
  Because an application cannot know if and how a style does indicate focus or 
hovering.

REPOSITORY
  R265 KConfigWidgets

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

To: ndavis, #frameworks, #vdg
Cc: broulik, manueljlin, alexde, ngraham, davidedmundson, filipf, cfeck, 
hpereiradacosta, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D25814: [KColorScheme] Add SeparatorColor

[Noah Davis]

ndavis added a comment.


  I think this is still ultimately the right thing to do. Making the 
frame/separator color a mix of 2 other colors makes it harder to get just the 
right color for both the light and dark variations of a theme unless you design 
the color palettes to solve problems in the QStyle. That makes it more 
difficult for users/3rd parties to design color schemes unless you're very 
familiar with the way the QStyle works.
  
  I missed this from before:
  
  In D25814#574152 , @cfeck wrote:
  
  > I also agree with the concerns rised by Hugo. An application will use 
various frame primitives, and the style decides how to draw them. It doesn't 
belong in the API, though (but neither do Focus and Hover colors).
  
  
  Why don't focus and hover colors belong?

REPOSITORY
  R265 KConfigWidgets

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

To: ndavis, #frameworks, #vdg
Cc: broulik, manueljlin, alexde, ngraham, davidedmundson, filipf, cfeck, 
hpereiradacosta, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D25814: [KColorScheme] Add SeparatorColor

[Noah Davis]

ndavis added a subscriber: broulik.
ndavis added a comment.


  In D25814#591053 , @ngraham wrote:
  
  > I wonder if a path forward is to move this option into Breeze itself and 
keep it out of the color scheme. We would add an option for "dark separators 
when using a dark color scheme" in the super hidden Breeze settings window.
  
  
  Hugo did suggest that. Although @broulik was just mentioning in the VDG chat 
that QML can't reuse the color because it's defined in Breeze and not part of 
the colorscheme.

REPOSITORY
  R265 KConfigWidgets

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

To: ndavis, #frameworks, #vdg
Cc: broulik, manueljlin, alexde, ngraham, davidedmundson, filipf, cfeck, 
hpereiradacosta, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D25814: [KColorScheme] Add SeparatorColor

[Nathaniel Graham]

ngraham added a comment.


  I wonder if a path forward is to move this option into Breeze itself and keep 
it out of the color scheme. We would add an option for "dark separators when 
using a dark color scheme" in the super hidden Breeze settings window.

REPOSITORY
  R265 KConfigWidgets

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

To: ndavis, #frameworks, #vdg
Cc: manueljlin, alexde, ngraham, davidedmundson, filipf, cfeck, 
hpereiradacosta, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D25814: [KColorScheme] Add SeparatorColor

[Noah Davis]

ndavis added a comment.


  In D25814#574395 , @filipf wrote:
  
  > Already asked in VDG, but repeating here: how would we handle dark 
separators on really dark color schemes?
  >
  > This is the most downloaded color scheme and it won't work well with dark 
separators: https://store.kde.org/p/1294011/
  
  
  Are you asking in the case that we don't have a colorscheme color for 
separators or in the case that we do?
  Without: Either use special logic to use light separators when the background 
gets too dark or just allow the separators to be nearly invisible
  With: The colorscheme specifies an appropriate separator color or use Window 
Background 75%, Window Text 25% as the fallback color.

REPOSITORY
  R265 KConfigWidgets

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

To: ndavis, #frameworks, #vdg
Cc: alexde, ngraham, davidedmundson, filipf, cfeck, hpereiradacosta, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D25814: [KColorScheme] Add SeparatorColor

[Filip Fila]

filipf added a comment.


  In D25814#574389 , @ngraham wrote:
  
  > In D25814#574380 , @ndavis wrote:
  >
  > > FWIW, Adwaita Dark uses dark separators, but GNOME doesn't use nearly as 
many icons as we do. F7806761: Screenshot_20191209_155912.png 

  >
  >
  > To me, this image really shows how good the dark separators look. I don't 
think the fact that our UIs are more icon-heavy is a significant factor here. 
Unlike GNOME, we tend to use ToolButtons, which have no outline until hovered 
over. So in fact there are probably fewer lines with the separator color in our 
Toolbutton-heavy UIs compared to GNOME.
  
  
  Already asked in VDG, but repeating here: how would we handle dark separators 
on really dark color schemes?
  
  This is the most downloaded color scheme and it won't work well with dark 
separators: https://store.kde.org/p/1294011/

REPOSITORY
  R265 KConfigWidgets

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

To: ndavis, #frameworks, #vdg
Cc: ngraham, davidedmundson, filipf, cfeck, hpereiradacosta, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D25814: [KColorScheme] Add SeparatorColor

[Nathaniel Graham]

ngraham added a comment.


  In D25814#574380 , @ndavis wrote:
  
  > FWIW, Adwaita Dark uses dark separators, but GNOME doesn't use nearly as 
many icons as we do. F7806761: Screenshot_20191209_155912.png 

  
  
  To me, this image really shows how good the dark separators look. I don't 
think the fact that our UIs are more icon-heavy is a significant factor here. 
Unlike GNOME, we tend to use ToolButtons, which have no outline until hovered 
over. So in fact there are probably fewer lines with the separator color in our 
Toolbutton-heavy UIs compared to GNOME.

REPOSITORY
  R265 KConfigWidgets

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

To: ndavis, #frameworks, #vdg
Cc: ngraham, davidedmundson, filipf, cfeck, hpereiradacosta, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D25814: [KColorScheme] Add SeparatorColor

[Noah Davis]

ndavis added a comment.


  In D25814#574356 , 
@hpereiradacosta wrote:
  
  > I meant: do the "light" (with window text color) toolbar icons play well 
with dark toolbar separators as opposed to current semi-light toolbar separators
  
  
  I assume you mean visually play well. It could, but it depends on other 
factors like consistency with the rest of the theme and other types of styling. 
Fusion uses dark toolbar separators (although they are lighter on the right 
side) and it doesn't stand out as ugly, but they're not quite my taste and a 
bit hard to see. F7806755: Screenshot_20191209_155237.png 

  
  FWIW, Adwaita Dark uses dark separators, but GNOME doesn't use nearly as many 
icons as we do. F7806761: Screenshot_20191209_155912.png 
 F7806768: Screenshot_20191209_160349.png 

  
  I see icons as similar to text in how other UI elements should be designed 
around them.

REPOSITORY
  R265 KConfigWidgets

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

To: ndavis, #frameworks, #vdg
Cc: ngraham, davidedmundson, filipf, cfeck, hpereiradacosta, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D25814: [KColorScheme] Add SeparatorColor

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  
  
  > I'm not sure what you mean when you wonder how it would play with 
monochrome icons. It should have nothing to do with them unless you embed 
stylesheets in the SVGs to use the separator color in breeze icons.
  
  I meant: do the "light" (with window text color) toolbar icons play well with 
dark toolbar separators as opposed to current semi-light toolbar separators
  
  > 
  > 
  >> - in breeze and in almost all widget themes, button frames are not the 
same color are ordinary frames. The former uses ButtonText and 
ButtonBackground, the later uses WindowText and WindowBackground. By replacing 
this by one single color, one would effectively break all the color schemes 
that use different colors for these two sets.
  > 
  > Decoration colors can be customized individually for each color set, so 
button frames can use Button SeparatorColor.
  
  Fair point: I forgot you can specify the three different color flavors 
separately. Still: that makes three colors to change to make one happy with 
dark "separators"
  
  > Some other thoughts:
  > 
  > - Complementary SeparatorColor could be used for dark separators when a 
theme uses both light and dark separators or frames.
  > - I suppose I could use AlternateBackground or InactiveText instead of 
SeparatorColor, but that seems semantically incorrect.
  
  Agreed

REPOSITORY
  R265 KConfigWidgets

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

To: ndavis, #frameworks, #vdg
Cc: ngraham, davidedmundson, filipf, cfeck, hpereiradacosta, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D25814: [KColorScheme] Add SeparatorColor

[Noah Davis]

ndavis added a comment.


  In D25814#574250 , 
@hpereiradacosta wrote:
  
  > - You should add toolbar separators, tabboxes and group boxes to that. For 
toolbar separators, I wonder how this would play with monochrome (color-themed) 
icons.
  
  
  I'm not sure what you mean when you wonder how it would play with monochrome 
icons. It should have nothing to do with them unless you embed stylesheets in 
the SVGs to use the separator color in breeze icons.
  
  > - Note that for frames, they come in three flavor (raised, sunken or flat) 
and not all widget themes ignore this additional setting the way breeze does 
(design decision). Oxygen does not.
  
  That's a good point and I hadn't considered it. I suppose whether or not 
SeparatorColor is used for frames would be left up to the theme, but that does 
hurt where this color can be applied.
  
  > - in breeze and in almost all widget themes, button frames are not the same 
color are ordinary frames. The former uses ButtonText and ButtonBackground, the 
later uses WindowText and WindowBackground. By replacing this by one single 
color, one would effectively break all the color schemes that use different 
colors for these two sets.
  
  Decoration colors can be customized individually for each color set, so 
button frames can use Button SeparatorColor.
  
  Some other thoughts:
  
  - Complementary SeparatorColor could be used for dark separators when a theme 
uses both light and dark separators or frames.
  - I suppose I could use AlternateBackground or InactiveText instead of 
SeparatorColor, but that seems semantically incorrect.

REPOSITORY
  R265 KConfigWidgets

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

To: ndavis, #frameworks, #vdg
Cc: ngraham, davidedmundson, filipf, cfeck, hpereiradacosta, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D25814: [KColorScheme] Add SeparatorColor

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  In D25814#574197 , @ndavis wrote:
  
  > In D25814#574160 , 
@davidedmundson wrote:
  >
  > > Can we clarify what separators we're referring to.
  >
  >
  > Frames around UI elements and horizontal/vertical lines that separate UI 
elements.
  >  These are the most common examples, but probably not absolutely 
everything: F7806225: Screenshot_20191209_070836-1.png 

  
  
  
  
  - You should add toolbar separators, tabboxes and group boxes to that. For 
toolbar separators, I wonder how this would play with monochrome (color-themed) 
icons.
  - Note that for frames, they come in three flavor (raised, sunken or flat) 
and not all widget themes ignore this additional setting the way breeze does 
(design decision). Oxygen does not.
  - as discuss, oxygen does not fit in this picture: most of the frames 
mentioned here are rendered using shadows only. (but then if there is no plan 
to implement this in oxygen this is not a problem)
  - in breeze and in almost all widget themes, button frames are not the same 
color are ordinary frames. The former uses ButtonText and ButtonBackground, the 
later uses WindowText and WindowBackground. By replacing this by one single 
color, one would effectively break all the color schemes that use different 
colors for these two sets.

REPOSITORY
  R265 KConfigWidgets

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

To: ndavis, #frameworks, #vdg
Cc: ngraham, davidedmundson, filipf, cfeck, hpereiradacosta, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D25814: [KColorScheme] Add SeparatorColor

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  
  
  > So the background here is that we've gotten a new VDG designer who produced 
mockups of Breeze dark with dark separators, and some people absolutely fell in 
love with them, while other people hated them. We could not achieve consensus 
on moving to use dark separators for Breeze Dark, so there was a desire to 
offer people that choice. It occurs to me that  we could put this in the Breeze 
style's own settings, if it's strictly necessary to expose this to users. 
Personally I would prefer to just make a decision on separator colors one way 
or the other rather than making it explicitly configurable (dark separators FTW 
:) ).
  
  Full +1 on this ! (does not happen so often). I really think one should just 
make a decision here, knowing that some people will not be happy about it. 
There will always be vocal unhappy people no matter what you do. Adding options 
to avoid that is not the right way to go. This literally is a bikeshed 
discussion.

REPOSITORY
  R265 KConfigWidgets

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

To: ndavis, #frameworks, #vdg
Cc: ngraham, davidedmundson, filipf, cfeck, hpereiradacosta, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D25814: [KColorScheme] Add SeparatorColor

[Nathaniel Graham]

ngraham added a comment.


  In D25814#574197 , @ndavis wrote:
  
  > In D25814#574160 , 
@davidedmundson wrote:
  >
  > > Can we clarify what separators we're referring to.
  >
  >
  > Frames around UI elements and horizontal/vertical lines that separate UI 
elements.
  >  These are the most common examples, but probably not absolutely 
everything: F7806225: Screenshot_20191209_070836-1.png 

  
  
  So the background here is that we've gotten a new VDG designer who produced 
mockups of Breeze dark with dark separators, and some people absolutely fell in 
love with them, while other people hated them. We could not achieve consensus 
on moving to use dark separators for Breeze Dark, so there was a desire to 
offer people that choice. It occurs to me that  we could put this in the Breeze 
style's own settings, if it's strictly necessary to expose this to users. 
Personally I would prefer to just make a decision on separator colors one way 
or the other rather than making it explicitly configurable (dark separators FTW 
:) ).

REPOSITORY
  R265 KConfigWidgets

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

To: ndavis, #frameworks, #vdg
Cc: ngraham, davidedmundson, filipf, cfeck, hpereiradacosta, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D25814: [KColorScheme] Add SeparatorColor

[Noah Davis]

ndavis added a comment.


  In D25814#574129 , 
@hpereiradacosta wrote:
  
  > Few more comments on this:
  >
  > - general: you will never be able to make all the opiniated people happy, 
and you have to draw a line (otherwise your code will become bloated, buggy, 
and unmaintainable)
  > - regarding this specific case: many widget style will not implement this 
new color. For those this will just generate bugs reports: why is my color 
scheme not respectd ?
  > - some widget styles (oxygen at least, but I'm sure there are others) use 
two colors for frames and separators, to mimic shadows or relief effects. 
Adding one single color for this will not fit such schemes.
  > - in the end if you need an extra color for a given theme (be it 
future-breeze or whatever), there is also the possibility to add it as a extra 
option for this specific theme, rather than forcing it to kcolorscheme and 
imposing it to all styles (or making kcolorscheme broken for all the styles 
that wont use it). This is how window decoration shadows and glow were handled 
to oxygen. I think this change will break more things than it will fix, 
especially if the fix is to make some opinionated people happy.
  
  
  
  
  - I know it's not a great reason, but I want the setting too. I'm not only 
making it for others.
  - The most popular 3rd party themes (Kvantum themes) don't even support 
colorschemes. For Kvantum users, colorschemes are only useful for setting the 
colors on QML apps, which Kvantum doesn't properly set the colors for. AFAIK, 
the only other commonly used QStyles are Breeze, Oxygen and QStyles provided by 
Qt. Qt's QStyles only uses colors that map to QPalette colors. The only 
customizable colors which don't map to QPalette that Oxygen and Breeze use are 
View FocusColor, View HoverColor and View NegativeText. Oxygen also uses 
KColorScheme's shade colors. If KColorScheme is only used by us, what is is the 
point of having KColorScheme if we barely use it and we cannot extend it for 
use with our own software?
  - I don't intend to change Oxygen.
  - The VDG has been trying to make it easier to navigate through SySe, so I 
want to avoid putting more settings in poorly visible locations. I also want 
access to this color outside of the Breeze widget style so that it can be used 
in Plasmashell and QML apps. I suppose I wouldn't have to make this 
customizable for users to do the last part.
  - I'm not sure what you mean by "break". Do you mean themes that previously 
worked will stop working?

REPOSITORY
  R265 KConfigWidgets

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

To: ndavis, #frameworks, #vdg
Cc: davidedmundson, filipf, cfeck, hpereiradacosta, kde-frameworks-devel, 
LeGast00n, GB_2, michaelh, ngraham, bruns

D25814: [KColorScheme] Add SeparatorColor

[Noah Davis]

ndavis added a comment.


  In D25814#574160 , @davidedmundson 
wrote:
  
  > Can we clarify what separators we're referring to.
  
  
  Frames around UI elements and horizontal/vertical lines that separate UI 
elements.
  These are the most common examples, but probably not absolutely everything: 
F7806225: Screenshot_20191209_070836-1.png 


REPOSITORY
  R265 KConfigWidgets

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

To: ndavis, #frameworks, #vdg
Cc: davidedmundson, filipf, cfeck, hpereiradacosta, kde-frameworks-devel, 
LeGast00n, GB_2, michaelh, ngraham, bruns

D25814: [KColorScheme] Add SeparatorColor

[David Edmundson]

davidedmundson added a comment.


  Can we clarify what separators we're referring to.

REPOSITORY
  R265 KConfigWidgets

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

To: ndavis, #frameworks, #vdg
Cc: davidedmundson, filipf, cfeck, hpereiradacosta, kde-frameworks-devel, 
LeGast00n, GB_2, michaelh, ngraham, bruns

D25814: [KColorScheme] Add SeparatorColor

[Christoph Feck]

cfeck added a comment.


  I also agree with the concerns rised by Hugo. An application will use various 
frame primitives, and the style decides how to draw them. It doesn't belong in 
the API, though (but neither do Focus and Hover colors).

REPOSITORY
  R265 KConfigWidgets

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

To: ndavis, #frameworks, #vdg
Cc: filipf, cfeck, hpereiradacosta, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D25814: [KColorScheme] Add SeparatorColor

[David Faure]

dfaure resigned from this revision.
dfaure added a comment.
This revision now requires review to proceed.


  Hugo's arguments make sense, removing my approval.

REPOSITORY
  R265 KConfigWidgets

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

To: ndavis, #frameworks, #vdg
Cc: filipf, cfeck, hpereiradacosta, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D25814: [KColorScheme] Add SeparatorColor

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  Few more comments on this:
  
  - general: you will never be able to make all the opiniated people happy, and 
you have to draw a line (otherwise your code will become bloated, buggy, and 
unmaintainable)
  - regarding this specific case: many widget style will not implement this new 
color. For those this will just generate bugs reports: why is my color scheme 
not respectd ?
  - some widget styles (oxygen at least, but I'm sure there are others) use two 
colors for frames and separators, to mimic shadows or relief effects. Adding 
one single color for this will not fit such schemes.
  - in the end if you need an extra color for a given theme (be it 
future-breeze or whatever), there is also the possibility to add it as a extra 
option for this specific theme, rather than forcing it to kcolorscheme and 
imposing it to all styles (or making kcolorscheme broken for all the styles 
that wont use it).
  
  This is how window decoration shadows and glow were handled to oxygen. 
  I think this change will break more things than it will fix, especially if 
the fix is to make some opinionated people happy.

REPOSITORY
  R265 KConfigWidgets

BRANCH
  separator-color (branched from master)

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

To: ndavis, #frameworks, #vdg, dfaure
Cc: filipf, cfeck, hpereiradacosta, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D25814: [KColorScheme] Add SeparatorColor

[Filip Fila]

filipf added a comment.


  I think this is useful. Every once in a while we get opinionated people who 
think light separators are awful in the Breeze Dark scheme (I would hate dark 
ones on the other hand) or even some ricer people who want to turn off 
separators (which they could do now by matching it with window color I guess).
  
  Implementation-wise there definitely always needs to be a fallback value so 
we don't break third-party color-schemes.

REPOSITORY
  R265 KConfigWidgets

BRANCH
  separator-color (branched from master)

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

To: ndavis, #frameworks, #vdg, dfaure
Cc: filipf, cfeck, hpereiradacosta, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D25814: [KColorScheme] Add SeparatorColor

[Noah Davis]

ndavis added a comment.


  @hpereiradacosta, Fair points and I'm glad you spoke up. JFYI, I'm in no rush 
to land this and I will consider reserving this change for KF6 if experienced 
KDE devs think that is best.
  
  In D25814#574105 , 
@hpereiradacosta wrote:
  
  > Adding new entries to the kcolorscheme should be done with a lot of care, 
because it could be seen as some sort of API break for existing colorscheme, as 
soon as you start using this color in the widget style: you would need a 
fallback implementation, for all the colorscheme in the open which do not 
implement this particular color. These kind of additions happened a lot whith 
gtk3 css style and resulting of overall unhappiness and distrust from 
theme/color scheme developpers. This also increases the complexity of the code 
and difficulty to maintain.
  
  
  In terms of breakage, I think it would be fine if I made the color fallback 
(as you said) to what the breeze widget style currently does. 3rd party widget 
styles wouldn't use this color without modification, so it shouldn't hurt them. 
KColorScheme already has default colors, but they're defined before 
KColorScheme tries to read colors from the colorscheme file instead of when a 
color is not found. That hurts the flexibility of these defaults and seems 
backwards to me, but there may be a reason for it that I don't know of.
  
  > Also I am not convinced about the usefullness of this option either, 
especially considering that tweaking the colorscheme in the systemsettings is 
already quite advanced configuration (I would bet that most users stop at 
switching between pre-made color schemes, rather than tweaking a particular 
one. This will get even less the case if you start implementing more corner 
case colors.
  
  Yes, it is yet another option in a sea of options, many of which are 
completely useless when a user ventures outside of the Common Colors section. 
You're probably correct that most users don't even bother with manually 
adjusting colors since choosing good colors can be tricky. I don't think that 
makes this option useless though, except for the fact that it's repeated for 
each color set and only a few versions of it will get any amount of use. I hope 
to simplify the colorscheme editor in the future so that only options that do 
something are exposed.
  
  > Or do you plan to ship a "breeze dark with dark separators" +and+ a "breeze 
dark with light separators" ?
  
  I do not.
  
  > All in all i would say that people (that is: active developpers and the 
very few that manifest themselves on developper/vdg channels) having different 
opinion is not a strong enough argument to expose such a low level tweaking to 
all. Or is there an actual poll that shows (on a larger data sample) that one 
really cannot decide between the two options ?
  
  There is no such poll.

REPOSITORY
  R265 KConfigWidgets

BRANCH
  separator-color (branched from master)

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

To: ndavis, #frameworks, #vdg, dfaure
Cc: cfeck, hpereiradacosta, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D25814: [KColorScheme] Add SeparatorColor

[Hugo Pereira Da Costa]

hpereiradacosta added a subscriber: cfeck.
hpereiradacosta added a comment.


  Would also be good to have the opinion of @cfeck on this.

REPOSITORY
  R265 KConfigWidgets

BRANCH
  separator-color (branched from master)

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

To: ndavis, #frameworks, #vdg, dfaure
Cc: cfeck, hpereiradacosta, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D25814: [KColorScheme] Add SeparatorColor

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  Adding new entries to the kcolorscheme should be done with a lot of care, 
because it could be seen as some sort of API break for existing colorscheme, as 
soon as you start using this color in the widget style: you would need a 
fallback implementation, for all the colorscheme in the open which do not 
implement this particular color. These kind of additions happened a lot whith 
gtk3 css style and resulting of overall unhappiness and distrust from 
theme/color scheme developpers. This also increases the complexity of the code 
and difficulty to maintain.
  
  Also I am not convinced about the usefullness of this option either, 
especially considering that tweaking the colorscheme in the systemsettings is 
already quite advanced configuration (I would bet that most users stop at 
switching between pre-made color schemes, rather than tweaking a particular 
one. This will get even less the case if you start implementing more corner 
case colors. 
  Or do you plan to ship a "breeze dark with dark separators" +and+ a "breeze 
dark with light separators" ? 
  All in all i would say that people (that is: active developpers and the very 
few that manifest themselves on developper/vdg channels) having different 
opinion is not a strong enough argument to expose such a low level tweaking to 
all. Or is there an actual poll that shows (on a larger data sample) that one 
really cannot decide between the two options ?
  
  My two cents.
  
  Hugo

REPOSITORY
  R265 KConfigWidgets

BRANCH
  separator-color (branched from master)

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

To: ndavis, #frameworks, #vdg, dfaure
Cc: hpereiradacosta, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D25814: [KColorScheme] Add SeparatorColor

[David Faure]

dfaure accepted this revision.
dfaure added a comment.
This revision is now accepted and ready to land.


  Code looks OK. No opinion on usefulness though.

REPOSITORY
  R265 KConfigWidgets

BRANCH
  separator-color (branched from master)

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

To: ndavis, #frameworks, #vdg, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25814: [KColorScheme] Add SeparatorColor

[Noah Davis]

ndavis updated this revision to Diff 71096.
ndavis added a comment.


  - update @since version

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25814?vs=71095=71096

BRANCH
  separator-color (branched from master)

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

AFFECTED FILES
  src/kcolorscheme.cpp
  src/kcolorscheme.h

To: ndavis, #frameworks, #vdg, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25814: [KColorScheme] Add SeparatorColor

[Noah Davis]

ndavis edited the summary of this revision.

REPOSITORY
  R265 KConfigWidgets

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

To: ndavis, #frameworks, #vdg, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25814: [KColorScheme] Add SeparatorColor

[Noah Davis]

ndavis edited the summary of this revision.

REPOSITORY
  R265 KConfigWidgets

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

To: ndavis, #frameworks, #vdg, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25814: [KColorScheme] Add SeparatorColor

[Noah Davis]

ndavis edited the summary of this revision.

REPOSITORY
  R265 KConfigWidgets

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

To: ndavis, #frameworks, #vdg, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25814: [KColorScheme] Add SeparatorColor

[Noah Davis]

ndavis edited the summary of this revision.
ndavis added reviewers: Frameworks, VDG, dfaure.

REPOSITORY
  R265 KConfigWidgets

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

To: ndavis, #frameworks, #vdg, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25814: [KColorScheme] Add SeparatorColor

[Noah Davis]

ndavis created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
ndavis requested review of this revision.

REVISION SUMMARY
  Users sometimes ask for the ability to customize the color of separators.
  When talking about how the next version of Breeze Dark should look, some 
developers

REPOSITORY
  R265 KConfigWidgets

BRANCH
  separator-color (branched from master)

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

AFFECTED FILES
  src/kcolorscheme.cpp
  src/kcolorscheme.h

To: ndavis
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns