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

RESOLVED FIXED

Status

MailNews Core
Backend
--
major
RESOLVED FIXED
11 years ago
3 years ago

People

(Reporter: Andy Boze, Assigned: Bienvenu)

Tracking

(Blocks: 2 bugs, {qawanted, verified1.8.1.2})

Trunk
qawanted, verified1.8.1.2
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [comment 17?])

Attachments

(4 attachments)

(Reporter)

Description

11 years ago
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.
(Reporter)

Comment 1

11 years ago
Created attachment 251500 [details]
Custom tag status not being removed properly
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

11 years ago
xref bug 355205, this is perhaps a dupe of that.
(Reporter)

Comment 4

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

Comment 5

11 years ago
"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

11 years ago
Created attachment 255585 [details] [diff] [review]
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.
Assignee: mail → mnyromyr
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #255585 - Flags: superreview?(bienvenu)
Attachment #255585 - Flags: review?(neil)

Updated

11 years ago
Component: MailNews: Main Mail Window → MailNews: Backend
OS: Windows XP → All
Product: Mozilla Application Suite → Core
Hardware: PC → All
Version: unspecified → Trunk

Updated

11 years ago
Attachment #255585 - Attachment description: only remove known tags and do without hacks → only remove known tags and do that without hacks (TB and SM)

Comment 7

11 years ago
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 ;-)
Attachment #255585 - Flags: review?(neil) → review+
(Assignee)

Comment 8

11 years ago
so, for imap, whenever we select the folder again, we'll get all the keywords back from the server, when we list all flags...
(Assignee)

Comment 9

11 years ago
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...
Attachment #255585 - Flags: superreview?(bienvenu) → superreview-
(Assignee)

Comment 10

11 years ago
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.
(Assignee)

Comment 11

11 years ago
Created attachment 255649 [details] [diff] [review]
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?
(Assignee)

Comment 12

11 years ago
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

11 years ago
(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).
(Assignee)

Comment 14

11 years ago
Created attachment 255796 [details] [diff] [review]
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.
Assignee: mnyromyr → bienvenu
Attachment #255796 - Flags: superreview?(mscott)

Updated

11 years ago
Attachment #255796 - Flags: superreview?(mscott) → superreview+
(Assignee)

Comment 15

11 years ago
fixed on trunk and branch
Keywords: fixed1.8.1.2

Comment 16

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

Comment 17

11 years ago
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)
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

11 years ago
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. 
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.
verified fixed with 2.0.0.0 rc2 on WinXP

should this be marked fixed per comment #15 ?
Keywords: fixed1.8.1.2 → verified1.8.1.2
(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.
Blocks: 432710
QA Contact: backend
Product: Core → MailNews Core

Comment 23

8 years ago
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.
Whiteboard: qawanted
Keywords: qawanted
Whiteboard: qawanted → [comment 17?]
should we close this and file a new bug for anything remaining?
ref: comment 22
Flags: needinfo?(acelists)

Comment 25

5 years ago
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.
Flags: needinfo?(acelists) → needinfo?(neil)
(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.
Flags: needinfo?(neil)
Blocks: 1050784
(In reply to neil@parkwaycc.co.uk 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?
Flags: needinfo?(neil)
(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.
Flags: needinfo?(neil)
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.