This is implemented for local collections now, but it's still missing a way to undo.

I implemented a trash function for local documents, but without any undo functionality whatsoever. Is this a requirement? Otherwise I'd create a patch for this.

I think an undo functionality in a little infobar/banner would be good to have for this; feel free to create and attach a patch though even if you don't plan to implement that though, it can be useful to others to iterate on top of.

Current state for this bug:

 * Delete works for locally created collections. Not for cloud-stored collections.
 * Delete does not work for documents.
 * Deletion of locally created collections is not undoable.

Changes that we want:

 * Deletion for local documents.
 * Undo for deleting documents and local collections (this should work similar to the behaviour in Contacts or Boxes).

Do we want to still trash the items or delete (Shift + Delete) the items ?

After talking to jimmac, we don't want to involve the trash at all. Atleast not for the moment. The idea is that people rarely go there looking for something that they accidentally deleted two days ago.

When a user deletes, we should hide it from the view and let the action be undone in the next N seconds. If I recall correctly N == 10 in Boxes. If not undone, then it would be gone for ever.

Created attachment 282092 [details] [review]
documents: Let everybody provide their own trash implementation

Review of attachment 282092 [details] [review]:

::: src/documents.js
@@ +377,3 @@
+
+        let job = new DeleteItemJob(this.id);
+        job.run(null);

I'm a bit confused by this part... Shouldn't it go inside the if (collection) switch in LocalDocument's trashImpl override?

Created attachment 283095 [details] [review]
notifications: Add DeleteNotification

(In reply to comment #8)
> Review of attachment 282092 [details] [review]:
> 
> ::: src/documents.js
> @@ +377,3 @@
> +
> +        let job = new DeleteItemJob(this.id);
> +        job.run(null);
> 
> I'm a bit confused by this part... Shouldn't it go inside the if (collection)
> switch in LocalDocument's trashImpl override?

I wanted to minimize the amount of code in trashImpl because it will be implemented every subclass.

Strictly speaking there is no need to run DeleteItemJob for local documents because tracker will notice that the file was removed from the filesystem and update itself, but there is no harm in running it either. This way, every subclass that offers trashImpl won't have to bother about DeleteItemJob.

By the way, the "undo" button is too tall at the moment. Since the button in Boxes is affected in the same way, I am trying to figure out whether this really needs fixing or it is a theme issue.

(In reply to comment #11)
> By the way, the "undo" button is too tall at the moment. Since the button in
> Boxes is affected in the same way, I am trying to figure out whether this
> really needs fixing or it is a theme issue.

Filed as bug 734614

Review of attachment 282092 [details] [review]:

OK then, thanks for the explanation.

Review of attachment 283095 [details] [review]:

Two stylistic comments here, looks good otherwise.

::: src/notifications.js
@@ +95,3 @@
+
+    _destroy: function() {
+            }));

I would move this in the undo callback for clarity.

::: src/selections.js
@@ +904,3 @@
             function(urn) {
                 let doc = Application.documentManager.getItemById(urn);
+                docs.push(doc);

You could here call removeItem at the same time, to avoid iterating over the array twice.

(In reply to comment #14)
> Review of attachment 283095 [details] [review]:

Thanks for the review, Cosimo.

> ::: src/selections.js
> @@ +904,3 @@
>              function(urn) {
>                  let doc = Application.documentManager.getItemById(urn);
> +                docs.push(doc);
> 
> You could here call removeItem at the same time, to avoid iterating over the
> array twice.

The thing is that removing items from DocumentManager results in the selection array inside SelectionsController being modified. I was not sure if the selection is returned as a deep copy or is a reference to the one in SelectionsController. If it is a reference then removing the documents while iterating over the selection could be a problem.

(In reply to comment #15)

> The thing is that removing items from DocumentManager results in the selection
> array inside SelectionsController being modified. I was not sure if the
> selection is returned as a deep copy or is a reference to the one in
> SelectionsController. If it is a reference then removing the documents while
> iterating over the selection could be a problem.

Ah, makes sense then; feel free to push the patch with the other stylistic comment addressed.

Created attachment 283164 [details] [review]
notifications: Add DeleteNotification

Move _destroy into the callbacks.

*** Bug 760555 has been marked as a duplicate of this bug. ***