[Colors KCM] Port to new design
ClosedPublic

Authored by broulik on Apr 17 2018, 11:37 AM.

Details

Summary

Overall the user experience has been streamlined and simplified a lot:

  • The "Default" theme option has been dropped in favor of having the "Defaults" button revert the selected theme to Breeze. While technically the old code made it read the hardcoded default colors in KColorScheme (which cause the window decoration to turn blue as it cannot write into KWin config like the theme files can), this change makes most sense from a UX POV.
  • The "Current" theme option has also been removed. Technically, when applying a theme the colors are copied into kdeglobals, so you could have a custom theme that is not an actual .colors file on disk. However, this is imho quite a niche usecase. Ideally, we showed a "Custom" theme as soon as the actual theme diverges from any theme file installed but that would require tediously comparing dozens of settings values which I don't think is feasible. At least when the color scheme name set in kdeglobals does not exist, a warning is now displayed.
  • The "Apply to non-Qt applications checkbox" which isn't something one would want to uncheck has been removed. It is still read from kcmdisplayrc for those who really want to disable it but there is no user-visible checkbox anymore.

KColorSchemeEditor is now completely disentangled from the KCM and is merely launched as separate process:

  • When editing a system scheme, upon clicking "Save" the user is prompted to type a new scheme name. This ensures that any custom scheme is always present on disk reducing the need for a "Current" entry. When the dialog is then closed, the newly saved theme is selected.
  • When editing a user scheme, the "Save" button turns into "Apply", which when clicked updates the scheme with any changes made in the dialog (different behavior from when kcolorschemeeditor is launched standalone, where "Save" is always essentially "Save As")

The rewrite also comes with all the goodies we got in the other new KCMs, such as the ability to drop a .colors file into the view to install it or undo deletion until you apply your changes.

BUG: 307216
FIXED-IN: 5.16.0

Test Plan

Closes T7243

Minor issues:

  • KColorSchemeEditor lacks an OK button so when launched from the KCM you always have to do Save/Apply and Close instead.

Diff Detail

Repository
R119 Plasma Desktop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
broulik created this revision.Apr 17 2018, 11:37 AM
Restricted Application added a project: Plasma. · View Herald TranscriptApr 17 2018, 11:37 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
broulik requested review of this revision.Apr 17 2018, 11:37 AM

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

ngraham edited the test plan for this revision. (Show Details)Apr 17 2018, 1:13 PM
ngraham added a task: T7243: Colors.

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.

abetts added a subscriber: abetts.Apr 17 2018, 2:46 PM

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

mart added a subscriber: mart.Apr 21 2018, 7:57 PM

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

mart added a comment.Apr 21 2018, 8:00 PM

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?

-1 for "current" here too.

I also don't like "Default" either. What does that even mean to a regular user?

In D12278#251210, @mart 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.

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

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

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.

broulik updated this revision to Diff 32881.Apr 23 2018, 1:08 PM
broulik edited the summary of this revision. (Show Details)
broulik edited the test plan for this revision. (Show Details)
  • 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

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.

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

This is what it will most likely look like once the tinting of controls has been figured out

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

mart added a comment.May 2 2018, 12:34 PM

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

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

mart added a comment.May 2 2018, 1:35 PM

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

Ping? Is this KCM doing alright?

broulik planned changes to this revision.Jun 28 2018, 3:05 PM
nicolasfella added inline comments.
kcms/colors/colors.cpp
314

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

Any word on this?

GB_2 added a subscriber: GB_2.Jan 2 2019, 9:00 AM

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.

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

broulik updated this revision to Diff 50123.Wed, Jan 23, 4:08 PM
broulik retitled this revision from WIP: [Colors KCM] Port to new design to [Colors KCM] Port to new design.
broulik edited the summary of this revision. (Show Details)
broulik edited the test plan for this revision. (Show Details)
GB_2 added inline comments.Wed, Jan 23, 8:10 PM
kcms/colors/colors.cpp
59

Better: "Choose the color scheme"

kcms/colors/kcm_colors.desktop
105–106

Better: "Choose the color scheme"

kcms/colors/package/contents/ui/main.qml
31

Better: "This module lets you choose the color scheme."

kcms/colors/package/metadata.desktop
4

Better: "Choose the color scheme"

broulik updated this revision to Diff 50164.Thu, Jan 24, 7:26 AM
broulik edited the summary of this revision. (Show Details)
broulik edited the test plan for this revision. (Show Details)
  • Update wording
  • Add "disabled text" to preview and make delegate slightly larger because of this
  • Use "Apply" button in color scheme editor when in overwrite mode
  • Try to set editor as modal dialog but didn't work
broulik edited the test plan for this revision. (Show Details)Thu, Jan 24, 7:26 AM
broulik edited the summary of this revision. (Show Details)Thu, Jan 24, 7:56 AM
broulik updated this revision to Diff 50172.Thu, Jan 24, 10:33 AM
  • Support installing files from remote locations through KIO like in the other KCMs, downloads to temp folder and then installs it
broulik updated this revision to Diff 50175.Thu, Jan 24, 10:40 AM

Unify with the other KCMs:

  • Honor KIOSK ghns restriction (should this also count for the "Upload Scheme" button"?)
  • Double click delegate to apply instantly
davidedmundson added inline comments.Thu, Jan 24, 1:09 PM
kcms/colors/colors.cpp
229

what would happen if this is called twice before the file_copy completes?

kcms/colors/package/contents/ui/main.qml
72

It seems these should be standard across all GridView KCMs.

broulik updated this revision to Diff 50181.Thu, Jan 24, 1:19 PM
broulik edited the test plan for this revision. (Show Details)
  • Cleanup a bit
  • Disable KCM while a file is being downloaded
  • Fix modal stuff for scheme editor
This revision is now accepted and ready to land.Thu, Jan 24, 1:22 PM
This revision was automatically updated to reflect the committed changes.

It's amazing to see all the work that was done here. Thanks everyone for working together on this. I hope our users see this new KCM as a step forward.

aacid added a subscriber: aacid.Thu, Jan 31, 6:13 PM

Did this land in Plasma 5.15 on purpose or was it a mistake?