amantia added inline comments. INLINE COMMENTS
> mwolff wrote in foldermodel.cpp:1316 > adding some comment here would help people reading the code in the future. > I.e. something like "exclude items that are shown on a different screen" > > also, I personally think this code is not pretty at all. When no mapping is > found, the first model that encounters the item in this `filterAcceptsRow` > will take up the mapping. But... Isn't that too random? Shouldn't the mapping > be done elsewhere? Why can there be items encountered here that have no > mapping? At the very least, can you add a comment here to explain why "first > comes, first served" is the right approach? Added comments and made it less random (folder views on the first available screen will own the new items). There is a TODO though, as we seem to not react correctly for the case when a folder view is removed from being a desktop containment. Will handle it in a followup diff. > mwolff wrote in foldermodel.cpp:1691 > shorten this to: > > if (containment && containment->corona()) > m_screenMapper->registerCorona(containment->corona()); > > ? Or is asking for the corona too expensive? I don't like to call the same method twice, and in corona() call usually is is an if and qobject_cast, but can be also a recursive call. > mwolff wrote in screenmapper.cpp:96 > shouldn't you reset m_firstScreen and potentially the other maps, too? Actually that clear() there was wrong. Available screens are either added via registerScreen or when the screen setup changes, via signals. > mwolff wrote in screenmapper.cpp:121 > count -> size > > newMap.reserve(size / 2); count() and size() is the same, no? REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D8493 To: amantia, #plasma, ervin, mlaurent, dvratil, hein, aacid, davidedmundson, apol Cc: mwolff, davidedmundson, broulik, mart, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
