hein added a comment.
Modulo above edge-casey comments it looks good to me. INLINE COMMENTS > foldermodel.cpp:1324 > + const QString name = item.url().toString(); > + const int screen = m_screenMapper->screenForItem(name); > + // don't do anything if the folderview is not associated with a > screen You're not testing m_screenMapper!=nullptr here. > foldermodel.cpp:1688 > + if (m_screenMapper) { > + connect(m_screenMapper, &ScreenMapper::screensChanged, this, > &FolderModel::invalidateFilter); > + connect(m_screenMapper, &ScreenMapper::screenMappingChanged, this, > &FolderModel::invalidateFilter); I know it's a very theoretical case, but you're not disconnecting from an old non-null ScreenMapper here. You're also not calling invalidateFilter even though filterAcceptsRow uses the mapper. It's all a bit non-hygienic in the sense of "allow stuff to be set and reset in any order". Which can be somewhat important, especially as FolderModel doesn't inherit from QQmlParserStatus (hint: would be a great follow-up patch) and sometimes things can fall apart in config-dependent ways during initialization. I'd appreciate a pass through this code for that concern for good Qt Quick hygiene. REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D8493 To: amantia, #plasma, ervin, mlaurent, dvratil, hein, aacid, davidedmundson, apol, mwolff Cc: anthonyfieroni, ngraham, mwolff, davidedmundson, broulik, mart, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol