D12278: WIP: [Colors KCM] Port to new design

[Nathaniel Graham]

ngraham added a comment.


  In D12278#384981 , @GB_2 wrote:
  
  > > This removes the "Apply to non-Qt applications checkbox" which isn't 
something one should uncheck.
  >
  > I disagree. I even think it should be disabled by default, because it 
causes issues in GTK 2 apps such as this: https://i.stack.imgur.com/jpeYm.jpg
  >  I think it is not needed, because the GTK 2 theme already defines which 
colors are used and that should not be overwritten because of the issues.
  
  
  If we remove the checkbox for this, then it needs to be off by default so 
people stop hitting https://bugs.kde.org/show_bug.cgi?id=355540. If we remove 
the checkbox and don't include a way to turn off the setting, then there is no 
way to recover from that bug.
  
  Alternatively I guess we could fix the bug. :)

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, #vdg
Cc: GB_2, nicolasfella, mart, abetts, ngraham, davidedmundson, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D12278: WIP: [Colors KCM] Port to new design

[Björn Feber]

GB_2 added a comment.


  > This removes the "Apply to non-Qt applications checkbox" which isn't 
something one should uncheck.
  
  I disagree. I even think it should be disabled by default, because it causes 
issues in GTK 2 apps such as this: https://i.stack.imgur.com/jpeYm.jpg
  I think it is not needed, because the GTK 2 theme already defines which 
colors are used and that should not be overwritten because of the issues.

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, #vdg
Cc: GB_2, nicolasfella, mart, abetts, ngraham, davidedmundson, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D12278: WIP: [Colors KCM] Port to new design

[Nathaniel Graham]

ngraham added a comment.


  Any word on this?

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, #vdg
Cc: nicolasfella, mart, abetts, ngraham, davidedmundson, plasma-devel, GB_2, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D12278: WIP: [Colors KCM] Port to new design

[Nicolas Fella]

nicolasfella added inline comments.

INLINE COMMENTS

> colors.cpp:313
> +QStringList colorItemList;
> +colorItemList << "BackgroundNormal"
> +  << "BackgroundAlternate"

Can you use an initializer_list here and elsewhere?
Also QStringLiteral?

See https://www.angrycane.com.br/en/2018/06/19/speeding-up-cornercases/ for a 
comparison

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, #vdg
Cc: nicolasfella, mart, abetts, ngraham, davidedmundson, plasma-devel, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D12278: WIP: [Colors KCM] Port to new design

[Andres Betts]

abetts added a comment.


  Ping? Is this KCM doing alright?

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, #vdg
Cc: mart, abetts, ngraham, davidedmundson, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D12278: WIP: [Colors KCM] Port to new design

[Marco Martin]

mart added a comment.


  ok,  will work on the widget tinting on next weeks then :)

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, #vdg
Cc: mart, abetts, ngraham, davidedmundson, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D12278: WIP: [Colors KCM] Port to new design

[Kai Uwe Broulik]

broulik added a comment.


  In D12278#257148 , @mart wrote:
  
  > what about having this with just color boxes on 5.13 and then figure out 
controls tinting in 5.14?
  
  
  I would prefer not to rush things and have something half-baked, I still need 
to figure out the editor and proper theme applying as well as custom color 
scheme stuff. The KCM has been like this for two decades, it won't hurt if it 
stays another couple of months imho

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, #vdg
Cc: mart, abetts, ngraham, davidedmundson, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D12278: WIP: [Colors KCM] Port to new design

[Marco Martin]

mart added a comment.


  what about having this with just color boxes on 5.13 and then figure out 
controls tinting in 5.14?

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, #vdg
Cc: mart, abetts, ngraham, davidedmundson, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D12278: WIP: [Colors KCM] Port to new design

[Nathaniel Graham]

ngraham added a comment.


  Beautiful! Could we move the pushbuttons over like 2 pixels to the left so 
they're vertically aligned with the labels above them?

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, #vdg
Cc: mart, abetts, ngraham, davidedmundson, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D12278: WIP: [Colors KCM] Port to new design

[Kai Uwe Broulik]

broulik added a comment.


  This is what it will most likely look like once the tinting of controls has 
been figured out
  F5819517: Screenshot_20180423_185432.png 


REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, #vdg
Cc: mart, abetts, ngraham, davidedmundson, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D12278: WIP: [Colors KCM] Port to new design

[Andres Betts]

abetts added a comment.


  In D12278#252314 , @ngraham wrote:
  
  > Hmm, I'm not sure those white strips communicate "This is what text looks 
like." I think some sort of placeholder text would work better. To be honest I 
kinda liked the first one better, but I think it's mostly moot until we can 
show a real window or view preview.
  
  
  I am all for creating a concept that can do that. We make a sample window 
with the colors of each. Would it be too hard? @broulik

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, #vdg
Cc: mart, abetts, ngraham, davidedmundson, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D12278: WIP: [Colors KCM] Port to new design

[Nathaniel Graham]

ngraham added a comment.


  Hmm, I'm not sure those white strips communicate "This is what text looks 
like." I think some sort of placeholder text would work better. To be honest I 
kinda liked the first one better, but I think it's mostly moot until we can 
show a real window or view preview.

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, #vdg
Cc: mart, abetts, ngraham, davidedmundson, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D12278: WIP: [Colors KCM] Port to new design

[Kai Uwe Broulik]

broulik updated this revision to Diff 32881.
broulik edited the summary of this revision.
broulik edited the test plan for this revision.
broulik added a comment.


  - Implement applying color schemes and notifying applications about that
  - Implement "Install from file" including drag and drop
  - Implement removing color schemes including undo feature
  - Remove "apply to non qt applications" checkbox
  - Somewhat improve thumbnail but it's still not ideal

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12278?vs=32376=32881

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

AFFECTED FILES
  kcms/colors/CMakeLists.txt
  kcms/colors/Messages.sh
  kcms/colors/colors.cpp
  kcms/colors/colors.desktop
  kcms/colors/colors.h
  kcms/colors/colorscm.cpp
  kcms/colors/kcm_colors.desktop
  kcms/colors/package/contents/ui/main.qml
  kcms/colors/package/metadata.desktop

To: broulik, #plasma, #vdg
Cc: mart, abetts, ngraham, davidedmundson, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D12278: WIP: [Colors KCM] Port to new design

[Andres Betts]

abetts added a comment.


  In D12278#251722 , @ngraham wrote:
  
  > In D12278#251602 , @broulik 
wrote:
  >
  > > > I also don't like "Default" either. What does that even mean to a 
regular user?
  > >
  > > That basically reloads the "no config" case hardcoded in KColorScheme. I 
think we should drop that and then make sure the "Default" button works well 
(as in: select Breeze).
  >
  >
  > Agreed. Where does that leave the "default" color scheme in KColorscheme 
though? Is it still needed? If so, I'll need a review for D12392: Fix the 
"Default" color scheme to match Breeze again 
 :)
  
  
  Should I be working on mockups to address comments? One was about not using a 
color palette.

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, #vdg
Cc: mart, abetts, ngraham, davidedmundson, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D12278: WIP: [Colors KCM] Port to new design

[Nathaniel Graham]

ngraham added a comment.


  In D12278#251602 , @broulik wrote:
  
  > > I also don't like "Default" either. What does that even mean to a regular 
user?
  >
  > That basically reloads the "no config" case hardcoded in KColorScheme. I 
think we should drop that and then make sure the "Default" button works well 
(as in: select Breeze).
  
  
  Agreed. Where does that leave the "default" color scheme in KColorscheme 
though? Is it still needed? If so, I'll need a review for D12392: Fix the 
"Default" color scheme to match Breeze again 
 :)

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, #vdg
Cc: mart, abetts, ngraham, davidedmundson, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D12278: WIP: [Colors KCM] Port to new design

[Kai Uwe Broulik]

broulik added a comment.


  > I also don't like "Default" either. What does that even mean to a regular 
user?
  
  That basically reloads the "no config" case hardcoded in KColorScheme. I 
think we should drop that and then make sure the "Default" button works well 
(as in: select Breeze).

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, #vdg
Cc: mart, abetts, ngraham, davidedmundson, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D12278: WIP: [Colors KCM] Port to new design

[Nathaniel Graham]

ngraham added a comment.


  In D12278#251210 , @mart wrote:
  
  > In D12278#248160 , @ngraham 
wrote:
  >
  > > It would be really cool if the delegates would show a little preview of a 
window actually using these colors, instead of the current grid of colored 
squares.
  >
  >
  > it's probably not trivial with how colors are done in qqc2 right now.. i 
plan to add more custom colors for kirigami, but non essential for now?
  
  
  Yes, definitely non-essential. A nice-to-have, not a need-to-have.

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, #vdg
Cc: mart, abetts, ngraham, davidedmundson, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D12278: WIP: [Colors KCM] Port to new design

[Nathaniel Graham]

ngraham added a comment.


  -1 for "current" here too.
  
  I also don't like "Default" either. What does that even mean to a regular 
user?

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, #vdg
Cc: mart, abetts, ngraham, davidedmundson, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D12278: WIP: [Colors KCM] Port to new design

[Marco Martin]

mart added a comment.


  In D12278#248160 , @ngraham wrote:
  
  > It would be really cool if the delegates would show a little preview of a 
window actually using these colors, instead of the current grid of colored 
squares.
  
  
  it's probably not trivial with how colors are done in qqc2 right now.. i plan 
to add more custom colors for kirigami, but non essential for now?

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, #vdg
Cc: mart, abetts, ngraham, davidedmundson, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D12278: WIP: [Colors KCM] Port to new design

[Marco Martin]

mart added a comment.


  In D12278#248109 , @davidedmundson 
wrote:
  
  > > Not too fond of this "Current"
  >
  > me neither, if it was up to me, I'd just show a prompt asking for a name 
when you click "edit" on a system scheme. Would be nice and simple.
  >
  > but it was redone by Marco quite recently and still kept. So clearly for a 
reason?
  
  
  it was like that, i redid the thing after enough user complaints, and yeah, i 
don't like current either

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, #vdg
Cc: mart, abetts, ngraham, davidedmundson, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol

D12278: WIP: [Colors KCM] Port to new design

[Andres Betts]

abetts added a comment.


  Could it be possible, just to agree with the team, to have a small preview of 
a sample window showing the colors? Something like this:
  
  https://macgui.com/downloads/?cat_id=10

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, #vdg
Cc: abetts, ngraham, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D12278: WIP: [Colors KCM] Port to new design

[Nathaniel Graham]

ngraham added a comment.


  > Make the "apply to non Qt apps" checkbox work (no idea why anyone would 
want to uncheck this, though)
  
  Does this setting even still work? I believe support for it is spotty to 
nonexistent in today's GTK3 apps.
  
  I would be in favor of removing the control from this page and turning the 
setting either on  or off by default, and relocating it to somewhere more 
hidden if necessary.

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, #vdg
Cc: ngraham, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12278: WIP: [Colors KCM] Port to new design

[Nathaniel Graham]

ngraham added a comment.


  It would be really cool if the delegates would show a little preview of a 
window actually using these colors, instead of the current grid of colored 
squares.

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, #vdg
Cc: ngraham, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12278: WIP: [Colors KCM] Port to new design

[David Edmundson]

davidedmundson added a comment.


  > Not too fond of this "Current"

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, #vdg
Cc: davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12278: WIP: [Colors KCM] Port to new design

[Kai Uwe Broulik]

broulik created this revision.
broulik added reviewers: Plasma, VDG.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.
broulik requested review of this revision.

REVISION SUMMARY
  Pretty much WIP
  What works:
  
  - Grid layout with crappy thumbnail
- The screenshots just show a quick and dirty attempt to mimic the old tiny 
icons we used to have. Perhaps either it should show *all* available colors in 
the theme but that might turn into a pixel mudd? or instead show a live preview 
of some controls, maybe a "fake window" in there?
  - Downloading color themes through GHNS

TEST PLAN
  Implements T7243 
  
  Not too fond of this "Current" (carried over from the old code). Perhaps we 
could do some magic to only show it when the current theme is not in the list 
or is a modified version of a theme? That would make the dialog less confusing 
as to what "Current" means.
  
  F5810572: Screenshot_20180417_132441.png 

  TODO:
  
  - Make applying theme work (write config and signal applications)
  - Make restore defaults work
  - Make "Install from file" work
  - Make edit work (the edit dialog is quite entangled with the code, I thought 
it was completely standalone nowadays)
  - Make removing theme work
  - Make the "apply to non Qt apps" checkbox work (no idea why anyone would 
want to uncheck this, though)

REPOSITORY
  R119 Plasma Desktop

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

AFFECTED FILES
  kcms/colors/CMakeLists.txt
  kcms/colors/colors.cpp
  kcms/colors/colors.desktop
  kcms/colors/colors.h
  kcms/colors/colorscm.cpp
  kcms/colors/kcm_colors.desktop
  kcms/colors/package/contents/ui/main.qml
  kcms/colors/package/metadata.desktop

To: broulik, #plasma, #vdg
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart