536048 - Cannot distinguish the log for "Empty Trash" from folder deletion in Activity manager

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[Masahiko Imanaka [:marsf]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; ja; rv:1.9.1.6) Gecko/20091201 Firefox/3.5.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20091219 Shredder/3.1a1pre

When I've deleted the normal folder named "trash", Activity manager says: "Deleted folder trash". When I've empty the Trash folder, Activity manager says: "Deleted folder Trash".

They are using same entity "deletedFolder" in activity.properties. I think the log for "Empty Trash" is not proper. Because the Trash folder has not actually deleted.


Reproducible: Always

Steps to Reproduce:
1. Empty the Trash folder.
2. Delete a normal folder named "trash".
3. See the log in Activity manager.
Actual Results:  
Shows same log in Activity manager.

Expected Results:  
When I empty the Trash folder, the log should be separated with folder deletion. It should be "the Trash folder has emptied".

Comment 1

[Masahiko Imanaka [:marsf]]

The solution is simple.
Please add a string for "Empty Trash" log.

Comment 2

[J.Pietschmann]

Actually, getting "Deleted folder Trash" in the activity manager after emptying the Trash is rather irritating.

Comment 3

[Marcel Guzman]

I've got a fix for this and am working on learning how tests work before submitting it.

Currently my fix adds a string for "Empty Trash" but it's easy enough to suppress this. What do you guys think?

Comment 6

[Blake Winton (:bwinton) (:☕️)]

(In reply to comment #3)
> I've got a fix for this and am working on learning how tests work before
> submitting it.
> 
> Currently my fix adds a string for "Empty Trash" but it's easy enough to
> suppress this. What do you guys think?

I'ld add the "Empty Trash" string, but Bryan Clark is our UX guy, so let's see what he says.  :)

Later,
Blake.

Comment 7

[David :Bienvenu]

Marcel, would you care to attach your patch for this?

Comment 8

[Marcel Guzman]

Attachment: Adds an Empty Trash string to the Activity Manager

This also contains fixes to some related issues that I found while fixing this bug (and couldn't find on Bugzilla). I can split these out into separate patches/bugs if you'd prefer.

1. When renaming a folder, both a delete and a rename event are fired, resulting in two entries in the Activity Manager. My patch removes the delete event.
2. When moving a folder Foo to the trash, the message in the Activity Manager is "Renamed folder foo to foo". I changed it to: "Moved folder foo to trash"

One possible side effect of this change is that renaming a folder within the trash folder (ie. from Trash/foo to Trash/bar) still adds the "Moved folder foo to trash" string. I can't figure out a way to fix this - all information about the original folder's parent is nulled out before calling NotifyFolderRenamed().

Comment 9

[Ludovic Hirlimann [:Usul]]

(In reply to comment #8)
> Created an attachment (id=427994) [details]
> Adds an Empty Trash string to the Activity Manager

Thanks a lot for the patch. To be able to get your patch into the tree you need to follow a few requirements described at https://developer.mozilla.org/en/comm-central#comm-central_tree_rules. For now you would need to request a review. Your patch lacks a unit test, these are required since January 4th (see http://groups.google.com/group/mozilla.dev.apps.thunderbird/browse_thread/thread/d0690bb47c771568/716d8aa540749c9f?lnk=gst&q=test+policy#716d8aa540749c9f). Will be glad to provide guidance for the test part.

Comment 10

[Masahiko Imanaka [:marsf]]

Thank you Marcel.

FYI, string freeze date is 2010-03-23 (next Tuesday).
When the patch will be reviewed?

Comment 11

[Blake Winton (:bwinton) (:☕️)]

Well, it would be a lot more likely to get reviewed if Marcel set the review flag to "?", and the reviewer to ":bwinton".  ;)

Comment 12

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 427994 [details] [diff] [review]
Adds an Empty Trash string to the Activity Manager

Looks good to me.

Now you need to ask someone else for an additional review.  I suggest either Standard8 or Bienvenu, since they seem to have reviewed these files before.

Later,
Blake.

Comment 13

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 427994 [details] [diff] [review]
Adds an Empty Trash string to the Activity Manager

Drive by ui-review!

"Emptied Trash" - looks good
"Moved folder #1 to Trash" - capitalize Trash since it's a "special folder"

ui-r+ with that change.

Comment 14

[Marcel Guzman]

Comment on attachment 427994 [details] [diff] [review]
Adds an Empty Trash string to the Activity Manager

Thanks for the reviews guys!

Comment 15

[Mark Banner (:standard8)]

I've checked this in with Bryan's comment addressed:

http://hg.mozilla.org/comm-central/rev/ac77cab73bd1

Thanks for the patch.