Trash behaviour improvements: check for the amount of parentReference items on a fileId before processing the delete command in src/kio_gdrive.cpp
Needs ReviewPublic

Authored by martijnschmidt on Jan 9 2018, 9:32 PM.

Details

Reviewers
elvisangelaccio
Group Reviewers
KIO GDrive
Maniphest Tasks
T3443: Make Trash work again
Summary

As per the FIXME comments in the code for src/, this patch improves the handling of the fileId delete behaviour.

Files can have multiple parentReferences, meaning the same fileId and name can show up as a childReference in more than one folder. However, outright deleting the fileId would remove it from both folders which might result in an unintended loss of data. Therefore, we first check if the file has more than one parent - when this is the case we find the parentId of the current folder and remove that parentId from the fileId. If the fileId has exactly one parentId, we delete the entire fileId rather than removing the last parentId (which would cause the file to become invisible in your GDrive).

Please note this patch depends on a bugfix which should be rolled out in LibKGAPI: https://phabricator.kde.org/D9774

Test Plan
  1. Insert an additional parentReference for an existing fileId through the Google API explorer: https://developers.google.com/drive/v2/reference/parents/insert
  1. Verify the second parentReference is now visible on the fileId through the Google API explorer: https://developers.google.com/drive/v2/reference/parents/list
  1. "Delete" the file in one folder through the KIO-GDrive plugin, verify the file still exists in the other folder because we actually only removed one of the two parentReference items.
  1. Delete the file in the second folder through the KIO-GDrive plugin, and verify that the file is no longer visible through KIO-GDrive.
  1. Open your GDrive through the webinterface, and check the Recent menu to verify that the file has not simply had all of its parentReferences removed in step 4, rather than actually being deleted. A so-called "ghost" fileId without any parentReference will still appear in https://drive.google.com/drive/recent

Diff Detail

Repository
R219 KIO GDrive
Lint
Lint Skipped
Unit
Unit Tests Skipped
martijnschmidt requested review of this revision.Jan 9 2018, 9:32 PM
martijnschmidt created this revision.

Nice job! I have one remark, but that's probably up to @elvisangelaccio to decide which is the best course of action.

src/kio_gdrive.cpp
949

I think we should only trash the file, not outright delete it. Even if we don't have Trash in KIO GDrive, the user should still be able to go to GDrive web and recover the file there IMO.

martijnschmidt added inline comments.Jan 10 2018, 1:04 PM
src/kio_gdrive.cpp
949

Well, the KIO seems to interpret this function as "Delete" which would normally be under Shift+Del. Trash e.g. "Move to Trash" would normally be under Del. I would consider creating a new function which toggles the trashed property of the file and implement that separately. Then the code that I uploaded now would only be called if the user really really wants to delete the file?

src/kio_gdrive.cpp
949

Hmm this is tricky. The problem is that KIO/Dolphin don't support the trash operation with non-local URLs. So in Dolphin both Del and Shift+Del trigger the same code (i.e. this function) if the URL is non-local.

martijnschmidt added inline comments.Feb 4 2018, 11:29 AM
src/kio_gdrive.cpp
949

Before we dive down this rabbit hole, what are your thoughts regarding the support of non-local trash in KIO/Dolphin? Should it exist? Evidently several cloud storage services (GDrive, MS OneDrive, Dropbox, etc) implement this feature, even if the traditional non-local storage options (such as SMB/CIFS, SCP/FTP, etc) don't really have something like this.

elvisangelaccio added inline comments.Feb 4 2018, 4:01 PM
src/kio_gdrive.cpp
949

That would not be a problem, because we can add a new metadata key (e.g. trashing or similar) to the protocols specification, so that we can know which protocol supports non-local trashing.

Imho such a feature would be nice, but I'm not sure how much work would it take to implement.

martijnschmidt added inline comments.Feb 5 2018, 12:41 AM
src/kio_gdrive.cpp
949

It seems Dolphin, Krusader, and Konqueror check whether the file is local before showing the trash option. The trash feature is replaced by the delete action for non-local files so the user doesn't notice weird behaviour.

Gwenview, Okular, Marble, Amarok, Kate, etc - pretty much every project that only uses KIO for the Open... Ctrl+O dialog, all just show the Trash option anyway for remote files accessed through KIO-GDrive. If you attempt to use the trash button in the right-click menu you'll receive an Access denied to trash:/filename.extension. error which seems to be the result of a KIO::ERR_ACCESS_DENIED call. For example, Gwenview submits a KIO::trash(urlList) to KIO::Job in app/fileoperations.cpp, which then lands in the KIO project's src/core/copyjob.cpp code as a CopyJob::Move to destination folder trash:/. And the comments in src/core/copyjob.h have indeed documented that trashing is currently only supported for local files and directories because the special trash:/ destination is provided by an ioslave defined in src/ioslaves/trash/kio_trash.cpp.

I don't think it'd be desirable for applications to have to worry about whether they should use KIO::trash() or something else. Because we aren't using a CopyJob::Move to the trash:/ destination, but rather toggling GDrive's trashed boolean through LibKGAPI for our use case, I feel like it may be best to add some if/else logic in KIO's src/core/copyjob.cpp to check whether a file is local; when remote, check a remoteTrash boolean which defaults to false when not defined elsewhere and allow KIO slaves such as KIO-GDrive to toggle it to true. Such a remoteTrash boolean would also have to be checked when Dolphin's url.isLocalFile() statement returns false, before deciding whether or not the moveToTrashAction is available. Thoughts?

PS: where can I find the protocols specification you mentioned - it sounds like a good place for that remoteTrash boolean?
PPS: as I mentioned earlier I haven't done a lot of programming before, so don't hesitate to tell me if something I'm talking about should be done differently!

src/kio_gdrive.cpp
949

Sounds good to me. You may also want to send an email to kde-frameworks-devel@kde.org (and CC the KIO maintainer faure@kde.org) to get some more feedback about this.

I'm not sure there is a formal protocol specification. Best thing to do is looking at the code the does the actual parsing of the protocols (which use either the KConfig formar or the new json format). You can find it in kio/src/core/kprotocolinfo.cpp.

@martijnschmidt Ping. What's the status of this patch?