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
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,
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,
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,
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
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
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
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 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
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 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,
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
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
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 =
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
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
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
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?
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
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
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
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
> +
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
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
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
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
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
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);
> +
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
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
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`
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
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,
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
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,
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
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
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());
> +
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
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,
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
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
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
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,
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
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
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,
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,
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
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
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
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
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
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`?
>
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
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,
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
57 matches
Mail list logo
