D12459: [Icon KCM] Port to new design

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> main.cpp:393
> +for (int i = 0; i < KIconLoader::LastGroup; i++) {
> +KIconLoader::emitChange(KIconLoader::Group(i));
> +

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?

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, #vdg, hein
Cc: kossebau, lueck, schwarzer, hein, zzag, abetts, ngraham, plasma-devel, 
Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, 
ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, sebas, 
apol, ahiemstra, mart

D12459: [Icon KCM] Port to new design

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R119:9496728fcb6c: [Icon KCM] Port to new design (authored by 
broulik).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D12459?vs=33086=33206#toc

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12459?vs=33086=33206

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

AFFECTED FILES
  kcms/icons/CMakeLists.txt
  kcms/icons/Messages.sh
  kcms/icons/icons.cpp
  kcms/icons/icons.desktop
  kcms/icons/icons.h
  kcms/icons/iconsmodel.cpp
  kcms/icons/iconsmodel.h
  kcms/icons/iconthemes.cpp
  kcms/icons/iconthemes.h
  kcms/icons/kcm_icons.desktop
  kcms/icons/main.cpp
  kcms/icons/main.h
  kcms/icons/package/contents/ui/IconSizePopup.qml
  kcms/icons/package/contents/ui/main.qml
  kcms/icons/package/metadata.desktop
  kcms/icons/tests/CMakeLists.txt
  kcms/icons/tests/testicons.cpp

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

D12459: [Icon KCM] Port to new design

[Eike Hein]

hein accepted this revision.
hein added a comment.
This revision is now accepted and ready to land.


  Go go go

REPOSITORY
  R119 Plasma Desktop

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

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

D12459: [Icon KCM] Port to new design

[Frederik Schwarzer]

schwarzer added inline comments.

INLINE COMMENTS

> broulik wrote in iconsmodel.cpp:41
> 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.

Ah, ok. Thanks for the explanation. :)

REPOSITORY
  R119 Plasma Desktop

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

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

D12459: [Icon KCM] Port to new design

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> schwarzer wrote in iconsmodel.cpp:41
> Is this logic the right way around?

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.

REPOSITORY
  R119 Plasma Desktop

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

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

D12459: [Icon KCM] Port to new design

[Frederik Schwarzer]

schwarzer added inline comments.

INLINE COMMENTS

> iconsmodel.cpp:41
> +{
> +if (parent.isValid()) {
> +return 0;

Is this logic the right way around?

REPOSITORY
  R119 Plasma Desktop

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

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

D12459: [Icon KCM] Port to new design

[Frederik Schwarzer]

schwarzer added a subscriber: lueck.
schwarzer added a comment.


  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?

REPOSITORY
  R119 Plasma Desktop

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

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

D12459: [Icon KCM] Port to new design

[Kai Uwe Broulik]

broulik added a subscriber: schwarzer.
broulik added a comment.


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

REPOSITORY
  R119 Plasma Desktop

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

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

D12459: [Icon KCM] Port to new design

[Eike Hein]

hein added a comment.


  Model looks good! :)

REPOSITORY
  R119 Plasma Desktop

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

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

D12459: [Icon KCM] Port to new design

[Kai Uwe Broulik]

broulik updated this revision to Diff 33086.
broulik added a comment.


  - Use custom model class

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12459?vs=32871=33086

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

AFFECTED FILES
  kcms/icons/CMakeLists.txt
  kcms/icons/Messages.sh
  kcms/icons/icons.cpp
  kcms/icons/icons.desktop
  kcms/icons/icons.h
  kcms/icons/iconsmodel.cpp
  kcms/icons/iconsmodel.h
  kcms/icons/iconthemes.cpp
  kcms/icons/iconthemes.h
  kcms/icons/main.cpp
  kcms/icons/main.h
  kcms/icons/package/contents/ui/IconSizePopup.qml
  kcms/icons/package/contents/ui/main.qml
  kcms/icons/package/metadata.desktop
  kcms/icons/tests/CMakeLists.txt
  kcms/icons/tests/testicons.cpp

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

D12459: [Icon KCM] Port to new design

[Eike Hein]

hein added a comment.


  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.

INLINE COMMENTS

> main.cpp:68
> +: KQuickAddons::ConfigModule(parent, args)
> +, m_iconGroups{
> +QStringLiteral("Desktop"),

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

> main.cpp:96
> +m_model->setItemRoleNames({
> +{Qt::DisplayRole, QByteArrayLiteral("display")},
> +{DescriptionRole, QByteArrayLiteral("description")},

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

In Plasma models we usually use uppercase role names btw.

> main.h:26
>  
> +#pragma once
>  

Is this legal in KDE code?

> IconSizePopup.qml:64
> +Layout.fillHeight: true
> +Layout.preferredHeight: iconTypeList.count * 
> measureDelegate.height + 4
> +activeFocusOnTab: false

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

> IconSizePopup.qml:90
> +highlighted: ListView.isCurrentItem
> +text: [
> +i18n("Desktop"),

Can we get rid of these copies too?

> IconSizePopup.qml:127
> +
> +// I have no idea what this code does but it works 
> and is just copied from the old KCM
> +var index = -1;

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.

> main.qml:83
> +Timer {
> +id: thumbTimer
> +interval: 1000

This timer seems to be unused?

> main.qml:121
> +
> +Component.onCompleted: {
> +// avoid reloading it when icon sizes or dpr changes on 
> startup

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

REPOSITORY
  R119 Plasma Desktop

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

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

D12459: [Icon KCM] Port to new design

[Vlad Zagorodniy]

zzag added a comment.


  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.

REPOSITORY
  R119 Plasma Desktop

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

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

D12459: [Icon KCM] Port to new design

[Kai Uwe Broulik]

broulik added a comment.


  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.

REPOSITORY
  R119 Plasma Desktop

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

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

D12459: [Icon KCM] Port to new design

[Vlad Zagorodniy]

zzag added a comment.


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

REPOSITORY
  R119 Plasma Desktop

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

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

D12459: [Icon KCM] Port to new design

[Kai Uwe Broulik]

broulik updated this revision to Diff 32871.
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.
broulik edited the test plan for this revision.
broulik added a comment.


  - 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

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12459?vs=32843=32871

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

AFFECTED FILES
  kcms/icons/CMakeLists.txt
  kcms/icons/Messages.sh
  kcms/icons/icons.cpp
  kcms/icons/icons.desktop
  kcms/icons/icons.h
  kcms/icons/iconthemes.cpp
  kcms/icons/iconthemes.h
  kcms/icons/kcm_icons.desktop
  kcms/icons/main.cpp
  kcms/icons/main.h
  kcms/icons/package/contents/ui/IconSizePopup.qml
  kcms/icons/package/contents/ui/main.qml
  kcms/icons/package/metadata.desktop
  kcms/icons/tests/CMakeLists.txt
  kcms/icons/tests/testicons.cpp

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