[Icon KCM] Port to new design
ClosedPublic
Actions

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

Details

Reviewers

Group Reviewers

Plasma
VDG

Commits

R119:9496728fcb6c: [Icon KCM] Port to new design

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.

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

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

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.

In D12459#252290, @broulik wrote

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.

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)

Use custom model class

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?

In D12459#253942, @broulik wrote:

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

Go go go

This revision is now accepted and ready to land.Apr 27 2018, 2:28 PM

Closed by commit R119:9496728fcb6c: [Icon KCM] Port to new design (authored by broulik). · Explain WhyApr 27 2018, 2:32 PM

This revision was automatically updated to reflect the committed changes.

kossebau added a subscriber: kossebau.Apr 25 2020, 3:05 PM

kossebau added inline comments.

kcms/icons/main.cpp
393	Is KIconLoader::emitChange also for kde4 apps only? Asking as KIconLoader of KIconThemes seems to have this as part of normal API, without any deprecation notes, and e.g. the respective client-side signal is used in KXmlGui and other places. What would be the Qt5/KF5 way otherwise?

Revision Contents
Changeset List

		Path
M		kcms/icons/CMakeLists.txt (22 lines)
M		kcms/icons/Messages.sh (2 lines)
D	M	kcms/icons/icons.h (147 lines)
D	M	kcms/icons/icons.cpp (716 lines)
V		kcms/icons/{icons.desktop → kcm_icons.desktop}
A	M	kcms/icons/iconsmodel.h (84 lines)
A	M	kcms/icons/iconsmodel.cpp (205 lines)
D	M	kcms/icons/iconthemes.h (74 lines)
D	M	kcms/icons/iconthemes.cpp (491 lines)
V		kcms/icons/{kcm_icons.desktop ← icons.desktop}
M		kcms/icons/main.h (93 lines)
M		kcms/icons/main.cpp (541 lines)
A	M	kcms/icons/package/contents/ui/IconSizePopup.qml (169 lines)
A	M	kcms/icons/package/contents/ui/main.qml (276 lines)
A	M	kcms/icons/package/metadata.desktop (16 lines)
D	M	kcms/icons/tests/CMakeLists.txt (16 lines)
D	M	kcms/icons/tests/testicons.cpp (16 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	32843			Apr 22 2018, 8:36 PM	★	★
Diff 2	32871			Apr 23 2018, 9:52 AM	★	★
Diff 3	33086			Apr 25 2018, 4:21 PM	★	★
Diff 4	33206	9961a72	R119:9496728fcb6c012cf1deb773a3d0a5129b4a629e	Apr 27 2018, 2:28 PM	★	★