D24519: Delegate KCM cursor theme config management to KConfigXT
This revision was automatically updated to reflect the committed changes. Closed by commit R119:fec538fb02e0: Delegate KCM cursor theme config management to KConfigXT (authored by bport). REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24519?vs=67949=67953 REVISION DETAIL https://phabricator.kde.org/D24519 AFFECTED FILES kcms/cursortheme/kcmcursortheme.cpp kcms/cursortheme/kcmcursortheme.h kcms/cursortheme/package/contents/ui/Delegate.qml kcms/cursortheme/package/contents/ui/main.qml 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
D24519: Delegate KCM cursor theme config management to KConfigXT
bport updated this revision to Diff 67949. bport added a comment. Add notify to setPreferredSize REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24519?vs=67606=67949 REVISION DETAIL https://phabricator.kde.org/D24519 AFFECTED FILES kcms/cursortheme/kcmcursortheme.cpp kcms/cursortheme/kcmcursortheme.h kcms/cursortheme/package/contents/ui/Delegate.qml kcms/cursortheme/package/contents/ui/main.qml 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
D24519: Delegate KCM cursor theme config management to KConfigXT
ervin requested changes to this revision.
ervin added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> kcmcursortheme.cpp:157
> {
> -return m_selectedThemeRow;
> -}
> -
> -void CursorThemeConfig::setSelectedSizeRow(int row)
> -{
> -Q_ASSERT (row < m_sizesModel->rowCount() && row >= 0);
> -
> -// we don't return early if m_selectedSizeRow == row as this is called
> after the model is changed
> -m_selectedSizeRow = row;
> -emit selectedSizeRowChanged();
> -
> -int size = m_sizesModel->item(row)->data().toInt();
> -
> m_preferredSize = size;
> }
Once you got the NOTIFY part of the property it'll be emitted from here.
> kcmcursortheme.h:50
> Q_PROPERTY(bool downloadingFile READ downloadingFile NOTIFY
> downloadingFileChanged)
> +Q_PROPERTY(int preferredSize READ preferredSize WRITE setPreferredSize)
>
Should also have the NOTIFY part.
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
D24519: Delegate KCM cursor theme config management to KConfigXT
bport updated this revision to Diff 67606. bport added a comment. Take in consideration feedbacks REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24519?vs=67561=67606 REVISION DETAIL https://phabricator.kde.org/D24519 AFFECTED FILES kcms/cursortheme/kcmcursortheme.cpp kcms/cursortheme/kcmcursortheme.h kcms/cursortheme/package/contents/ui/Delegate.qml kcms/cursortheme/package/contents/ui/main.qml 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
D24519: Delegate KCM cursor theme config management to KConfigXT
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 ) const;
s/row/index/ I'd say.
> kcmcursortheme.h:82
> +Q_INVOKABLE int cursorThemeIndex(const QString ) 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
> +#include
>
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
D24519: Delegate KCM cursor theme config management to KConfigXT
bport added inline comments. INLINE COMMENTS > davidedmundson wrote in main.qml:99 > I think this will be broken if you change this combo box, then change theme. > > When a user explicitly changes this combo box this binding gets broken with > currentIndex now reflecting an explicit number. > > On theme change cursorThemeSettings.cursorSize changes correctly but our > binding is gone. > > This might have to be a connection again 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) 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
D24519: Delegate KCM cursor theme config management to KConfigXT
davidedmundson added inline comments.
INLINE COMMENTS
> main.qml:99
> textRole: "display"
> +currentIndex:
> kcm.cursorSizeIndex(kcm.cursorThemeSettings.cursorSize);
> onActivated: {
I think this will be broken if you change this combo box, then change theme.
When a user explicitly changes this combo box this binding gets broken with
currentIndex now reflecting an explicit number.
On theme change cursorThemeSettings.cursorSize changes correctly but our
binding is gone.
This might have to be a connection again
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
D24519: Delegate KCM cursor theme config management to KConfigXT
bport created this revision. bport added reviewers: Plasma, mart, ervin. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. bport requested review of this revision. REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D24519 AFFECTED FILES kcms/cursortheme/kcmcursortheme.cpp kcms/cursortheme/kcmcursortheme.h kcms/cursortheme/package/contents/ui/Delegate.qml kcms/cursortheme/package/contents/ui/main.qml kcms/cursortheme/xcursor/sortproxymodel.h To: bport, #plasma, mart, ervin Cc: 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
