455478 - New added tag is not sorted when sorted by tag

Status: NEW

(Thunderbird :: Mail Window Front End, defect)

Description

[Boying Lu]

re-produce steps:
0. start thunderbird with a new profile
1. setup an IMAP account
2. make sure that "Tag" column is shown and click "Tag" cloumn to make the thread pane is sorted by "Tag". (Don't click other columns)
3. select an e-mail without any tags
4. right click it and select "Tag" menu item
5. select a tag for the mail e.g. "Later"
6. click the "Tag" column again.

Expected result:
The new added tag (e.g. "Later") should be sorted as other tags (i.e. in aphabet order).

Actual result:
The new added tag is not sorted

Comment 1

[Boying Lu]

Attachment: patch

The root cause is that m_sortValid is not set to false when new tag is added to an e-mail.

Comment 2

[Mark Banner (:standard8)]

Comment on attachment 338842 [details] [diff] [review]
patch

This just doesn't look right. For starters I think you're trying to change the pointer, which won't actually get changed properly in the caller because the pointer is being passed by value, and you'd be trying to change a local variable into a pointer anyway.

Having said that, the better way would be to see if we can somehow either record the tags as numbers, or maybe do a hash value and return that. Kent/David may have some ideas here.

Comment 3

[Kent James (:rkent)]

I'm not sure that a PRUint32 can store a pointer on all platforms - doesn't that break for a 64 bit build? So I'll second the notion of using a hash value for the tags and storing that. Since an occasional missed re-sort is not a big deal, a simple hash such as summing the values of the characters might be sufficient. 

Also note that there are other issues with tag sorting (bug 385032). I spent a few minutes on that bug a few months ago, thinking it would be easy, but quickly got lost in the abstraction of the sort ordering functions.

Comment 4

[Boying Lu]

Comment on attachment 338842 [details] [diff] [review]
patch

Thanks a lot for your comments. I'll make a new patch.

Comment 5

[Boying Lu]

Attachment: patch v1

Sort tags by their ordinal instead of their names. i.e. tags are sorted by the order shown in "Perferences"-->"Display" --> "Tags".
I think this makes much more sense.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 339943 [details] [diff] [review]
patch v1

ns(C)String have .ToInteger and .AppendInt functions, I think it would be best if you used those, rather than atoi and the printf variants that you've used in this patch.

I'd like to see those changes done before I test this please.

Comment 7

[Boying Lu]

Attachment: remove atoi and printf

Comment 8

[Mark Banner (:standard8)]

Hmm, I'm just trying this and I'm not seeing it, could it be that I have an imap account that doesn't support tags?

Comment 9

[Boying Lu]

(In reply to comment #8)
> Hmm, I'm just trying this and I'm not seeing it, could it be that I have an
> imap account that doesn't support tags?

I can re-produce the bug by the build of 20081022.
To re-produce the bug try:
1. create a new profile 

2. set up your IMAP account

3. Date column is sorted by default, click the upper-right corner in the 
   thread pane and uncheck the "Date" column to make it invisible and 
   check "Tag" column to make it visible.

4. click on the "Tag" to make the mails sorted by "Tag"

5. select a mail without any tag in the thread pane

6. set a tag on it by clicking the "Tag" in the toolbar.

7. click "Tag" column again to sorting by "Tag" again

You will see the new tagged message is not sorted by "Tag" as expected.

Comment 10

[Mark Banner (:standard8)]

I still can't reproduce. I'll get an account set up on a different server that allows tags and see if I can reproduce it then.

Comment 11

[Boying Lu]

(In reply to comment #10)
> I still can't reproduce. I'll get an account set up on a different server that
> allows tags and see if I can reproduce it then.

I can re-produce the bug with my Gmail IMAP account.

Comment 12

[Boying Lu]

Mark, can you re-produce the bug now?

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 343203 [details] [diff] [review]
remove atoi and printf

(In reply to comment #12)
> Mark, can you re-produce the bug now?

Sorry for the delay in this. I now have a server where I can reproduce the bug. Unfortunately I can reproduce the bug with or with the patch applied. I had to unbitrot it, but I think I did it right.

Let me know if you want more information for debugging this.

Comment 14

[Boying Lu]

(In reply to comment #13)
> (From update of attachment 343203 [details] [diff] [review])
> (In reply to comment #12)
> > Mark, can you re-produce the bug now?
> 
> Sorry for the delay in this. I now have a server where I can reproduce the bug.
> Unfortunately I can reproduce the bug with or with the patch applied. I had to
> unbitrot it, but I think I did it right.
> 
> Let me know if you want more information for debugging this.

I run the steps in comment #9.
I can still re-produce the bug on the latest trunk code 11/17/2008 with my Gmail  account 

Please set several emails tag e.g. "Work" before running the test case

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 343203 [details] [diff] [review]
remove atoi and printf

Boying, I will try again, I've just found out the build system was changed a while ago which means mailnews/base changes aren't picked up unless you build in that directory as well (see note on the newsgroups).

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 343203 [details] [diff] [review]
remove atoi and printf

Sorry for the delay in this, I think this is ok, and it seems to work. I can't quite remember why, but I'm sure a few days ago I was a little nervous about something. I can't remember what now :-( but bienvenu knows this code better than I do, so punting to him, and I'll do the sr if he's happy.

Comment 17

[David :Bienvenu]

yikes, I can't believe there's not a simpler way to fix this. It seems like the only thing that's really needed is to mark sort valid as false if a hdr's tag property changes, and the current sort is by tag.

Comment 18

[David :Bienvenu]

this seems to sort the tag column by the newest tag created in the set of tags for a message, which is pretty arbitrary. We currently sort by the whole set of tags applied to the msg, which is also arbitrary, but at least predictable. But the proposed patch will not sort "foo bar" separately from "goo bar" if bar is newer than goo or foo. So sorting by tags might produce an order like this:

"foo bar"
"goo bar"
"foo bar"

which doesn't seem right.

Comment 19

[Boying Lu]

Thanks a lot for your reply.

I'll continue to work on this bug.

Comment 20

[WADA:World Anti-bad-Duping Agency]

To Boying Lu(bug opener):

What is your expectation(or "should be") of "sort order of Tag"?

There are several orders in tag.
(1) Order of internal tag name(ABCD), and order of external tag name(VWXYZ)
    in tag definition by Tb.
    > user_pref("mailnews.tags.ABCDE.tag", "VWXYZ");
(2) Order of tags in X-Mozilla-Keys: header, when local mail folder.
    Basically sequence of addition of tag.
    But it may be different when tag overflow occurs.
(3) Order of tags in mailDB(.msf).
    If overflow of tag data occurs on X-Mozilla-Keys:,
    order of (2) and (3) may be different. 
(4) Order of tags(flags) in response returned by IMAP server.
    Is consistent when same IMAP server, same mail folder, same mail?
(5) When IMAP, there are flags for other than "Tag" for Tb.
    It possibly makes phenomenon complicated.

I think sort by external tag name is natural for user, and is easy to understand for user.
  (1) Tag definition. T_a=L_4, T_b=L_2, T_c=L_3, T_d=L_1
  (2) X-Mozilla-Keys: T_a, T_c, T_d, T_b
  => Thread pane display : L_1, L_2, L_3, L_4 (sorted by external name of tag)
  => Sort order          : Sort by L_p + L_q + L_r + L_s of a mail,
                           where   L_p < L_q < L_r < L_s 

But I think special care for $lable1 to $lable5 is needed.
> user_pref("mailnews.tags.$label1.tag", "Important");
> user_pref("mailnews.tags.$label2.tag", "Work");
> user_pref("mailnews.tags.$label3.tag", "Personal");
> user_pref("mailnews.tags.$label4.tag", "To Do");
> user_pref("mailnews.tags.$label5.tag", "Later");
=> Order of thread pane display :
   $label1(Important), $label2(Work), ... , $label5(Later), L_1, L_2, L_3, L_4
=> Sort order :
   Sort by $label1 + $label2 + ... $label5 + L_p + L_q + L_r + L_s of a mail

What do you think?

Comment 21

[Boying Lu]

(In reply to comment #18)
> this seems to sort the tag column by the newest tag created in the set of tags
> for a message, which is pretty arbitrary. We currently sort by the whole set of
> tags applied to the msg, which is also arbitrary, but at least predictable. But
> the proposed patch will not sort "foo bar" separately from "goo bar" if bar is
> newer than goo or foo. So sorting by tags might produce an order like this:
> 
> "foo bar"
> "goo bar"
> "foo bar"
> 
> which doesn't seem right.

I can't set nsMsgDBView::m_sortValid because:
1. There isn't a interface exported  or
2. I can't set m_sortValid in nsMsgDBView::OnHdrPropertyChanged either
   because we don't know the change is "Tag" or not.

In my patch, I think we don't sort "Tag" by their names, we sort them by their
"positions" in the tag list. see the bug 436017

Comment 22

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #21)
> In my patch, I think we don't sort "Tag" by their names, we sort them by their
> "positions" in the tag list. see the bug 436017

Is it "should be" you believe?
Bug 436017 Comment #3 says;
> Thunderbird sorts tags by importance.
> See bug 368084 comment 12 for details
> - basically TB needs a more advanced pref pane like SeaMonkey has.
Is your "positions" same as "importance of tag" by Bug 436017 Comment #3 (i.e. same as "importance of tag" by Bug 368084 comment #12)?

Comment 23

[Boying Lu]

(In reply to comment #22)
> (In reply to comment #21)
> > In my patch, I think we don't sort "Tag" by their names, we sort them by their
> > "positions" in the tag list. see the bug 436017
> 
> Is it "should be" you believe?
Yes.

> Bug 436017 Comment #3 says;
> > Thunderbird sorts tags by importance.
> > See bug 368084 comment 12 for details
> > - basically TB needs a more advanced pref pane like SeaMonkey has.
> Is your "positions" same as "importance of tag" by Bug 436017 Comment #3 (i.e.
> same as "importance of tag" by Bug 368084 comment #12)?
Yes.

Actually what I want to do is:
1. Associate a integer with each tag. e.g we can use the "index" in the tag list.
   and sort "Tags" by their indexs instead of their names.

2. Port the "Raise importance" and "Lower Importance" logic into TB, so user can
   define their own tags orders. (I tried it in the latest seamonkey, but it seems not work).

My patch posted here is to solve the first issue. If it is accepted, I want to 
solve the second one.

Comment 24

[Karsten Düsterloh]

Patching around a problem while at the same time providing the impression of not knowing the way things are working currently always makes me very nervous. Especially if they are in the backend...

> > Is it "should be" you believe?
> Yes.

I don't even understand the question you are answering so surely.
 
> > Is your "positions" same as "importance of tag" by Bug 436017
> > Comment #3 (i.e. same as "importance of tag" by Bug 368084 comment #12)?
> Yes.

Okay.

> Actually what I want to do is:
> 1. Associate a integer with each tag. e.g we can use the "index" in the tag
> list.
>    and sort "Tags" by their indexs instead of their names.

You _are_ aware that *preferences* are always sorted alphabetically?
The default sort order of tags is based upon this implied order, so no special sort key is needed. Only(!) if the implied order is not usable or wanted, we regard a special search key.
This special search key is alphanumeric, because we don't want to reindex the whole list just because we need a new integer index between 3 and 4...

> (I tried it in the latest seamonkey, but it seems not work).

What does that mean? What did you try and how does that "seem" not to work?

Comment 25

[Mark Banner (:standard8)]

Comment on attachment 343203 [details] [diff] [review]
remove atoi and printf

Cancelling reviews for now because based on at least David's comments, there needs to be a new patch.

Comment 26

[Boying Lu]

(In reply to comment #24)
> > Actually what I want to do is:
> > 1. Associate a integer with each tag. e.g we can use the "index" in the tag
> > list.
> >    and sort "Tags" by their indexs instead of their names.
> 
> You _are_ aware that *preferences* are always sorted alphabetically?
Really?  The tags in my "Tags" perference windows are listed in following orlder:

Important
Work
Personal
To Do
Later

But after sorting, mails are ordered in following order:
Important
Personal
To Do
Work

> The default sort order of tags is based upon this implied order, so no special
> sort key is needed. Only(!) if the implied order is not usable or wanted, we
> regard a special search key.

Does the "special search key" is the value returned by nsIMsgHdr->GetLabel()?

> This special search key is alphanumeric, because we don't want to reindex the
> whole list just because we need a new integer index between 3 and 4...
> 

I read the related source code and I got following:
1. All the "sorting" work is done in nsMsgDBView::Sort()
2. Data to be sorted is gotten by calling nsMsgDBView::GetCollationKey()
3. When sorting by Tags, the data is actually from nsMsgDBView::FetchTags()
   which is the value in "keywords" field. these value are strings shown in "tags" perference window

4. nsMsgDBView::Sort() does a quick sort by calling sorting function 
   nsMsgDBView::FnSortIdKey() which calls PL_strcmp() finally. The call sequence 
   is
   nsMsgDBView::FnSortIdKey() -->nsMsgDatabase::CompareCollationKeys() -->
   nsCollationUnix::CompareRawSortKey  --> PL_strcmp()

So I say those tags are sorted by the tag names.


> > (I tried it in the latest seamonkey, but it seems not work).
> 
> What does that mean? What did you try and how does that "seem" not to work?
I changed the tag order by clicking "Lower Importance" button and then sort mails by tags again. But mails still are sorted by their tag names instead of 
the order shown in the tag list.

Comment 27

[Karsten Düsterloh]

> > You _are_ aware that *preferences* are always sorted alphabetically?
> Really?

Yes.

> The tags in my "Tags" perference windows are listed in following orlder:
> 
> Important
> Work
> Personal
> To Do
> Later

Yes, because your prefs.js contains:

user_pref("mailnews.tags.$label1.color", "#FF0000");
user_pref("mailnews.tags.$label1.tag", "Important");
user_pref("mailnews.tags.$label2.color", "#FF9900");
user_pref("mailnews.tags.$label2.tag", "Work");
etc.

and these are ordered alphabetically - $label1 < $label2, $label1.color < $label1.tag. If you reordered the tags eg. using SeaMonkey, you may find this in your prefs.js:

user_pref("mailnews.tags.$label1.color", "#FF0000");
user_pref("mailnews.tags.$label1.ordinal", "$m");
user_pref("mailnews.tags.$label1.tag", "Important");
user_pref("mailnews.tags.$label2.color", "#FF9900");
user_pref("mailnews.tags.$label2.ordinal", "&");
user_pref("mailnews.tags.$label2.tag", "Work");
etc.

If read nsMsgTagService.cpp and apply it to the "Important" tag, you'll find:

GetOrdinal => $m      
GetKey     => $label1 

The method CompareMsgTags uses these to sort the tags.
Unfortunately, it's not a public method yet.

> But after sorting, mails are ordered in following order:
> Important
> Personal
> To Do
> Work

Yes, because sorting by tag was not implemented yet!

If you sort by tag, the sorting routine gets each message's string property "keywords", which contains a space delimited list of tag keys set for this message, in the order they got set. 

This means, for the values above, if you first set "Work", then "Important" on a tag-free message, it's property "keywords" has the value "$label2 $label1".

Sorting by tag means sorting by property "keywords" which of course is not particularly meaningful.

> Does the "special search key" is the value returned by nsIMsgHdr->GetLabel()?

No, see above.

> 3. When sorting by Tags, the data is actually from nsMsgDBView::FetchTags()
>    which is the value in "keywords" field. these value are strings shown in
> "tags" perference window

Not exactly, see above.

> I changed the tag order by clicking "Lower Importance" button and then sort
> mails by tags again. But mails still are sorted by their tag names instead of 
> the order shown in the tag list.

Sure, because a meaningful sort by tag wasn't implemented yet.


So, what needs to be done?

Sorting by tag should mean sorting by tag importance.
Thus we need a comparison function for Quicksort, which
- gets two "keywords" values and
- compares these based upon the tag order.
We can't do a lexical sort on the "keywords" string, because
- the lexical order of the contained tag keys does not represent tag
  importance,
- (at least in the IMAP case) external programs can change its value and
- importance changes would require touching all messages.

Comment 28

[Pander]

Please see comment 10 and 11 on bug 436017