[Icon KCM] Port to new design
ClosedPublic

Authored by broulik on Apr 22 2018, 8:44 PM.

Details

Summary

This completely drops the icon customization with icon effects and only retains the icon size settings. I also couldn't find any trace of that "animations enabled" check box it used to have.
It adds drag and drop support for installing archives as themes. It can also download them from remote location when dropped from e.g. a browser. Installing theme files actually never worked in Plasma 5 as it got broken in the port from KIO NetAccess to storedGet.
An animated preview is also added on hover showing a common selection of icons.

Implements T7262

BUG: 367619
BUG: 334301
CCBUG: 163992

Test Plan


Icon size config flyout, I chose not to use a separate page as in the mockup as it's really just the size settings now:

The slider shows the possible size steps

Removing installed theme

It comes with an undo feature and only if you hit Apply or OK it will acutally delete the themes, so you can undo individual or all (by using Revert button) deletions. If this is accepted, I'll look into rolling this out to the other KCMs that allow permanently deleting actual files. It is inspired by wallpaper settings.

Installing theme with InlineMessage

Live preview on hover, shows a random selection, then shows common mime types, then shows common folder icons

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 22 2018, 8:44 PM
Restricted Application added a project: Plasma. · View Herald TranscriptApr 22 2018, 8:44 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
broulik requested review of this revision.Apr 22 2018, 8:44 PM

Oh my god, this looks so, so good. I love the icon size slider panel.

abetts added a subscriber: abetts.Apr 22 2018, 9:27 PM

Love it! I like how convenient the configure button is.

broulik updated this revision to Diff 32871.Apr 23 2018, 9:55 AM
broulik retitled this revision from WIP: [Icon KCM] Port to new design to [Icon KCM] Port to new design.
broulik edited the summary of this revision. (Show Details)
broulik edited the test plan for this revision. (Show Details)
  • Kill old KCM code
  • "Defaults" will switch back to KIconTheme::defaultThemeName or if that isn't there (is hicolor here which doesn't make sense) will use breeze
  • "Defaults" will also revert custom icon effects so the user can get rid of them now that they're no longer configurable
  • Apply icon settings to KDE4 apps
  • Add animated preview
zzag added a subscriber: zzag.Apr 23 2018, 11:48 AM

Delete button is not visible in the attached video at 0:14 (when you hover Breeze).

In D12459#252269, @zzag wrote:

Delete button is not visible in the attached video at 0:14 (when you hover Breeze).

Looks just like compression artefact in the screenshot. The theme cannot be uninstalled as it's system-wide and the thin lines seemed to throw it off.

zzag added a comment.Apr 23 2018, 12:50 PM

Looks just like compression artefact in the screenshot. The theme cannot be uninstalled as it's system-wide and the thin lines seemed to throw it off.

Yeah, it makes sense.

hein added a subscriber: hein.Apr 25 2018, 4:14 PM

In general: This code would be cleaner if it wasn't using QStandardItemModel but just a QAbstractListModel subclass. Then stuff like the "selected theme index" could use either a role or a QItemSelectionModel, and the role enum wouldn't need to live outside of the model. :)

Otherwise it looks quite good.

kcms/icons/main.cpp
63

Do we have these strings somewhere in KIcon* so we don't need to duplicate them?

98

DisplayRole is usually already defined, are you sure you need to duplicate it here?

In Plasma models we usually use uppercase role names btw.

kcms/icons/main.h
26

Is this legal in KDE code?

kcms/icons/package/contents/ui/IconSizePopup.qml
65

Can you explain this better? Measure items and fixed pixel values raise red flags :)

91

Can we get rid of these copies too?

128

I think it's trying to correct sizes being off by a fixed offset. But why is this necessary? What breaks if you remove this? This might not be a problem in 2018.

kcms/icons/package/contents/ui/main.qml
84

This timer seems to be unused?

122

We usually have these at the bottom (this confused me while reading because I thought the Repeater was outside the Flow)

broulik updated this revision to Diff 33086.Apr 25 2018, 4:22 PM
  • Use custom model class
hein added a comment.Apr 25 2018, 4:33 PM

Model looks good! :)

broulik edited the summary of this revision. (Show Details)Apr 25 2018, 9:41 PM

@schwarzer I'm changing the catalog name to be more consistent with the other KCMs, is that an issue?

@schwarzer I'm changing the catalog name to be more consistent with the other KCMs, is that an issue?

There have been some discussions recently about catalog naming but I did not follem them.
@lueck Do you have an opinion about this?

schwarzer added inline comments.Apr 26 2018, 7:39 AM
kcms/icons/iconsmodel.cpp
42

Is this logic the right way around?

broulik added inline comments.Apr 26 2018, 8:18 AM
kcms/icons/iconsmodel.cpp
42

The parent is valid for sub levels in a tree but this is a flat model so there will never be a parent as we only have a root layer.

schwarzer added inline comments.Apr 26 2018, 9:16 AM
kcms/icons/iconsmodel.cpp
42

Ah, ok. Thanks for the explanation. :)

hein accepted this revision.Apr 27 2018, 2:28 PM

Go go go

This revision is now accepted and ready to land.Apr 27 2018, 2:28 PM
This revision was automatically updated to reflect the committed changes.