Port kcm icons to kconfigxt
ClosedPublic

Authored by bport on Oct 22 2019, 6:53 AM.

Details

Summary

Also fix 2 bugs:

  • keyboard navigation on size category didn't work as expected, we didn't saved what we see

(open size widget: arrow down, arrow right, save => will save the size to desktop entry instead of toolbar

  • the icon slider was not updated when we change theme

Diff Detail

Repository
R119 Plasma Desktop
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 18348
Build 18366: arc lint + arc unit
bport created this revision.Oct 22 2019, 6:53 AM
Restricted Application added a project: Plasma. · View Herald TranscriptOct 22 2019, 6:53 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
bport requested review of this revision.Oct 22 2019, 6:53 AM
broulik added inline comments.
kcms/icons/main.cpp
141

Does this KIconTheme instance need caching? I recall creating those parses a tonne of files and directories and is quite slow

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

Do you not need to leave this (or a similar) connect in place?

47–53

I don't like this duplication, especially since sometimes you use currentIndex and sometimes the currentCategory

bport added inline comments.Oct 22 2019, 7:13 AM
kcms/icons/package/contents/ui/IconSizePopup.qml
45

This one is replaced (and that fix bug described above by call onVisibleChange)
and with kconfigxt property handling

47–53

I wiil look if I can simplify that

ervin requested changes to this revision.Oct 25 2019, 12:52 PM
ervin added inline comments.
kcms/icons/main.cpp
141

Agreed, it'll hit the disk quite a bit, this might be a problem in a function which takes part in bindings (more than one which will change around the same time).

179

Indentation looks wrong here.

kcms/icons/package/contents/ui/IconSizePopup.qml
47–53

I like the intent of trying to have names instead of just indices everywhere... but that might not be feasible without extra complexity we'll want to avoid indeed.

I think the more fundamental problem is that the settings are based on those names, while the invokables (most notably availableIconSizes) are based on indices. Probably something to address, like have names everywhere and just a function to map from index to name in the iconTypeList.

This revision now requires changes to proceed.Oct 25 2019, 12:52 PM
bport updated this revision to Diff 69036.Oct 30 2019, 10:37 AM

Take in consideration feedback (cache kicon call)
Port to ManagedConfigModule

ervin requested changes to this revision.Oct 30 2019, 12:07 PM
ervin added inline comments.
kcms/icons/main.cpp
101

Shouldn't you be able to remove that connect? I'd expect this signal trickling down to changing the settings object, wouldn't it?

102

Now that my ManagedConfigModule change landed, you should use a proper compile time check connect to settingsChanged here.

110

Don't do foreach. Instead write:

for (auto theme : qAsConst(m_iconThemeCache)) {

Or even better, just use qDeleteAll:

qDeleteAll(m_iconThemeCache)

Or even better yet : try to use std::unique_ptr, std::shared_ptr or QScopedPointer as values in your associative container (I let you check which one fits best).

137

nitpick, I find = more readable in such a context (and less prone to the most vexing parse since you don't use curly braces init).

I'd write: const auto themeName = m_model->selectedTheme();

138

Couldn't you fill the cache as soon as the selected theme changes instead? This way you wouldn't need to modify your cache here.

kcms/icons/main.h
34

s/QMap/QHash/

83

Killing the const here is semantically wrong IMO.

117

You don't need the keys to be sorted, so please use QHash instead.

Also we do camel case here, not snake case. :-)

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

I think I'd have expected that logic on the C++ side actually. Others might disagree. :-)

91

Urgh, I'd assert than silently swallow that I think.

This revision now requires changes to proceed.Oct 30 2019, 12:07 PM
bport updated this revision to Diff 70452.Nov 27 2019, 6:53 PM

Take in consideration feedback

ervin requested changes to this revision.Nov 29 2019, 12:06 PM

Looks like you didn't address a faire number of comments, or they ended up in the wrong commit (by the look of D24847).

kcms/icons/main.cpp
102

I still think those connects might not be necessary.

110

Still not addressed... mind your for loops...

137–139

nitpick, I find = more readable in such a context (and less prone to the most vexing parse since you don't use curly braces init).

I'd write: const auto themeName = m_model->selectedTheme();

kcms/icons/main.h
34

Still not addressed, use a QHash

83

Still not addressed, put the const back

117

Still not addressed... s/m_kicon_theme_map/m_iconThemeMap/ (we do camel case here) and should be a QHash.

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

Not on the C++ side still?

91

Still not addressed

This revision now requires changes to proceed.Nov 29 2019, 12:06 PM
bport updated this revision to Diff 71117.Dec 9 2019, 12:28 PM

Take in consideration feedbacks

bport updated this revision to Diff 71127.Dec 9 2019, 4:54 PM
bport marked 5 inline comments as done.

Take in consideration ervin feedback

bport updated this revision to Diff 71668.Dec 16 2019, 3:17 PM

Add theme to kconfigXT

crossi added inline comments.Dec 17 2019, 8:56 AM
kcms/icons/iconsmodel.cpp
29 ↗(On Diff #71668)

include not used

bport updated this revision to Diff 71956.Dec 21 2019, 3:48 PM

Remone unused include

Just a couple of stylistic issues left AFAICT

kcms/icons/iconsmodel.h
47 ↗(On Diff #71956)

Kind of a nitpick in this context, but parent should go last and default to nullptr. It's the Qt convention for QObject ctors.

kcms/icons/iconssettings.cpp
48 ↗(On Diff #71956)

The opening curly braces should be on the same line than the if

kcms/icons/main.cpp
148–149

This one likely requires a comment, since normally you wouldn't need such an emit.

bport updated this revision to Diff 72189.Dec 26 2019, 8:19 AM

Update according to ervin feedback

bport marked 3 inline comments as done.Dec 26 2019, 8:19 AM
ervin accepted this revision.Dec 26 2019, 8:23 AM
This revision is now accepted and ready to land.Dec 26 2019, 8:23 AM
This revision was automatically updated to reflect the committed changes.