Make Trash work again
Open, Needs TriagePublic

Description

Original description by Daniel:

The original plan was to have a virtual "Trash" folder in top-level directory listing,
which would contain all trashed files and folders. The feature has been removed
though, because it breaks with folders. Trashed folders still have their original
parent reference set, so we fail to resolve their UID properly in
KIOGDrive::resolveFileIdFromPath() This could be solved by not using nice file names
in trash, but that would mean two codepaths in all io methods just to handle the trash case.

elvisangelaccio updated the task description. (Show Details)

It looks like the whole trash feature is no longer made available in the Google Drive API v3: https://developers.google.com/drive/v3/reference/files

Trashed files shouldn't show up in the list function either: https://developers.google.com/drive/v3/reference/files/list

All one can still really do through the v3 API is empty the trash, otherwise it seems Google just wants files to be deleted outright. It seems to me like this backlog item would become a non-issue as soon as LibKGAPI moves to the v3 API spec?

martijnschmidt added a comment.EditedJan 7 2018, 4:34 PM

Looking at your comments in the code with regard to trash:

// FIXME: Verify that a single file cannot actually have multiple parent
// references. If it can, then we need to be more careful: currently this
// implementation will simply remove the file from all it's parents but
// it actually should just remove the current parent reference

Turns out it can, but you need to add the second reference through the API: I was not able to insert a second parent reference through Google's webinterface, and copy-and-pasting a file from one folder to another folder on the same Drive through KIO-GDrive does not work at all (drag-and-drop moving or cutting-and-pasting the file does work).

For example, if your fileId A currently has a parentId B, you could add another parentId C like so - having tested this through Google's API Explorer:

POST https://www.googleapis.com/drive/v2/files/A/parents
Request body contains:
{
  "id": "C",
}

After that, you'll see two parentReference items enumerated under the parentList if you use:

GET https://www.googleapis.com/drive/v2/files/A/parents
{
 "kind": "drive#parentList",
 "etag": "\"D\"",
 "selfLink": "https://www.googleapis.com/drive/v2/files/A/parents",
 "items": [
  {
   "kind": "drive#parentReference",
   "id": "B",
   "selfLink": "https://www.googleapis.com/drive/v2/files/A/parents/B",
   "parentLink": "https://www.googleapis.com/drive/v2/files/B",
   "isRoot": false
  },
  {
   "kind": "drive#parentReference",
   "id": "C",
   "selfLink": "https://www.googleapis.com/drive/v2/files/A/parents/C",
   "parentLink": "https://www.googleapis.com/drive/v2/files/C",
   "isRoot": false
  }
 ]
}

Once the file has two parents then changes that are made to the file in Kate (opened the file via KIO-GDrive) take effect in both folders. Renaming the file through KIO-GDrive also takes effect in both folders, but I have to press F5 to make the changed filename appear even after clicking through the Dolphin UI to view different folders - is the content of the folder cached somewhere, instead of pulled through the API when the user opens the folder? Performing a DELETE removes both copies of the file and skips the trash.

DELETE https://www.googleapis.com/drive/v2/files/A
Returns a HTTP 204 No Content.

I've spent some more time experimenting by removing the last parent from a fileId, which is another API-only action:

DELETE https://www.googleapis.com/drive/v2/files/A/parents/B
Returns a HTTP 204 No Content.

After doing this we can still list the parents, but there won't be a single parentReference item in the response:

GET https://www.googleapis.com/drive/v2/files/A/parents
{
 "kind": "drive#parentList",
 "etag": "\"D\"",
 "selfLink": "https://www.googleapis.com/drive/v2/files/A/parents",
 "items": []
}

If the file has no parents at all, it is still visible in the list all files query GET https://www.googleapis.com/drive/v2/files, but with an empty parents block as described in the parentsList code block.

You can still view the file in the webinterface, but only at the https://drive.google.com/drive/recent page. After right-clicking and choosing "Add to My Drive", the file appears in the root folder of the user's Drive at that point it once again has a parent (e.g. the "isRoot": true root folder).

The second comment in the code with regard to trash was as follows:

// FIXME: Because of the above, we are not really deleting the file, but only
// moving it to trash - so if users really really really wants to delete the
// file, they have to go to GDrive web interface and delete it there. I think
// that we should do the DELETE operation here, because for trash people have
// their local trashes. This however requires fixing the first FIXME first,
// otherwise we are risking severe data loss.

I think this is a good middle road, but it would probably be best if we "wrapped" the straight-up delete behaviour like so:

  1. If the user tries to delete a fileId, remove the parentReference relating to the current folder.
  2. If parentId.isEmpty() returns as true then proceed to run a deleteJob as well. Else, do nothing because the fileId still has another parentReference.

Should the file still be moved to the local trash in this procedure? Currently, Move to Trash - Del is not available in the Dolphin right-click sub-menu and Del behaves as if I had pressed Shift+Del, prompting Do you really want to delete this item? rather than Do you really want to move this item to the trash?. I would be happy to try submitting a patch for this, but I'm not a very experienced programmer and have actually been going through the code as a learning project for myself.

@martijnschmidt Wow, thanks for your analysis!

You are 100% welcome to submit a patch, even if it's the first time for you. That's what code review is for, don't worry :)

dvratil added a subscriber: dvratil.Jan 8 2018, 9:02 AM

From the KGAPI perspective, looking at the V2->V3 API migration guide (https://developers.google.com/drive/v3/web/migration) it appears that it should not be too much work to add V3 API to KGAPI for KIO GDrive to use.

The migration document also mentions some changes in parent handling that might make it actually easier to deal with the parent changes.

Regarding the Trash, the change in V3 is that it's no longer an actual folder that can be referenced from the API but simply just a collection of all Files with the trashed parameter set to true. My understanding is that there is still Trash in V3, but it's up to the implementators to create it themselves and only show trashed files in it. You can list all trashed files via

GET https://www.googleapis.com/drive/v3/files?q=trashed%3Dtrue&key={YOUR_API_KEY}

So trashing items should only be a matter of updating the file's trashed property.

Playing around with copying of files in the GDrive web UI, I was only able to create a copy in the current folder, which actually creates a new file called "Copy of [Foo]" that shares nothing with the original file (in the files.list API response it's a separate entry that does not reference the original file in any way), so I think we should make sure we do the same in KIO GDrive (maybe we already do, haven't checked). Then copies will no longer have the problem with multiple parent references and trashing each copy will be safe.

Your comments about dealing with files with multiple parents still apply though.

(n.b.: as far as I understand, the problem with Dolphin not refreshing when you delete/move a file is a long-standing issue with Dolphin/KIO on remote filesystems. I believe it automatically only works on filesystems that can be watched for changes via inotify/FAM), which does not apply to abstract file systems like the Drive.)

From the KGAPI perspective, looking at the V2->V3 API migration guide (https://developers.google.com/drive/v3/web/migration) it appears that it should not be too much work to add V3 API to KGAPI for KIO GDrive to use.

The migration document also mentions some changes in parent handling that might make it actually easier to deal with the parent changes.

Regarding the Trash, the change in V3 is that it's no longer an actual folder that can be referenced from the API but simply just a collection of all Files with the trashed parameter set to true. My understanding is that there is still Trash in V3, but it's up to the implementators to create it themselves and only show trashed files in it. You can list all trashed files via

GET https://www.googleapis.com/drive/v3/files?q=trashed%3Dtrue&key={YOUR_API_KEY}

So trashing items should only be a matter of updating the file's trashed property.

Ahh, I had completely missed that. So looking at the v3 API we should do something like the following to move the file to trash:

PATCH https://www.googleapis.com/drive/v3/files/fileId
{
  "trashed": true
}

Playing around with copying of files in the GDrive web UI, I was only able to create a copy in the current folder, which actually creates a new file called "Copy of [Foo]" that shares nothing with the original file (in the files.list API response it's a separate entry that does not reference the original file in any way), so I think we should make sure we do the same in KIO GDrive (maybe we already do, haven't checked). Then copies will no longer have the problem with multiple parent references and trashing each copy will be safe.

I had the same result as you did with the GDrive web UI. In KIO GDrive we can not copy files at all, Dolphin shows a pop-up with the message "The file or folder gdrive:/accountname/filename does not exist." so presumably it can't find the source file.

Your comments about dealing with files with multiple parents still apply though.

(n.b.: as far as I understand, the problem with Dolphin not refreshing when you delete/move a file is a long-standing issue with Dolphin/KIO on remote filesystems. I believe it automatically only works on filesystems that can be watched for changes via inotify/FAM), which does not apply to abstract file systems like the Drive.)

I see, but maybe we could call a refresh or something like that immediately after deleting/moving the file? Then we would essentially work around the issue.

martijnschmidt added a comment.EditedJan 9 2018, 9:46 PM

I've uploaded two Differentials for this task;

  • D9774 is a bugfix for LibKGAPI: I kept getting an empty result out of ParentReferenceFetchJob which was needed to count the amount of parentReference items for the fileId, which turned out to be caused by a misbehaving sanity check. I couldn't tag the LibKGAPI project as a reviewer for D9774 so I have added you (Dan & Elvis) as reviewers, I hope that's okay!
  • D9775 is the actual delete behaviour patch which implements the comments that were made last week. The design of the patch changed slightly since it allowed me to avoid a third API call in cases where the file actually needs to be deleted:

Previously described:

When the user tries to delete a fileId, remove the parentReference relating to the current folder. If parentId.isEmpty() returns as true then proceed to run a deleteJob as well. Else, do nothing because the fileId still has another parentReference.
1. parentReferenceDeleteJob
2. parentReferenceFetchJob
3. fileDeleteJob

Now implemented:

When the user tries to delete a fileId, count the amount of parentReference objects for this fileId. If there is more than one parentReference, run a ParentReferenceDeleteJob. Elseif there is exactly one parentReference, run a FileDeleteJob. Else, generate an error because the fileId had no parentReference at all.
1. parentReferenceFetchJob
2. parentReferenceDeleteJob OR fileDeleteJob