D24519: Delegate KCM cursor theme config management to KConfigXT

[Kevin Ottens]

ervin requested changes to this revision.
ervin added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kcmcursortheme.cpp:335
>  
> +int CursorThemeConfig::cursorSizeIndex(const int cursorSize) const
> +{

int, not const inst

> kcmcursortheme.cpp:349
>  
> -void CursorThemeConfig::save()
> +int CursorThemeConfig::cursorSizeFromIndex(int row)
>  {

s/row/index/

> kcmcursortheme.h:50
>      Q_PROPERTY(bool downloadingFile READ downloadingFile NOTIFY downloadingFileChanged)
> +    Q_PROPERTY(int preferredSize WRITE setPreferredSize)
>  

This is rather unusual, could we have a full fledged property here despite the boilerplate? I'm thinking of the future developer who might get bitten when changing the QML code and having a half working property.

> kcmcursortheme.h:79
>  
> +    Q_INVOKABLE int cursorSizeIndex(const int cursorSize) const;
> +    Q_INVOKABLE int cursorSizeFromIndex(int row);

int, not const int

> kcmcursortheme.h:80
> +    Q_INVOKABLE int cursorSizeIndex(const int cursorSize) const;
> +    Q_INVOKABLE int cursorSizeFromIndex(int row);
> +    Q_INVOKABLE int cursorThemeIndex(const QString &cursorTheme) const;

s/row/index/ I'd say.

> kcmcursortheme.h:82
> +    Q_INVOKABLE int cursorThemeIndex(const QString &cursorTheme) const;
> +    Q_INVOKABLE QString cursorThemeFromIndex(int index) const;
>  Q_SIGNALS:

nitpick: Please add an empty line after that one.

> bport wrote in main.qml:99
> After testing seems to be not broken.
> the binding is set to a method that transorm current size value to index (kcm.cursorSizeIndex do that)

David has a point, the binding of currentIndex will be broken as soon as the user changes the entry in the combo box.

That being said, IIRC this shouldn't matter much in that context though, because we seem to be just after pre-selecting the right entry in the combo box when the module is displayed and not always enforcing a currentIndex value (which we can't in the context of combo box as David pointed out).

What matters is that the settings get properly updated when the user select a new entry in the combo box which is handled with onActivated just fine.

> sortproxymodel.h:25
>  #include <thememodel.h>
> +#include <cursortheme.h>
>  

Why is that include added? This doesn't seem necessary to me.

REPOSITORY
  R119 Plasma Desktop

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

To: bport, #plasma, mart, ervin
Cc: davidedmundson, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20191009/7c2b8845/attachment-0001.html>