D20198: Don't lose list position after installing KNS cursor themes

leinir added a comment. In D20198#443779 , @ngraham wrote: > FYI the patch gets closed automatically only if the commit message has `Differential Revision: https://phabricator.kde.org/D20198` somewhere in it. If you use `arc land` or

D20198: Don't lose list position after installing KNS cursor themes

ngraham added a comment. FYI the patch gets closed automatically only if the commit message has `Differential Revision: https://phabricator.kde.org/D20198` somewhere in it. If you use `arc land` or cherry-pick the commit hash of the arc-created patch, that happens automatically. REPOSITORY

D20198: Don't lose list position after installing KNS cursor themes

leinir closed this revision. leinir added a comment. Forgot to close the revision in R119:4545601adec0 REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D20198 To: leinir, #plasma,

D20198: Don't lose list position after installing KNS cursor themes

leinir updated this revision to Diff 55455. leinir marked an inline comment as done. leinir added a comment. Address @apol's comment REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D20198?vs=55403=55455 REVISION DETAIL

D20198: Don't lose list position after installing KNS cursor themes

leinir marked 2 inline comments as done. leinir added inline comments. INLINE COMMENTS > apol wrote in kcmcursortheme.cpp:468 > Use splitRef if you're just taking the last one. Quite - still need to convert that last to a string later, but less string creation's good :) Kind of a

D20198: Don't lose list position after installing KNS cursor themes

apol added a comment. Otherwise makes sense to me. INLINE COMMENTS > kcmcursortheme.cpp:468 > +for (const QString& deleted : entry.uninstalledFiles()) { > +QStringList list = deleted.split(QLatin1Char('/')); > +if

D20198: Don't lose list position after installing KNS cursor themes

leinir marked 4 inline comments as done. leinir added inline comments. INLINE COMMENTS > apol wrote in kcmcursortheme.cpp:472 > Maybe another solution would be having the model insert/remove instead of > resetting, although that works too I guess. New patch does this - less invasive change as

D20198: Don't lose list position after installing KNS cursor themes

leinir updated this revision to Diff 55403. leinir edited the summary of this revision. leinir edited the test plan for this revision. leinir added a comment. Less invasive change, but with more (what seem like fairly reasonable) assumptions. We now operate explicitly on installed and

D20198: Don't lose list position after installing KNS cursor themes

leinir added a comment. In D20198#442785 , @apol wrote: > So fixing refreshList to not reset isn't feasible? Hmm... i'll have a look, it might be REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D20198 To:

D20198: Don't lose list position after installing KNS cursor themes

apol added a comment. So fixing refreshList to not reset isn't feasible? REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D20198 To: leinir, #plasma, ngraham, broulik Cc: apol, plasma-devel, #plasma, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai,

D20198: Don't lose list position after installing KNS cursor themes

leinir updated this revision to Diff 55330. leinir added a comment. Address comments by @ngraham and @apol REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D20198?vs=55261=55330 REVISION DETAIL https://phabricator.kde.org/D20198 AFFECTED FILES

D20198: Don't lose list position after installing KNS cursor themes

leinir added a comment. In D20198#442364 , @ngraham wrote: > Why do we need this do-it-once helper function? To avoid having the lambda being called multiple times for subsequent installations, especially with incorrect parameters...

D20198: Don't lose list position after installing KNS cursor themes

apol added inline comments. INLINE COMMENTS > kcmcursortheme.cpp:466 > +const CursorTheme *theme = selectedIndex().isValid() ? > m_proxyModel->theme(selectedIndex()) : nullptr; > +if(theme) { > +QString currentTheme = theme->name(); This reads weird: how

D20198: Don't lose list position after installing KNS cursor themes

ngraham added a comment. Why do we need this do-it-once helper function? INLINE COMMENTS > kcmcursortheme.cpp:466 > +const CursorTheme *theme = selectedIndex().isValid() ? > m_proxyModel->theme(selectedIndex()) : nullptr; > +if(theme) { > +QString

D20198: Don't lose list position after installing KNS cursor themes

leinir created this revision. leinir added reviewers: Plasma, ngraham. leinir added a project: Plasma. Herald added a subscriber: plasma-devel. leinir requested review of this revision. REVISION SUMMARY When installing a new cursor theme from KNewStuff, the current theme would become