367011 - "Remove All Tags" does not do so for custom tags

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Andy Boze]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.2pre) Gecko/20070113 SeaMonkey/1.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.2pre) Gecko/20070113 SeaMonkey/1.1

If an IMAP (haven't checked on local or POP folders) e-mail is tagged with a custom tag, untagging it with "Remove All Tags" will make it appear to be untagged. However, switching to another folder and then back, the message will regain its tag color and it will have a check in the tag menu. However, the tag text will not be shown in the tag column. Built-in tags appear not to exhibit this problem.

Reproducible: Always

Steps to Reproduce:
1.Tag a message using a custom tag in IMAP folder
2.Untag the message using "Remove All Tags"
3.Switch to another folder and then back
Actual Results:  
Although when initially untagged, the messsage appears to have had the tag removed, upon switching to another folder and then back, the message will regain its tagged color and will show a check for the tag in the Tag menu. However, the text in the Tag column will not reflect the tag status as shown in the Tag menu. Using the specific tag in the Tag menu will remove the tag correctly.

Expected Results:  
"Remove All Tags" should completely remove the custom tag status and it should not reappear after switching to another folder and back.

I'm attaching an image to show an example. In part (1) you can see that I have tagged four messages using a custom tag called "Test Tag". In part (2) of the image, you can see that I have untagged all four of them. The first two were untagged by using "Remove All Tags". The second two were untagged by unchecking the custom tag from the Tag menu. In part (3) you can see that after switching to another folder and back, the first two of the four messages have regained their tagged color, even though the tag text is missing from the Tag column. In addition, the check is still there in the Tag menu for those two messages.

Comment 1

[Andy Boze]

Attachment: Custom tag status not being removed properly

Comment 2

[Bruno 'Aqualon' Escherl]

WFM with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.2pre) Gecko/20070110 SeaMonkey/1.1 and a POP3 account.

Comment 3

[Mike Cowperthwaite]

xref bug 355205, this is perhaps a dupe of that.

Comment 4

[Andy Boze]

Might also be related to another bug I reported, bug 366503. I'll have to experiment a little more.

Comment 5

[Karsten Düsterloh]

"Remove all tags" tries to take a shortcut way to do the removal fast, but this seems to be quite problematic it seems. (See http://lxr.mozilla.org/seamonkey/source/mailnews/base/resources/content/mailWindowOverlay.js#471)
It's probably better to just do a loop to delete the tags...

Comment 6

[Karsten Düsterloh]

Attachment: only remove known tags and do that without hacks (TB and SM)

We're using a kind of hack to remove all tags from message: we just delete all keywords set on a message. This (a) obviously doesn't always notify all "interested parties" and (b) is really wrong, I have to confess - this even kills the (non)junk state, which is a keyword internally, too, at least for IMAP, and keywords set on IMAP messages by other clients.
This patch just deletes all tags known by us in a loop.

Comment 7

[[e-mail]]

Comment on attachment 255585 [details] [diff] [review]
only remove known tags and do that without hacks (TB and SM)

>+  // this crudely handles cross-folder virtual folders with selected messages
>+  // that spans folders, by coalescing consecutive messages in the selection
>+  // that happen to be in the same folder. nsMsgSearchDBView does this better,
>+  // but nsIMsgDBView doesn't handle commands with arguments, and untag takes a
>+  // key argument. Furthermore, we only delete legacy labels and known tags,
>+  // keeping other keywords like (non)junk intact.
Perhaps you could create nsMsgViewCommandType::removeAllTags ;-)

Comment 8

[David :Bienvenu]

so, for imap, whenever we select the folder again, we'll get all the keywords back from the server, when we list all flags...

Comment 9

[David :Bienvenu]

Comment on attachment 255585 [details] [diff] [review]
only remove known tags and do that without hacks (TB and SM)

so, if you have 100 tags defined, doesn't this issue 100 separate imap commands keywords for each message, even if the message just has one keyword defined? That seems sub-optimal to me - I'll investigate a slightly friendlier solution to the imap server...

Comment 10

[David :Bienvenu]

I think it would be better to just remove the non tag keywords from the list of keywords to remove from the server, but still do that in one command for the server...it's terribly misleading, but the RemoveKeywordFromMessages command will actually work fine is the "keyword" is a list of keywords.

Comment 11

[David :Bienvenu]

Attachment: one possible approach

This is one possibility - figure out which keys are defined for any given message, and remove them all in one fell swoop. This does the right thing for imap (removes all the keys passed in) but nsMsgDBFolder::RemoveKeywordFromMessages only removes all keywords if you happen to pass in the exact same string as is stored in the keywords property.  So nsMsgDBFolder::RemoveKeywordFromMessages would need to explicitly handle multiple keywords passed in as one string, or I'd need to add a new method to remove multiple keywords, or add a method to remove all keywords. I think that's worthwhile because removing them one at a time to the imap server will be seen as silly by anyone who looks at the protocol we generate.

I'll attach a slight modification of Karsten's previous patch that just removes all possible keywords in one command, for all msgs in a given folder. That will generate a long protocol string if there are lots of tags defined, but that may not be too bad. I'm not sure if it's better to optimize the case of multiple messages selected, or lots of keywords defined. In any case, I'd still need to fix the backend to allow this, in one of the three ways I described above.

Thoughts?

Comment 12

[David :Bienvenu]

I think I'm just going to add a clearKeywordsForMessages method to nsIMsgFolder.idl so that the IMAP code can do the right thing.

Comment 13

[Karsten Düsterloh]

(In reply to comment #9)
> so, if you have 100 tags defined, doesn't this issue 100 separate imap commands
> keywords for each message, even if the message just has one keyword defined?

Eww. Yeah, didn't think too much of IMAP. :(

(In reply to comment #11)
> I'm not sure if it's better to optimize the case of multiple
> messages selected, or lots of keywords defined.

I guess it's far more likely that someone marks all messages in a folder to get rid of all their tags than having really large numbers of tags (at least from usecase, no real world data here).

Comment 14

[David :Bienvenu]

Attachment: proposed fix

This is a bit scary, but it seems to be working for me...

The first change is to replace AddKeywordToMessages with AddKeywordsToMessages, and the corresponding change for Replace, so that we can pass in multiple keywords to these routines. I changed all the callers and the implementations to handle multiple keywords, basically by just looping through each of the passed in keywords, and adding/removing them like we did before.  For local mail messages, I haven't optimized the code to deal with multiple keywords, since that would be scary at this point - for each message, we basically loop through the keywords as if the user had called Add/Remove multiple times...

For IMAP, we can do it all in one command, and I do so.

I changed the RemoveAllTags implementation to just remove all the known keys, and I batch the commands for msgs in the same folder, using Karsten's code for that. This is kind to imap servers in the case of multiple selection. If you have a lot of keywords defined, we'll generate a fairly long protocol string, but most servers don't care, and most users won't have a ton of tags.

The alternative change to RemoveAllTags is to do what I do in the previous patch and just remove the keywords that are defined in any given message. This is less optimal for multiple selection, but friendlier for users with a lot of tags.

Comment 15

[David :Bienvenu]

fixed on trunk and branch

Comment 16

[[e-mail]]

So gDBView.doCommand(nsMsgViewCommandType::removeAllTags); was a non-starter?

Comment 17

[David :Bienvenu]

no, I'm fine with that - it was orthogonal to the painful part of getting RemoveAllTags to work efficiently - we can still do that (but I think trunk-only)

Comment 18

[Wolfgang Rosenauer [:wolfiR]]

I wonder if "fixed1.8.1.2" is the right keyword here since AFAIK SeaMonkey 1.1.1 was released based on Gecko 1.8.1.2 but doesn't contain the fix AFAICS.

Comment 19

[Karsten Düsterloh]

If it was fixed in 1.1.1, it would have fixed-seamonkey1.1.1. As you can see in <http://bonsai.mozilla.org/cvsgraph.cgi?file=mozilla/mailnews/base/resources/content/mailWindowOverlay.js&rev=1.252>, the checkin was after the tagging for 1.1.1. 

Comment 20

[Wolfgang Rosenauer [:wolfiR]]

That wasn't in question ;-)
But that graph also shows that the change was in after the latest Firefox release tag which is more or less the point where Gecko 1.8.1.2 is also "tagged".
I was pretty confused by seeing this fixed1.8.1.2 while I think that's not true at all.

Comment 21

[Tracy Walker [:tracy]]

verified fixed with 2.0.0.0 rc2 on WinXP

should this be marked fixed per comment #15 ?

Comment 22

[Wayne Mery (:wsmwk)]

(In reply to comment #21)
> verified fixed with 2.0.0.0 rc2 on WinXP
> 
> should this be marked fixed per comment #15 ?

David could say for sure, but I think it was left open for comment 17.

Comment 23

[arnoldweissberg]

I didn't post a new bug but this is a problem with POP also.  The "0" (zero) key is supposed to remove all tags but it doesn't.

Comment 24

[Wayne Mery (:wsmwk)]

should we close this and file a new bug for anything remaining?
ref: comment 22

Comment 25

[:aceman]

There is Neil's comment 16 left to do. But if that means rewriting the fix into a C++ method of some class, then it is nothing for me ;)

For comment 23 we'd need more info, if this still happens and how to reproduce.

Comment 26

[[e-mail]]

(In reply to aceman from comment #25)
> There is Neil's comment 16 left to do. But if that means rewriting the fix
> into a C++ method of some class, then it is nothing for me ;)

Yes, the idea is that the UI calls gDBView.doCommand, and the base nsMsgDBView.cpp version would simply call ApplyCommandToIndices which would forward the command to the folder while nsMsgSearchDBView.cpp uses PartitionSelectionByFolder and calls ApplyCommandToIndices once for each folder. But either way it sounds like a followup bug should be filed on this.

Comment 27

[Wayne Mery (:wsmwk)]

(In reply to [e-mail] from comment #26)
> But either way it sounds like a followup bug should be filed on this.

bug 050784

Does this bug need to stay open?

Comment 28

[[e-mail]]

(In reply to Wayne Mery comment #27)
> (In reply to comment #26)
> > But either way it sounds like a followup bug should be filed on this.
> 
> bug 1050784
> 
> Does this bug need to stay open?

Well, the bug was worked around in the frontend code, so I guess not.