Remember the drop event target position and move the items once
they become available. This requires us to allow moving while no
drag image is being shown. Otherwise, we listen to the
copy job signals, which gets created by the drop job. This allows
us to map target filenames in their final form, i.e. after the user
potentially renamed the files to handle conflicts, to some desired
visual position. To stay on the safe side, we also periodically
cleanup the mapping after an idle timeout of 10s. This ensures we
don't grow the mapping with stale items. This is required to handle
errors in the file lister, or situations like overwriting an existing
file which would not trigger a rowAdded signal.
Details
Details
Diff Detail
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.
Comment Actions
So far, mostly food for thought. It kinda works but I see lots of problems with this approach... I'll investigate other approaches tomorrow
Comment Actions
Sound hacky and dangerous for me due to the point you mentioned. I'm not sure I have a better idea now .
Comment Actions
IRC talk for posterity:
[17:32] <milian> from the notes you sent mlaurent, and from what I remember form our past discussion, it is not clear to me how you envisioned (or even implemented) the mapping of drop action -> new file
[17:33] <milian> first of all, what happens when you move e.g. "foo" into a (different) folder that already contains a file of that name?
[17:33] <milian> the user would get the conflict dialog and either skip, or rename, the dropped file
[17:33] <Sho_> that one is easy: I didn't handle that complication yet :-)
[17:33] <milian> to me it looks like we will need to extend the dropjob extensively
[17:33] <milian> ah ok
[17:33] <Sho_> or let's put it another way, the general issue with my hack, and why it didn't get finished, was evicting the cache of expected files
[17:34] <milian> right
[17:34] <Sho_> similar to yours i was writing down notes about what to expect on drop, and then monitoring kio jobs coming in
[17:34] <milian> that could be hacked around with some timers as you suggested, but it would leave my situation above open
[17:34] <milian> i.e. a renamed file would then suddenly appear at the first empty spot
[17:34] <milian> not at the dropped spot
[17:34] <Sho_> yep, and my gut feeling is always "if you write a timer in expectation of something happening later you're most likely doing it wrong and/or being lazy"
[17:35] <milian> ++
[17:35] <Sho_> a delay-type QTimer is basically always a red flag f or me
[17:35] <milian> exactly my thoughts
[17:35] <Sho_> which is why I was very unhappy with it
[17:35] <milian> so what I'm thinking about right now is to extend the dropjob API considerably
[17:35] <milian> I haven't looked at its internals yet, but I hope it knows about everything that is going on internally
[17:35] <Sho_> extending DropJob and having some global D-Bus stuff (similar to the way that kioslaves signal file rename transactions to file managers) would be a lot better
[17:35] <fvogt> Hm, kwin_wayland is currently FUBAR, segfaults instantly on openQA
[17:36] <Sho_> so I like that you think in that direction as well
[17:36] <milian> it already contains a signal about a copied file, but that signal misses information about the dragged file
[17:36] <Sho_> one interesting thought: did kdesktop handle this?
[17:36] <milian> if that would be there, one could do a good mapping
[17:36] <Sho_> if so, how?
[17:36] <Sho_> (the old qwidget desktop in kde2/3)
[17:36] <Sho_> kio was around then, so maybe it had ideas
[17:36] <milian> while I did use kde3, I never looked at that code. I'll ask dfaure and others who might remember
[17:37] <Sho_> kio was more or less created for that icon view, so ...
[17:37] <milian> ok, but that is already the biggest questions I had, so thanks for confirming my thoughts (or fears?)
[17:37] <Sho_> maybe they knew what they wanted todo and how
Comment Actions
use copyjob signals to map the target urls, not the source urls, which
resolves a lot of the brittleness of my previous approach. overall, this
works quite nicely, imo and is already in an acceptable state (to me).
what's left is changing the folder view sort defaults and not dropping
to a target position when an explicit sorting/grouping was selected.
Comment Actions
I didn't tested the code yet, but in general looks good for me.
| containments/desktop/plugins/folder/foldermodel.cpp | ||
|---|---|---|
| 185 | Either remove or use categorized logging | |
Comment Actions
This looks quite good to me now - the fact that it's much shorter than my old attempt feels pretty nice ;)
Comment Actions
Unfortunately the current approach fails when the folderview points to desktop:/ . In that case the targetUrl in the copyJobStarted is different ("desktop:/foo") from the url we get from the dirlister model ("file://home/user/Desktop/./foo") and thus it fails to find the saved positions from the hash.
Another note is that in the "map" lambda, there is a need to call
m_screenMapper->addMapping(targetUrl.toString(), m_screen, ScreenMapper::DelayedSignal);
so it goes to the right screen (as soon as https://phabricator.kde.org/D8493 gets in). This one could be done in a followup commit though.
Comment Actions
Small comment change request in the comments, but code-wise good to go.
| containments/desktop/plugins/folder/foldermodel.cpp | ||
|---|---|---|
| 1066 | As mentioned on IRC I'd currently prefer to keep the Positioner sort of optional architecturally. The idea is that non-containment FolderViews shouldn't need or use the Positoner (though this isn't the case currently in the code - it will always use it, just with an empty mapping) as they don't support sparse positioning and can scale better. Will also be relevant for future uses of this code like the Plasma Mobile file manager. Therefore I think this TODO note should be removed. | |
| containments/desktop/plugins/folder/foldermodel.cpp | ||
|---|---|---|
| 155 | here as well then | |
Comment Actions
Milian, please also update the commit description (the last paragraph needs to go away).