Details
- Reviewers
mwolff hein - Group Reviewers
Plasma - Commits
- R119:cc9c3d32a381: Support drag and drop between shared folder view containments
Diff Detail
- Repository
- R119 Plasma Desktop
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Fix problem of having ghost items,
fix problem of losing items when an item is moved to a screen and back.
Add unit test for the positioner when items are moved between screens.
Problem: moving multiple items works, but the items are positioned wrongly.
andras, please comandeer this change. and I feel your pain re arc usability... phabricator with patch series is a nightmare for developers
| containments/desktop/package/contents/ui/FolderViewLayer.qml | ||
|---|---|---|
| 192 ↗ | (On Diff #22630) | looks like you send the full diff of your branch, use arc diff HEAD^ instead to only send one patch |
I guess we should split this patch up into three parts:
a) add unit test + the fix you found
b) (if still required) enforce uniqueness in Positoiner::updateMaps + unit test
c) support drag and drop
| containments/desktop/plugins/folder/autotests/positionertest.cpp | ||
|---|---|---|
| 219 ↗ | (On Diff #22631) | second? this is the first and only one in this test, no? |
| 229 ↗ | (On Diff #22631) | QVERIFY the wait, also below |
| 232 ↗ | (On Diff #22631) | why not use hashes here too, then you can compare those below directly without having to convert the returned hash to a map first? |
| 261 ↗ | (On Diff #22631) | verify |
| 282 ↗ | (On Diff #22631) | verify |
| containments/desktop/plugins/folder/autotests/positionertest.h | ||
| 58 ↗ | (On Diff #22631) | could be a free function, no need to make this a member |
| containments/desktop/plugins/folder/positioner.cpp | ||
| 28 | do we still need this? I don't think so - I added this only for debugging purposes. should probably be part of a unit test now | |
| 450 | should also be removed and covered by a unit test instead I think | |
| 694 | this may be obsoleted by now, can you check whether it's still required? | |
Clean unit test and code: move out the unique testing part (will be moved to a new patch)
| containments/desktop/plugins/folder/autotests/positionertest.cpp | ||
|---|---|---|
| 219 ↗ | (On Diff #22631) | There is one coming from init() (m_folderModel) |
| 229 ↗ | (On Diff #22631) | Changed, the rest is actually not needed (no folder parsing is performed, just the filter changes) |
| 232 ↗ | (On Diff #22631) | Right, I will change, makes sense. The original idea for the map was that I can more easily read that as it is sorted. |
| containments/desktop/plugins/folder/autotests/positionertest.h | ||
| 58 ↗ | (On Diff #22631) | I removed it. |
| containments/desktop/plugins/folder/positioner.cpp | ||
| 28 | I was wondering about it, especially that I don't like asserts :) I will remove them. | |
| 450 | Ok. | |
| 694 | I think it is still needed, but I will check. | |
- More stricter detection for moving between screens to fix issue
when a file is moved/copied out from within a folder of the desktop.
- Enable the code only when the folder view is used in a containment
Bugs to fix:
- d&d from dolphin to a secondary screen drops the item to the first screen
- d&d multiple items between the screen positions them in wrong place
Drop the items to the right screen when dragging from outside of the folder view.
I admit it is not the nicest code for finding the final URL (that will be returned by
KDirModel), ideas are welcome to improve it.
| containments/desktop/plugins/folder/foldermodel.cpp | ||
|---|---|---|
| 1126 | don't we need to check something like targetUrl.toString().startsWith(dropTargetUrl.toString()) like above? I.e. including schema and so forth? This could otherwise match for file://foo/bar vs. sftp://host/foo/bar, no? | |
- Improve path detection as suggested by Milian
- fix d&d for multiple items. There is only one issue, but I think it is acceptable this way:
when moving two or more items that are not near each other to a new screen, they will be layed
out in a row (or multiple rows) instead of keeping their original relative positions to each other,
like when it happens for moving around on the same screen.
| containments/desktop/plugins/folder/foldermodel.cpp | ||
|---|---|---|
| 1122 | The code in this patch doesn't test for m_screenMapper being set. I don't think FolderModel should require one to work. | |
lgtm, but Phab says "You can not accept this revision because you have already accepted it."