D8493: Make Folder View screen aware
This revision was automatically updated to reflect the committed changes. Closed by commit R119:88718b162ada: Make Folder View screen aware (authored by amantia). REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8493?vs=22795=23063 REVISION DETAIL https://phabricator.kde.org/D8493 AFFECTED FILES containments/desktop/package/contents/config/main.xml containments/desktop/package/contents/ui/FolderView.qml containments/desktop/package/contents/ui/FolderViewLayer.qml containments/desktop/plugins/folder/CMakeLists.txt containments/desktop/plugins/folder/autotests/CMakeLists.txt containments/desktop/plugins/folder/autotests/foldermodeltest.cpp containments/desktop/plugins/folder/autotests/foldermodeltest.h containments/desktop/plugins/folder/autotests/screenmappertest.cpp containments/desktop/plugins/folder/autotests/screenmappertest.h containments/desktop/plugins/folder/foldermodel.cpp containments/desktop/plugins/folder/foldermodel.h containments/desktop/plugins/folder/folderplugin.cpp containments/desktop/plugins/folder/screenmapper.cpp containments/desktop/plugins/folder/screenmapper.h To: amantia, #plasma, ervin, mlaurent, dvratil, hein, aacid, apol, mwolff Cc: anthonyfieroni, ngraham, mwolff, davidedmundson, broulik, mart, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
D8493: Make Folder View screen aware
amantia removed a reviewer: davidedmundson. This revision is now accepted and ready to land. REPOSITORY R119 Plasma Desktop BRANCH master REVISION DETAIL https://phabricator.kde.org/D8493 To: amantia, #plasma, ervin, mlaurent, dvratil, hein, aacid, apol, mwolff Cc: anthonyfieroni, ngraham, mwolff, davidedmundson, broulik, mart, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
D8493: Make Folder View screen aware
amantia added a comment. @davidedmundson Any comments? If not, I will push this tomorrow. 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
D8493: Make Folder View screen aware
hein accepted this revision. 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
D8493: Make Folder View screen aware
amantia added inline comments. INLINE COMMENTS > hein wrote in foldermodel.cpp:1688 > 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. As it ScreenMapper is a singleton, this will never happen, but nevertheless I added the disconnect. 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
D8493: Make Folder View screen aware
amantia updated this revision to Diff 22795. amantia marked 2 inline comments as done. amantia added a comment. Handle Eike's comments REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8493?vs=22444=22795 BRANCH master REVISION DETAIL https://phabricator.kde.org/D8493 AFFECTED FILES containments/desktop/package/contents/config/main.xml containments/desktop/package/contents/ui/FolderView.qml containments/desktop/package/contents/ui/FolderViewLayer.qml containments/desktop/plugins/folder/CMakeLists.txt containments/desktop/plugins/folder/autotests/CMakeLists.txt containments/desktop/plugins/folder/autotests/foldermodeltest.cpp containments/desktop/plugins/folder/autotests/foldermodeltest.h containments/desktop/plugins/folder/autotests/screenmappertest.cpp containments/desktop/plugins/folder/autotests/screenmappertest.h containments/desktop/plugins/folder/foldermodel.cpp containments/desktop/plugins/folder/foldermodel.h containments/desktop/plugins/folder/folderplugin.cpp containments/desktop/plugins/folder/screenmapper.cpp containments/desktop/plugins/folder/screenmapper.h 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
D8493: Make Folder View screen aware
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, ::screensChanged, this,
> ::invalidateFilter);
> +connect(m_screenMapper, ::screenMappingChanged, this,
> ::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
D8493: Make Folder View screen aware
amantia added a comment. Ok, let's wait for Eike and for the DND patches to be ready. 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
D8493: Make Folder View screen aware
mwolff added a comment. [14:24] andris: milian umm, how do I actually move files between screens? [14:26] the fact that you can actually resize it with Alt+right-click is a bug [14:27] kbroulik: dnd, which is WIP [14:28] milian: okay because right now the screen-aware FV does not work at all. other than that I dont have any icons on my second screen anymore :) [14:28] you mean the "position files at drop event"? [14:28] yes, but that is only one step [14:29] I'm working on the second, more important one [14:29] can you tell me what you mean by no icons on the second screen? [14:29] did you have some on the second screen and by applying the patch they got moved to the first screen? [14:29] i.e. some kind of config porting? [14:29] yes [14:29] andris: ^ that is useful important feedback, we need to handle that somehow [14:29] but kbroulik - how did you have icons on your second screen before? [14:29] milian: I have a FV on both screens, both pointing to desktop:/ [14:29] did you use the same folder? [14:30] yes [14:30] then both used to show the same icons, no? [14:30] they don't [14:30] err [14:30] yes, they *used to* [14:30] so this is somewhat to be expected I guess [14:30] right so nothing to be ported after all [14:30] yans: well, that's how it is [14:30] So i just asking, kbroulik thx for info. [14:30] what's missing is the DND to allow people to move files actually [14:30] I'm working on that, keep getting sidetracked by other projects [14:30] let me sit down on it again [14:35] milian: ah, okay, because right now DND is unchanged, ie. I drag files over and it says "would overwrite" :) [14:36] yes that is broken as before [14:36] I'm working on that and have a local WIP for that though [14:36] oki :) [14:36] so I cannot really comment on D8493 let's have Sho_ decide so I guess we may need to keep this one open until the two follow-up DND patches are ready to allow this to be tested in full-depth 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
D8493: Make Folder View screen aware
davidedmundson added a comment. Fine with me. Would be good if Eike could comment 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
D8493: Make Folder View screen aware
mwolff accepted this revision. mwolff added a comment. lgtm from my side. Anyone from the plasma team care to chime in? 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
D8493: Make Folder View screen aware
amantia added inline comments. INLINE COMMENTS > mwolff wrote in screenmapper.cpp:85 > this should imo be a for loop to make it clear that it is iterating over all > items (more ideomatic). Also, couldn't you use range-based for here even? range based loop: I need access to both key and value > mwolff wrote in screenmapper.cpp:91 > this is actually shared with above, so that could be in a lambda I don't see what can be extracted 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
D8493: Make Folder View screen aware
amantia updated this revision to Diff 22444. amantia added a comment. Some code reogranization per Milian's request REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8493?vs=22438=22444 BRANCH master REVISION DETAIL https://phabricator.kde.org/D8493 AFFECTED FILES containments/desktop/package/contents/config/main.xml containments/desktop/package/contents/ui/FolderView.qml containments/desktop/package/contents/ui/FolderViewLayer.qml containments/desktop/plugins/folder/CMakeLists.txt containments/desktop/plugins/folder/autotests/CMakeLists.txt containments/desktop/plugins/folder/autotests/foldermodeltest.cpp containments/desktop/plugins/folder/autotests/foldermodeltest.h containments/desktop/plugins/folder/autotests/screenmappertest.cpp containments/desktop/plugins/folder/autotests/screenmappertest.h containments/desktop/plugins/folder/foldermodel.cpp containments/desktop/plugins/folder/foldermodel.h containments/desktop/plugins/folder/folderplugin.cpp containments/desktop/plugins/folder/screenmapper.cpp containments/desktop/plugins/folder/screenmapper.h 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
D8493: Make Folder View screen aware
mwolff requested changes to this revision.
mwolff added a comment.
This revision now requires changes to proceed.
some more nits, sorry for that ;-)
INLINE COMMENTS
> screenmapper.cpp:69
> +auto adjustFirstScreen = [this, ](const QString ) {
> +int firstScreen = m_firstScreenForPath.value(path, -1);
> +if (firstScreen == screenId) {
this could now be inlined below since it's only being used in one of the
branches
> screenmapper.cpp:84
> +// needs to be updated.
> +const auto newFirstScreen =
> std::min_element(m_availableScreens.constBegin(),
> m_availableScreens.constEnd());
> +auto it = m_firstScreenForPath.begin();
this could be moved outside the branch and reused above, once the lambda is
inlined
> screenmapper.cpp:85
> +const auto newFirstScreen =
> std::min_element(m_availableScreens.constBegin(),
> m_availableScreens.constEnd());
> +auto it = m_firstScreenForPath.begin();
> +while (it != m_firstScreenForPath.end()) {
this should imo be a for loop to make it clear that it is iterating over all
items (more ideomatic). Also, couldn't you use range-based for here even?
> screenmapper.cpp:91
> +// we have now the path for the screen that was removed, so
> adjust it
> +pathIt = m_screensPerPath.find(it.key());
> +if (pathIt != m_screensPerPath.end()) {
this is actually shared with above, so that could be in a lambda
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
D8493: Make Folder View screen aware
amantia updated this revision to Diff 22438. amantia added a comment. Handle screen removal correctly: - adjust the first screen per path in a more efficient way - adjust the number of screens per path properly REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8493?vs=22434=22438 BRANCH master REVISION DETAIL https://phabricator.kde.org/D8493 AFFECTED FILES containments/desktop/package/contents/config/main.xml containments/desktop/package/contents/ui/FolderView.qml containments/desktop/package/contents/ui/FolderViewLayer.qml containments/desktop/plugins/folder/CMakeLists.txt containments/desktop/plugins/folder/autotests/CMakeLists.txt containments/desktop/plugins/folder/autotests/foldermodeltest.cpp containments/desktop/plugins/folder/autotests/foldermodeltest.h containments/desktop/plugins/folder/autotests/screenmappertest.cpp containments/desktop/plugins/folder/autotests/screenmappertest.h containments/desktop/plugins/folder/foldermodel.cpp containments/desktop/plugins/folder/foldermodel.h containments/desktop/plugins/folder/folderplugin.cpp containments/desktop/plugins/folder/screenmapper.cpp containments/desktop/plugins/folder/screenmapper.h 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
D8493: Make Folder View screen aware
mwolff added inline comments.
INLINE COMMENTS
> screenmapper.cpp:82
> +// the screen got completely removed, not only its path changed
> +pathIt = m_screensPerPath.begin();
> +while (pathIt != m_screensPerPath.end()) {
personally I'd make this code explicit. i.e. this here is somewhat harder to
grasp I think (and also slower, due to the repeated lookups) to something like
this:
const auto newFirstScreen = std::min_element(m_availableScreens.constBegin(),
m_availableScreens.constEnd());
for (auto : m_firstScreenForPath) {
if (screen == screenId) {
screen = newFirstScreen;
}
}
potentially we also need to update m_screensPerPath though, which the current
code doesn't do either. The branch above does write to `pathIt` though, is this
missing here?
> screenmapper.cpp:105
> +for (const auto : it.value()) {
> +const auto url = QUrl(name);
> +// add the items to the new screen, if they are on a disabled
> screen and their
unused variable, remove
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
D8493: Make Folder View screen aware
amantia updated this revision to Diff 22434. amantia added a comment. Remove unused var REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8493?vs=22428=22434 BRANCH master REVISION DETAIL https://phabricator.kde.org/D8493 AFFECTED FILES containments/desktop/package/contents/config/main.xml containments/desktop/package/contents/ui/FolderView.qml containments/desktop/package/contents/ui/FolderViewLayer.qml containments/desktop/plugins/folder/CMakeLists.txt containments/desktop/plugins/folder/autotests/CMakeLists.txt containments/desktop/plugins/folder/autotests/foldermodeltest.cpp containments/desktop/plugins/folder/autotests/foldermodeltest.h containments/desktop/plugins/folder/autotests/screenmappertest.cpp containments/desktop/plugins/folder/autotests/screenmappertest.h containments/desktop/plugins/folder/foldermodel.cpp containments/desktop/plugins/folder/foldermodel.h containments/desktop/plugins/folder/folderplugin.cpp containments/desktop/plugins/folder/screenmapper.cpp containments/desktop/plugins/folder/screenmapper.h 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
D8493: Make Folder View screen aware
amantia added inline comments. INLINE COMMENTS > mwolff wrote in screenmapper.cpp:108 > `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? I don't understand the problem. Yes m_itemsOnDisabledScreensMap is storing the names (well, paths) from m_screenItemMap. They will be in a format like file://foo or desktop://foo or similar. screenPathWithScheme is the base path for the screen which is either file://SOMEPATH (or some other scheme) or desktop:/ . The check is to see if the items from the disabled screen map are under this path or not. I don't see where we can get relative paths. I might miss something though, but I don't know what. :) > mwolff wrote in screenmapper.cpp:140 > is `name` also a `path`? Basically yes, it is a path with scheme, but I renamed from "url" to "name" to be clear this is not a QUrl. I can change to path, although that is not exactly correct either, as the scheme is included. 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
D8493: Make Folder View screen aware
amantia updated this revision to Diff 22428. amantia marked 2 inline comments as done. amantia added a comment. Add a code comment REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8493?vs=22387=22428 BRANCH master REVISION DETAIL https://phabricator.kde.org/D8493 AFFECTED FILES containments/desktop/package/contents/config/main.xml containments/desktop/package/contents/ui/FolderView.qml containments/desktop/package/contents/ui/FolderViewLayer.qml containments/desktop/plugins/folder/CMakeLists.txt containments/desktop/plugins/folder/autotests/CMakeLists.txt containments/desktop/plugins/folder/autotests/foldermodeltest.cpp containments/desktop/plugins/folder/autotests/foldermodeltest.h containments/desktop/plugins/folder/autotests/screenmappertest.cpp containments/desktop/plugins/folder/autotests/screenmappertest.h containments/desktop/plugins/folder/foldermodel.cpp containments/desktop/plugins/folder/foldermodel.h containments/desktop/plugins/folder/folderplugin.cpp containments/desktop/plugins/folder/screenmapper.cpp containments/desktop/plugins/folder/screenmapper.h 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
D8493: Make Folder View screen aware
amantia added inline comments. INLINE COMMENTS > mwolff wrote in screenmapper.cpp:41 > 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? Done. InvokeMethod would not help, as the idea here is to get only one screenMappingChanged instead of e.g 50 when FolderModel gets the entries on startup. 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
D8493: Make Folder View screen aware
amantia updated this revision to Diff 22387. amantia marked 2 inline comments as done. amantia added a comment. Fix for some of Milian's comments (rest later, need to move to different computer) REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8493?vs=22344=22387 BRANCH master REVISION DETAIL https://phabricator.kde.org/D8493 AFFECTED FILES containments/desktop/package/contents/config/main.xml containments/desktop/package/contents/ui/FolderView.qml containments/desktop/package/contents/ui/FolderViewLayer.qml containments/desktop/plugins/folder/CMakeLists.txt containments/desktop/plugins/folder/autotests/CMakeLists.txt containments/desktop/plugins/folder/autotests/foldermodeltest.cpp containments/desktop/plugins/folder/autotests/foldermodeltest.h containments/desktop/plugins/folder/autotests/screenmappertest.cpp containments/desktop/plugins/folder/autotests/screenmappertest.h containments/desktop/plugins/folder/foldermodel.cpp containments/desktop/plugins/folder/foldermodel.h containments/desktop/plugins/folder/folderplugin.cpp containments/desktop/plugins/folder/screenmapper.cpp containments/desktop/plugins/folder/screenmapper.h 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
D8493: Make Folder View screen aware
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, ::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 );`
> screenmapper.cpp:108
> +for (const auto : 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 , int screen,
> MappingSignalBehavior behavior)
> +{
is `name` also a `path`?
> screenmapper.cpp:150
> +
> +void ScreenMapper::removeFromMap(const QString )
> +{
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
D8493: Make Folder View screen aware
amantia updated this revision to Diff 22344. amantia added a comment. Restoring previous version overwritten by mistake. REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8493?vs=22342=22344 BRANCH master REVISION DETAIL https://phabricator.kde.org/D8493 AFFECTED FILES containments/desktop/package/contents/config/main.xml containments/desktop/package/contents/ui/FolderView.qml containments/desktop/package/contents/ui/FolderViewLayer.qml containments/desktop/plugins/folder/CMakeLists.txt containments/desktop/plugins/folder/autotests/CMakeLists.txt containments/desktop/plugins/folder/autotests/foldermodeltest.cpp containments/desktop/plugins/folder/autotests/foldermodeltest.h containments/desktop/plugins/folder/autotests/screenmappertest.cpp containments/desktop/plugins/folder/autotests/screenmappertest.h containments/desktop/plugins/folder/foldermodel.cpp containments/desktop/plugins/folder/foldermodel.h containments/desktop/plugins/folder/folderplugin.cpp containments/desktop/plugins/folder/screenmapper.cpp containments/desktop/plugins/folder/screenmapper.h 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
D8493: Make Folder View screen aware
amantia updated this revision to Diff 22342. amantia added a comment. - WIP: Multiscreen drag and drop support in folder view REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8493?vs=22313=22342 BRANCH arcpatch-D8598_1 REVISION DETAIL https://phabricator.kde.org/D8493 AFFECTED FILES containments/desktop/package/contents/config/main.xml containments/desktop/package/contents/ui/FolderView.qml containments/desktop/package/contents/ui/FolderViewLayer.qml containments/desktop/plugins/folder/CMakeLists.txt containments/desktop/plugins/folder/autotests/CMakeLists.txt containments/desktop/plugins/folder/autotests/foldermodeltest.cpp containments/desktop/plugins/folder/autotests/foldermodeltest.h containments/desktop/plugins/folder/autotests/screenmappertest.cpp containments/desktop/plugins/folder/autotests/screenmappertest.h containments/desktop/plugins/folder/foldermodel.cpp containments/desktop/plugins/folder/foldermodel.h containments/desktop/plugins/folder/folderplugin.cpp containments/desktop/plugins/folder/screenmapper.cpp containments/desktop/plugins/folder/screenmapper.h 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
D8493: Make Folder View screen aware
amantia updated this revision to Diff 22313. amantia marked 2 inline comments as done. amantia added a comment. Implement changes requested by anthonyfieroni REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8493?vs=22312=22313 BRANCH master REVISION DETAIL https://phabricator.kde.org/D8493 AFFECTED FILES containments/desktop/package/contents/config/main.xml containments/desktop/package/contents/ui/FolderView.qml containments/desktop/package/contents/ui/FolderViewLayer.qml containments/desktop/plugins/folder/CMakeLists.txt containments/desktop/plugins/folder/autotests/CMakeLists.txt containments/desktop/plugins/folder/autotests/foldermodeltest.cpp containments/desktop/plugins/folder/autotests/foldermodeltest.h containments/desktop/plugins/folder/autotests/screenmappertest.cpp containments/desktop/plugins/folder/autotests/screenmappertest.h containments/desktop/plugins/folder/foldermodel.cpp containments/desktop/plugins/folder/foldermodel.h containments/desktop/plugins/folder/folderplugin.cpp containments/desktop/plugins/folder/screenmapper.cpp containments/desktop/plugins/folder/screenmapper.h 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
D8493: Make Folder View screen aware
amantia updated this revision to Diff 22312. amantia added a comment. Handle the case when the folder view's path changes on one of the desktops REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8493?vs=21838=22312 BRANCH master REVISION DETAIL https://phabricator.kde.org/D8493 AFFECTED FILES containments/desktop/package/contents/config/main.xml containments/desktop/package/contents/ui/FolderView.qml containments/desktop/package/contents/ui/FolderViewLayer.qml containments/desktop/plugins/folder/CMakeLists.txt containments/desktop/plugins/folder/autotests/CMakeLists.txt containments/desktop/plugins/folder/autotests/foldermodeltest.cpp containments/desktop/plugins/folder/autotests/foldermodeltest.h containments/desktop/plugins/folder/autotests/screenmappertest.cpp containments/desktop/plugins/folder/autotests/screenmappertest.h containments/desktop/plugins/folder/foldermodel.cpp containments/desktop/plugins/folder/foldermodel.h containments/desktop/plugins/folder/folderplugin.cpp containments/desktop/plugins/folder/screenmapper.cpp containments/desktop/plugins/folder/screenmapper.h 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
D8493: Make Folder View screen aware
amantia added inline comments. INLINE COMMENTS > anthonyfieroni wrote in foldermodel.cpp:163-164 > When QObject dies it's disconnected to all signal/slots. In this case if you > want to not notify FolderModel you can use > > m_screenMapper->disconnect(this); Yes, I know, indeed this might be more clear. > anthonyfieroni wrote in screenmapper.cpp:32-36 > When you use singleton it's better to make variable construction in one line > > static ScreenMapper *s_instance = new ScreenMapper(); > return s_instance; > > or > > static ScreenMapper s_instance; > return _instance; > > In this way you don't have unwanted check for creation and variable at class > scope. Ok, although I don't see this commonly used in KDE (or Qt). If you really want, I can change it of course. 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
D8493: Make Folder View screen aware
anthonyfieroni added inline comments.
INLINE COMMENTS
> foldermodel.cpp:163-164
> +if (m_screenMapper) {
> +disconnect(m_screenMapper, ::screensChanged, this,
> ::invalidateFilter);
> +disconnect(m_screenMapper, ::screenMappingChanged,
> this, ::invalidateFilter);
> +m_screenMapper->removeScreen(m_screen, url());
When QObject dies it's disconnected to all signal/slots. In this case if you
want to not notify FolderModel you can use
m_screenMapper->disconnect(this);
> foldermodel.cpp:1673-1674
> +{
> +if (m_screenMapper == screenMapper)
> +return;
> +
{} braces even on one line, done it on all places.
> screenmapper.cpp:32-36
> +if (s_instance)
> +return s_instance;
> +
> +s_instance = new ScreenMapper();
> +return s_instance;
When you use singleton it's better to make variable construction in one line
static ScreenMapper *s_instance = new ScreenMapper();
return s_instance;
or
static ScreenMapper s_instance;
return _instance;
In this way you don't have unwanted check for creation and variable at class
scope.
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
D8493: Make Folder View screen aware
amantia updated this revision to Diff 21838. amantia added a comment. remove unimplemented method REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8493?vs=21837=21838 BRANCH master REVISION DETAIL https://phabricator.kde.org/D8493 AFFECTED FILES containments/desktop/package/contents/config/main.xml containments/desktop/package/contents/ui/FolderView.qml containments/desktop/package/contents/ui/FolderViewLayer.qml containments/desktop/plugins/folder/CMakeLists.txt containments/desktop/plugins/folder/autotests/CMakeLists.txt containments/desktop/plugins/folder/autotests/foldermodeltest.cpp containments/desktop/plugins/folder/autotests/foldermodeltest.h containments/desktop/plugins/folder/autotests/screenmappertest.cpp containments/desktop/plugins/folder/autotests/screenmappertest.h containments/desktop/plugins/folder/foldermodel.cpp containments/desktop/plugins/folder/foldermodel.h containments/desktop/plugins/folder/folderplugin.cpp containments/desktop/plugins/folder/screenmapper.cpp containments/desktop/plugins/folder/screenmapper.h To: amantia, #plasma, ervin, mlaurent, dvratil, hein, aacid, davidedmundson, apol, mwolff Cc: ngraham, mwolff, davidedmundson, broulik, mart, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
D8493: Make Folder View screen aware
amantia updated this revision to Diff 21837. amantia added a comment. - add unit tests for different scenarios of foldermodel and screenmapper usage on multiple screens - connect screenMappingChanged to invalidateFilter - as addMapping is called from withing filterAcceptRows, make it possible to emit screenMappingChanged in a delayed way and compressed to avoid multiple and recursive calls to filterAcceptRows - do not call invalidateFilter from setScreen as it is called via screensChanged - store firstscreen usage per configured folderview path. This is needed if the two screens are configured to show different folders. There is a unit test for it, but needs to be tested also visually (quick test shows some problems I need to debug before this is accepted). REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8493?vs=21711=21837 BRANCH master REVISION DETAIL https://phabricator.kde.org/D8493 AFFECTED FILES containments/desktop/package/contents/config/main.xml containments/desktop/package/contents/ui/FolderView.qml containments/desktop/package/contents/ui/FolderViewLayer.qml containments/desktop/plugins/folder/CMakeLists.txt containments/desktop/plugins/folder/autotests/CMakeLists.txt containments/desktop/plugins/folder/autotests/foldermodeltest.cpp containments/desktop/plugins/folder/autotests/foldermodeltest.h containments/desktop/plugins/folder/autotests/screenmappertest.cpp containments/desktop/plugins/folder/autotests/screenmappertest.h containments/desktop/plugins/folder/foldermodel.cpp containments/desktop/plugins/folder/foldermodel.h containments/desktop/plugins/folder/folderplugin.cpp containments/desktop/plugins/folder/screenmapper.cpp containments/desktop/plugins/folder/screenmapper.h To: amantia, #plasma, ervin, mlaurent, dvratil, hein, aacid, davidedmundson, apol, mwolff Cc: ngraham, mwolff, davidedmundson, broulik, mart, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
D8493: Make Folder View screen aware
mwolff requested changes to this revision.
mwolff added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> foldermodel.cpp:535
> +m_screenMapper->addScreen(screen);
> +invalidateFilter();
> +}
this should not be required, since `addScreen` calls `screensChanged` and that
is connected to `invalidateFilter` already.
> foldermodel.cpp:1678
> +if (m_screenMapper) {
> +connect(m_screenMapper, ::screensChanged, this,
> ::invalidateFilter);
> +}
this should also connect to `screenMappingChanged`. Once we handle DND across
views, we will update the mapping to point to a different screen. As a
reaction, both `FolderModel` instances (source and target of the DND action)
need to update their filter. The easiest is via this signal.
REPOSITORY
R119 Plasma Desktop
REVISION DETAIL
https://phabricator.kde.org/D8493
To: amantia, #plasma, ervin, mlaurent, dvratil, hein, aacid, davidedmundson,
apol, mwolff
Cc: ngraham, mwolff, davidedmundson, broulik, mart, plasma-devel, ZrenBot,
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
D8493: Make Folder View screen aware
amantia updated this revision to Diff 21711. amantia added a comment. Added unit test for ScreenMapper and improve the code to cover all the cases tested. Added asserts instead of disconnects. Change the ScreenMapper API completely to QString (from QUrl) as that is what is more natural (we store QStrings anyway in the config file). REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8493?vs=21703=21711 BRANCH master REVISION DETAIL https://phabricator.kde.org/D8493 AFFECTED FILES containments/desktop/package/contents/config/main.xml containments/desktop/package/contents/ui/FolderView.qml containments/desktop/package/contents/ui/FolderViewLayer.qml containments/desktop/plugins/folder/CMakeLists.txt containments/desktop/plugins/folder/autotests/CMakeLists.txt containments/desktop/plugins/folder/autotests/screenmappertest.cpp containments/desktop/plugins/folder/autotests/screenmappertest.h containments/desktop/plugins/folder/foldermodel.cpp containments/desktop/plugins/folder/foldermodel.h containments/desktop/plugins/folder/folderplugin.cpp containments/desktop/plugins/folder/screenmapper.cpp containments/desktop/plugins/folder/screenmapper.h To: amantia, #plasma, ervin, mlaurent, dvratil, hein, aacid, davidedmundson, apol Cc: ngraham, mwolff, davidedmundson, broulik, mart, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
D8493: Make Folder View screen aware
amantia edited the summary of this revision. amantia added dependencies: D8567: Emit signals when a screen is added or removed, D8566: Add API to retrieve the screen id for a screen name. REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D8493 To: amantia, #plasma, ervin, mlaurent, dvratil, hein, aacid, davidedmundson, apol Cc: ngraham, mwolff, davidedmundson, broulik, mart, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
D8493: Make Folder View screen aware
mwolff added inline comments. INLINE COMMENTS > amantia wrote in foldermodel.cpp:1671 > Done, although the screenmapper will never change. an assert would be valid in such a scenario > amantia wrote in foldermodel.cpp:1704 > From my understanding, this setAppletInterface is called only once when the > applet is created. I can do the disconnect, but imo that only pollutes the > code. then add an `Q_ASSERT(!m_appletInterface)`, this documents your intent and does not pollute the code > amantia wrote in screenmapper.cpp:97 > Done, but as in the other cases, this should be called only once. use an assert then REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D8493 To: amantia, #plasma, ervin, mlaurent, dvratil, hein, aacid, davidedmundson, apol Cc: ngraham, mwolff, davidedmundson, broulik, mart, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
D8493: Make Folder View screen aware
amantia edited the summary of this revision. 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
D8493: Make Folder View screen aware
amantia updated this revision to Diff 21703. amantia added a comment. Fix Milian's comments, simplify/merge ScreenMapper add/remove screen methods. Treat folderview destruction as screen removal, as it has the same effects. Makes the code work correctly when the containment changes from Folder View to Desktop and vice versa. REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8493?vs=21696=21703 BRANCH master REVISION DETAIL https://phabricator.kde.org/D8493 AFFECTED FILES containments/desktop/package/contents/config/main.xml containments/desktop/package/contents/ui/FolderView.qml containments/desktop/package/contents/ui/FolderViewLayer.qml containments/desktop/plugins/folder/CMakeLists.txt containments/desktop/plugins/folder/foldermodel.cpp containments/desktop/plugins/folder/foldermodel.h containments/desktop/plugins/folder/folderplugin.cpp containments/desktop/plugins/folder/screenmapper.cpp containments/desktop/plugins/folder/screenmapper.h 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
D8493: Make Folder View screen aware
amantia added inline comments. INLINE COMMENTS > mwolff wrote in foldermodel.cpp:1671 > missing disconnect on the old mapper for the invalidate filter Done, although the screenmapper will never change. > mwolff wrote in foldermodel.cpp:1704 > when the interface changes, don't you need to disconnect the old containment > here? >From my understanding, this setAppletInterface is called only once when the >applet is created. I can do the disconnect, but imo that only pollutes the >code. > mwolff wrote in screenmapper.cpp:90 > `return m_firstScreen`, no? faster, and your code would crash if there is no > screen available Couldn't use that in this version, but in the updated one I fixed, so now m_firstScreen is always correct and could just return it. > mwolff wrote in screenmapper.cpp:97 > missing disconnect on old m_corona, if valid Done, but as in the other cases, this should be called only once. 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
D8493: Make Folder View screen aware
mwolff added inline comments.
INLINE COMMENTS
> foldermodel.cpp:1671
> +return;
> +
> +m_screenMapper = screenMapper;
missing disconnect on the old mapper for the invalidate filter
> foldermodel.cpp:1704
> +setScreen(containment->screen());
> +connect(containment,
> ::Containment::screenChanged, this, ::setScreen);
> +}
when the interface changes, don't you need to disconnect the old containment
here?
> screenmapper.cpp:58
> +
> +void ScreenMapper::addScreen(int screenId)
> +{
conceptually, what is the difference between addScreen and registerScreen? I
can see the code of course, but the names are super similar, and the code as
well. Can/should they share code? Most notably `m_firstScreen` isn't updated in
`addScreen`?
> screenmapper.cpp:90
> +{
> +const auto it = std::min_element(m_availableScreens.constBegin(),
> m_availableScreens.constEnd());
> +return *it;
`return m_firstScreen`, no? faster, and your code would crash if there is no
screen available
> screenmapper.cpp:97
> +if (m_corona != corona) {
> +m_corona = corona;
> +if (m_corona) {
missing disconnect on old m_corona, if valid
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
D8493: Make Folder View screen aware
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
D8493: Make Folder View screen aware
amantia edited the summary of this revision. 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
D8493: Make Folder View screen aware
amantia updated this revision to Diff 21696. amantia marked 8 inline comments as done. amantia added a comment. Handle screen associated in C++ part (no need to go through QML now), fix issues mentioned by Milian. REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8493?vs=21685=21696 BRANCH master REVISION DETAIL https://phabricator.kde.org/D8493 AFFECTED FILES containments/desktop/package/contents/config/main.xml containments/desktop/package/contents/ui/FolderView.qml containments/desktop/package/contents/ui/FolderViewLayer.qml containments/desktop/plugins/folder/CMakeLists.txt containments/desktop/plugins/folder/foldermodel.cpp containments/desktop/plugins/folder/foldermodel.h containments/desktop/plugins/folder/folderplugin.cpp containments/desktop/plugins/folder/screenmapper.cpp containments/desktop/plugins/folder/screenmapper.h 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
D8493: Make Folder View screen aware
mwolff added a comment.
some code review from my side
INLINE COMMENTS
> foldermodel.cpp:77
> #include
> +#include
> +#include
wrong location of include, and the KF5 prefix is wrong too, I guess?
> foldermodel.cpp:1314
> +
> +const QString name = item.url().toString();
> +if (m_usedByContainment) {
move into conditional below, it's only used there
> foldermodel.cpp:1316
> +if (m_usedByContainment) {
> +const int screen = m_screenMapper->screenForUrl(name);
> +if (m_screen != -1) {
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?
> foldermodel.cpp:1689
> +Plasma::Applet *applet =
> appletInterface->property("_plasma_applet").value();
> +Plasma::Containment *containment = applet->containment();
> +
if the above fails for any reason, this will crash. Shouldn't you check the
value first?
> foldermodel.cpp:1691
> +
> +if (containment) {
> +Plasma::Corona *corona = containment->corona();
shorten this to:
if (containment && containment->corona())
m_screenMapper->registerCorona(containment->corona());
? Or is asking for the corona too expensive?
> foldermodel.h:191
> +
> +ScreenMapper* screenMapper() const;
> +void setScreenMapper(ScreenMapper* screenMapper);
unused, remove? also, the mapper is now a singleton? so why have it as a member
here, too?
> screenmapper.cpp:5
> + * Author: Andras Mantia *
> + * Work sponsored by the LiMux project of the city of Munich.*
> *
> + * This program is free software; you can redistribute it and/or modify *
dito, broken *
> screenmapper.cpp:30
> +
> +ScreenMapper *ScreenMapper::instance() {
> +if (s_instance)
{ on its own line
> screenmapper.cpp:92
> +
> +void ScreenMapper::registerCorona(Plasma::Corona *corona)
> +{
`register` sounds like this can be called multiple times and you'll remember
all registered coronas. But it seems like this is actually a `setCorona`, no?
Only the last called corona will be used after all
> screenmapper.cpp:96
> +m_corona = corona;
> +m_availableScreens.clear();
> +if (m_corona) {
shouldn't you reset m_firstScreen and potentially the other maps, too?
> screenmapper.cpp:121
> +QHash newMap;
> +const int count = mapping.count();
> +for (int i = 0; i < count - 1; i += 2) {
count -> size
newMap.reserve(size / 2);
> screenmapper.cpp:123
> +for (int i = 0; i < count - 1; i += 2) {
> +if ( i + 1 < count) {
> +newMap[mapping[i]] = mapping[i + 1].toInt();
remove space after (
> screenmapper.h:5
> + * Author: Andras Mantia *
> + * Work sponsored by the LiMux project of the city of Munich.*
> *
> + * *
that * at the very end is off
> screenmapper.h:47
> +
> +int screenForUrl(const QString ) const;
> +Q_INVOKABLE void addMapping(const QString , int screen);
a `screenForUrl` taking a `QString url` just asks for trouble, similar below.
If it's a URL, shouldn't it take `QUrl`?
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
D8493: Make Folder View screen aware
amantia updated this revision to Diff 21685. amantia added a comment. Use the plasma signals with screen id instead of QGuiApplication signals with the QScreen objects. REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8493?vs=21619=21685 BRANCH master REVISION DETAIL https://phabricator.kde.org/D8493 AFFECTED FILES containments/desktop/package/contents/config/main.xml containments/desktop/package/contents/ui/FolderView.qml containments/desktop/package/contents/ui/FolderViewLayer.qml containments/desktop/plugins/folder/CMakeLists.txt containments/desktop/plugins/folder/foldermodel.cpp containments/desktop/plugins/folder/foldermodel.h containments/desktop/plugins/folder/folderplugin.cpp containments/desktop/plugins/folder/screenmapper.cpp containments/desktop/plugins/folder/screenmapper.h To: amantia, #plasma, ervin, mlaurent, dvratil, hein, aacid, davidedmundson, apol Cc: davidedmundson, broulik, mart, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
D8493: Make Folder View screen aware
amantia added a reviewer: apol. REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D8493 To: amantia, #plasma, ervin, mlaurent, dvratil, hein, aacid, davidedmundson, apol Cc: davidedmundson, broulik, mart, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
D8493: Make Folder View screen aware
amantia added a comment. I have changed the code to not rely on screen removal/addition signals inside the FolderView plugin, but use the existing code from shellcorona. This also ensures that we get the signals for the right id, and works correctly as long as the screen layout detection in shellcorona detection works properly. What I tested: - removal of a secondary screen from the right: the removal for id == 2 is emited - moving the secondary screen so it slightly overlaps with the primary: no removal signal is emited - moving the secondary screen so it completely overlaps, but it is aligned to the right of the primary screen (my secondary screen has smaller resolution): removal for id == 2 is emited - moving the secondary screen so it completely overlaps, but it is left/top aligned: removal for id == 2 is emited - moving the secondary screen so it is a little bit left of the primary, but mostly overlaps: no removal signal is emited. This is actually a problem as visually I see only the secondary screen on both monitors, but there is no removal signal for the primary screen (id == 0), thus plasma and folderview thinks the primary screen is still available. I consider this a bug in shellcorona, although probably it is a corner-case. For all cases I tested if the add signal arrives correctly when the secondary screen is moved back to the right of the primary one, and it does. Do you think this is acceptable? I have no idea about how wayland works, but if plasma shell does the right thing, the folderview should also do the right thing as well. REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D8493 To: amantia, #plasma, ervin, mlaurent, dvratil, hein, aacid, davidedmundson Cc: davidedmundson, broulik, mart, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
D8493: Make Folder View screen aware
davidedmundson requested changes to this revision.
davidedmundson added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> screenmapper.cpp:65
> +
> +void ScreenMapper::handleScreenAdded(QScreen *screen)
> +{
I don't think you want to track QScreens.
We need to handle the case of overlapping (cloned) screens.
You'll have two QScreen objects, two screen pool IDs, but only one desktop
view. (and thus only one folder view).
If we were previously spanned, and then go into clone, we need Folderview to
act like there's only one screen attached. I don't think this will.
We need the same logic here, that ShellCorona has for creating DesktopViews.
This either means we do it like you're doing, then copy and paste
ShellCorona::isOutputRedundant here.
or the much neater solution for your problem, would be to move the overlap
detection from ShellCorona to ScreenPool, then use that as the canonical source
of screens attached which you can then use here with it all in sync.
(I sort of started that in the p-w branch davidedmundson/screenpool_changes)
REPOSITORY
R119 Plasma Desktop
REVISION DETAIL
https://phabricator.kde.org/D8493
To: amantia, #plasma, ervin, mlaurent, dvratil, hein, aacid, davidedmundson
Cc: davidedmundson, broulik, mart, plasma-devel, ZrenBot, progwolff,
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
D8493: Make Folder View screen aware
amantia edited the summary of this revision. REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D8493 To: amantia, #plasma, ervin, mlaurent, dvratil, hein, aacid Cc: broulik, mart, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
D8493: Make Folder View screen aware
amantia edited the summary of this revision. REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D8493 To: amantia, #plasma, ervin, mlaurent, dvratil, hein, aacid Cc: broulik, mart, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
D8493: Make Folder View screen aware
mlaurent added inline comments. INLINE COMMENTS > foldermodel.h:192 > +ScreenMapper* screenMapper() const; > +void setScreenMapper(ScreenMapper* screenMapper); > + Coding style "space before *" REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D8493 To: amantia, #plasma, ervin, mlaurent, dvratil, hein, aacid Cc: broulik, mart, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
D8493: Make Folder View screen aware
amantia updated this revision to Diff 21619. amantia added a comment. Basic handling of screen removal/addition. REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8493?vs=21447=21619 BRANCH master REVISION DETAIL https://phabricator.kde.org/D8493 AFFECTED FILES containments/desktop/package/contents/config/main.xml containments/desktop/package/contents/ui/FolderView.qml containments/desktop/package/contents/ui/FolderViewLayer.qml containments/desktop/plugins/folder/CMakeLists.txt containments/desktop/plugins/folder/foldermodel.cpp containments/desktop/plugins/folder/foldermodel.h containments/desktop/plugins/folder/folderplugin.cpp containments/desktop/plugins/folder/screenmapper.cpp containments/desktop/plugins/folder/screenmapper.h To: amantia, #plasma, ervin, mlaurent, dvratil, hein, aacid Cc: broulik, mart, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
D8493: Make Folder View screen aware
amantia updated this revision to Diff 21447. amantia added a comment. Make ScreenMapper a QML singleton, address the other issues mentioned by Kai-Uwe. CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8493?vs=21365=21447 REVISION DETAIL https://phabricator.kde.org/D8493 AFFECTED FILES containments/desktop/package/contents/config/main.xml containments/desktop/package/contents/ui/FolderView.qml containments/desktop/package/contents/ui/FolderViewLayer.qml containments/desktop/plugins/folder/CMakeLists.txt containments/desktop/plugins/folder/foldermodel.cpp containments/desktop/plugins/folder/foldermodel.h containments/desktop/plugins/folder/folderplugin.cpp containments/desktop/plugins/folder/screenmapper.cpp containments/desktop/plugins/folder/screenmapper.h To: amantia, #plasma, ervin, mlaurent, dvratil, hein, aacid Cc: broulik, mart, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
D8493: Make Folder View screen aware
broulik added inline comments. INLINE COMMENTS > amantia wrote in foldermodel.cpp:1314 > It can be and it is reached. m_screen != -1 means it is a containment, screen > == -1 means there is no screen information stored yet. This happens e.g when > using the updated code with an existing plasma-*appletsrc file. Sorry, I misread both as `m_screen` > amantia wrote in foldermodel.cpp:1659 > This is not a cast between library boundaries, but I don't mind to use > qobject_cast either. It's faster. > amantia wrote in screenmapper.cpp:51 > I need both the key and the value and AFAIK it is not supported in current Qt > version. Right. Again, misread something :) REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D8493 To: amantia, #plasma, ervin, mlaurent, dvratil, hein, aacid Cc: broulik, mart, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
D8493: Make Folder View screen aware
amantia added a comment. I've added some comments, I will address the ones I did not comment in a follow-up patch. INLINE COMMENTS > broulik wrote in main.xml:40 > What's the purpose of `hidden=true`? AFAIK it is not used in the GUI config. See also the others in the same file. > broulik wrote in foldermodel.cpp:1310 > Why not store a `QUrl`? Mostly due to the hash and also because for the configuration it anyway needs to be turned into a string (and it is the same as the Positioner does). > broulik wrote in foldermodel.cpp:1314 > This can never be reached It can be and it is reached. m_screen != -1 means it is a containment, screen == -1 means there is no screen information stored yet. This happens e.g when using the updated code with an existing plasma-*appletsrc file. > broulik wrote in foldermodel.cpp:1659 > Use `qobject_cast` (but this would not be neccessary, see my comment on the > `Q_PROPERTY` above) This is not a cast between library boundaries, but I don't mind to use qobject_cast either. > broulik wrote in folderplugin.cpp:66 > Using this is asking for trouble. Plasma uses shared engines for all applets, > so this context property would be accessible to anyone. I would prefer that > you used a singleton type. I was thinking about using a singleton, this pushed me towards it. :) > broulik wrote in screenmapper.cpp:51 > Range-for? I need both the key and the value and AFAIK it is not supported in current Qt version. > broulik wrote in screenmapper.h:21 > You can use `#pragma once` Not common in KDE code though (at least wasn't earlier when I was more involved and can't find anything like that in plasma-desktop) REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D8493 To: amantia, #plasma, ervin, mlaurent, dvratil, hein, aacid Cc: broulik, mart, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
D8493: Make Folder View screen aware
broulik added a comment.
Regardless of whether this is the right way to go, I put some coding style
and practise comments on the code, so you know what to look for in the future /
in general :)
INLINE COMMENTS
> main.xml:40
>
> +
> +
What's the purpose of `hidden=true`?
> foldermodel.cpp:1310
> +
> +const QString name = item.url().toString();
> +if (m_usedByContainment) {
Why not store a `QUrl`?
> foldermodel.cpp:1314
> +if (m_screen != -1) {
> +if (screen == -1)
> +m_screenMapper->addMapping(name, m_screen);
This can never be reached
> foldermodel.cpp:1316
> +m_screenMapper->addMapping(name, m_screen);
> +else if (m_screen != screen)
> +return false;
Coding style: one space between `else` and `if`, also put braces even around
one line statements.
> foldermodel.cpp:1659
> +{
> +const auto mapper = dynamic_cast(screenMapper);
> +if (!mapper || m_screenMapper == mapper)
Use `qobject_cast` (but this would not be neccessary, see my comment on the
`Q_PROPERTY` above)
> foldermodel.h:100
> +Q_PROPERTY(int screen READ screen WRITE setScreen NOTIFY screenChanged)
> +Q_PROPERTY(QObject* screenMapper READ screenMapper WRITE setScreenMapper
> NOTIFY screenMapperChanged)
>
You can use `ScreenMapper *` as property type if you did
`qmlRegisterType()` on it.
Also note that QML engine sucks at resolving namespaces, so you might need to
fully qualify it should it be in a namespace.
> foldermodel.h:190
> +
> +QObject* screenMapper() const;
> +void setScreenMapper(QObject* screenMapper);
Coding style: space //before// `*`, ie `QObject *foo()`
> folderplugin.cpp:66
>
> +void FolderPlugin::initializeEngine(QQmlEngine *engine, const char *uri)
> +{
Using this is asking for trouble. Plasma uses shared engines for all applets,
so this context property would be accessible to anyone. I would prefer that you
used a singleton type.
> screenmapper.cpp:50
> +{
> +QStringList result;
> +auto it = m_screenUrlMap.constBegin();
Add `reserve()` call, ie `result.reserve(screenUrlMap.count() * 2)`
> screenmapper.cpp:51
> +QStringList result;
> +auto it = m_screenUrlMap.constBegin();
> +while (it != m_screenUrlMap.constEnd()) {
Range-for?
> screenmapper.cpp:79
> +{
> +if (m_screenUrlMap.contains(url)) {
> +return m_screenUrlMap[url];
Avoid double look-up:
return m_screenUrlMap.value(url, -1);
the second argument is the default value to return if the item does not exist.
> screenmapper.h:21
> + ***/
> +#ifndef SCREENMAPPER_H
> +#define SCREENMAPPER_H
You can use `#pragma once`
> screenmapper.h:34
> +explicit ScreenMapper(QObject *parent = nullptr);
> +
> +QStringList screenMapping() const;
We typically explicitly add a default destructor.
~ScreenMapper() override = default;
REPOSITORY
R119 Plasma Desktop
REVISION DETAIL
https://phabricator.kde.org/D8493
To: amantia, #plasma, ervin, mlaurent, dvratil, hein, aacid
Cc: broulik, mart, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed,
jensreuterberg, abetts, sebas, apol
D8493: Make Folder View screen aware
amantia added a comment. Just to make clear, this is the first step and would not commit before the problems with display removing/additions are solved. The ideas for doing that are: - keep per screen configuration and per desktop setting: e.g a different set of position for icons if one, two, three screen are used - keep per desktop configuration, but not per screen combinaton: if a display is removed, the items from there are moved to a visible display, although their positions are remembered so when it is plugged back, it goes to the right place. If meantime the user rearranged the icons on the visible display, the configuration for the non-visible screens is forgotten. REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D8493 To: amantia, #plasma, ervin, mlaurent, dvratil, hein, aacid Cc: mart, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
D8493: Make Folder View screen aware
mart added a comment. I think this is really asking for a lot of trouble. what happens when a screen is dinamically added/removed? REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D8493 To: amantia, #plasma, ervin, mlaurent, dvratil, hein, aacid Cc: mart, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
D8493: Make Folder View screen aware
amantia created this revision. amantia added reviewers: Plasma, ervin, mlaurent, dvratil, hein, aacid. Restricted Application added a project: Plasma. Restricted Application added a subscriber: plasma-devel. REVISION SUMMARY When using multiple screen with a Folder View as a desktop containment, we need to make sure a file belongs only to one screen. The patch adds support for this by: 1. Introducing a ScreenMapper object shared acrossed all Folder Views 2. FolderModel registers the screen it resides on if used as a containment 3. FolderModel filters out items not on the current screen (if used as a containment) 4. FolderModel adds mapping for the newly appeared files. The new files will go to the registered screen having the smallest id. This also means by default all files appear on the screen with the smallest id. 5. url/screen mapping is stored in the configuration of each folder view applet. This means duplication of the information, and although they should be in sync unless manually modified, the logic is that the last view's mapping option is used. REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D8493 AFFECTED FILES containments/desktop/package/contents/config/main.xml containments/desktop/package/contents/ui/FolderView.qml containments/desktop/package/contents/ui/FolderViewLayer.qml containments/desktop/plugins/folder/CMakeLists.txt containments/desktop/plugins/folder/foldermodel.cpp containments/desktop/plugins/folder/foldermodel.h containments/desktop/plugins/folder/folderplugin.cpp containments/desktop/plugins/folder/folderplugin.h containments/desktop/plugins/folder/screenmapper.cpp containments/desktop/plugins/folder/screenmapper.h To: amantia, #plasma, ervin, mlaurent, dvratil, hein, aacid Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
