mwolff added inline comments. INLINE COMMENTS
> foldermodeltest.cpp:48 > + > +void FolderModelTest::initTestCase() > +{ remove the init and cleanup if both are empty? > screenmappertest.cpp:36 > + > +void ScreenMapperTest::cleanupTestCase() > +{ remove if empty > screenmappertest.cpp:45 > + > +void ScreenMapperTest::cleanup() > +{ dito > foldermodel.cpp:163 > + if (m_screenMapper) { > + m_screenMapper->disconnect(this); > + m_screenMapper->removeScreen(m_screen, url()); can you add a comment why this explicit disconnect is needed? is it b/c removeScreen would otherwise emit a signal that we'd try to handle but don't want to? otherwise the QObject dtor would disconnect us > foldermodel.h:294 > private: > + > struct DragImage { ws only, unneeded change, can be unset > screenmapper.cpp:41 > + this, &ScreenMapper::screenMappingChanged); > + m_screenMappingChangedTimer->setInterval(100); > + m_screenMappingChangedTimer->setSingleShot(true); this is a pretty arbitrary timer, can you add a comment on why 100ms is better than going through the eventloop once via QMetaObject::invokeMethod with the delayed flag set? > screenmapper.cpp:52 > + if (screenUrl.scheme().isEmpty()) > + screenUrl = QUrl::fromLocalFile(path); > + const auto screenPathWithScheme = screenUrl.url(); should this maybe be QUrl::fromUserInput with a prefer local file flag set? > screenmapper.cpp:69 > + if (firstScreen == screenId) { > + if (m_availableScreens.isEmpty()) { > + m_firstScreenForPath[path] = -1; could be rewritten/simplified: const auto it = std::min_element(...); m_firstScreenForPath[path] = it == m_availableScreens.constEnd() ? -1 : *it; > screenmapper.cpp:82 > + *pathIt = pathIt.value() - 1; > + } else if (path.isEmpty()) { > + pathIt = m_screensPerPath.begin(); so if `path` is empty, this adjusts all paths? can you add a comment on when this happens and what it's for? > screenmapper.cpp:98 > + > + QUrl screenUrl(path); > + if (screenUrl.scheme().isEmpty()) these three lines are shared with above, introduce a helper function in an anon namespace for it: `QUrl screenUrlForPath(const QString &path);` > screenmapper.cpp:108 > + for (const auto &name: it.value()) { > + const auto url = QUrl(name); > + // add the items to the new screen, if they are on a disabled > screen and their `m_itemsOnDisabledScreensMap` stores "names" from the `m_screenItemMap`, so I think this line here is wrong and should also use the proposed `screenUrlForPath` above, or `QUrl::fromUserInput`, no? Otherwise you may end up with relative urls for file paths? > screenmapper.cpp:124 > + m_availableScreens.append(screenId); > + if (!path.isEmpty()) { > + auto it = m_screensPerPath.find(path); again, seems like an empty path has a special meaning, can you document that somewhere please? > screenmapper.cpp:140 > + > +void ScreenMapper::addMapping(const QString &name, int screen, > MappingSignalBehavior behavior) > +{ is `name` also a `path`? > screenmapper.cpp:150 > + > +void ScreenMapper::removeFromMap(const QString &name) > +{ dito > screenmapper.h:43 > +public: > + enum MappingSignalBehavior { > + DelayedSignal = 0, this needs to be documented, i.e. why and when is it required to distinguish between these signal behaviors? 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