342065 - wrong order of Tags in Message -> Tag menu and hotkeys

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect)

Description

[Alexander Kluss]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060619 SeaMonkey/1.5a
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/2006061910 SeaMonkey/1.5a

The old Labels (Important, To Do,...) have not the same order in the Tag submenu like on the preferences dialog.

Reproducible: Always

Comment 1

[Karsten Düsterloh]

This happens in TB also.

The menupopup is filled from the tagService's enumerators, which in turn get their data from the prefBranch - and that one uses a hash table to store the prefs, thus no nice ordering can be seen...

The old label names (Important/Work/Personal/To Do/Later) implied some kind of order, we should mimic that with tags also - and let the user reorder this in the pref panel. This order should also apply to the order tags are shown in the threadpane column.

Since we store the tag-keys with the messages, we can't reorder tags by renaming the keys - we had to change each message with that key! - so I propose to introduce the pref 
    mailnews.tags.<key>.index="<index#>"
and use that value for ordering the tags menupopup and the tags threadpane column. (This would make it very easy to readd the 0-5 (0-9 even) keys as requested by bug 341173!)

David, what do you think?

Comment 2

[Karsten Düsterloh]

This is actually a backend issue, and it happens on Mac, too.

As an example of the current situation:
These tag definitions:

user_pref("mailnews.tags.###.color", "");
user_pref("mailnews.tags.###.tag", "###");
user_pref("mailnews.tags.$$$$!.color", "");
user_pref("mailnews.tags.$$$$!.tag", "$$$$!");
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");
user_pref("mailnews.tags.$label3.color", "#009900");
user_pref("mailnews.tags.$label3.tag", "Personal");
user_pref("mailnews.tags.$label4.color", "#3333FF");
user_pref("mailnews.tags.$label4.tag", "To Do");
user_pref("mailnews.tags.$label5.color", "#993399");
user_pref("mailnews.tags.$label5.tag", "Later");
user_pref("mailnews.tags._.color", "");
user_pref("mailnews.tags._.tag", " ");
user_pref("mailnews.tags.__.color", "");
user_pref("mailnews.tags.__.tag", "  ");
user_pref("mailnews.tags.___.color", "");
user_pref("mailnews.tags.___.tag", "   ");
user_pref("mailnews.tags.huhu.color", "");
user_pref("mailnews.tags.huhu.tag", "huhu");
user_pref("mailnews.tags.neuestag.color", "");
user_pref("mailnews.tags.neuestag.tag", "neuestag");
user_pref("mailnews.tags.version", 1);

run through nsMsgTagService::GetTagEnumerator on this Mac in this order:

count=25
24: mailnews.tags.$$$$!.tag
23: mailnews.tags.###.color
22: mailnews.tags.huhu.tag
21: mailnews.tags.$label1.color
20: mailnews.tags.$label3.tag
19: mailnews.tags.__.color
18: mailnews.tags.$label4.tag
17: mailnews.tags.$label1.tag
16: mailnews.tags.huhu.color
15: mailnews.tags.###.tag
14: mailnews.tags.$label5.tag
13: mailnews.tags.$label5.color
12: mailnews.tags.$label2.color
11: mailnews.tags._.color
10: mailnews.tags.neuestag.tag
9: mailnews.tags.__.tag
8: mailnews.tags._.tag
7: mailnews.tags.$label4.color
6: mailnews.tags.___.tag
5: mailnews.tags.$label3.color
4: mailnews.tags.$label2.tag
3: mailnews.tags.$$$$!.color
2: mailnews.tags.___.color
1: mailnews.tags.version
0: mailnews.tags.neuestag.color

Comment 3

[David :Bienvenu]

What about just sorting the keys alphabetically, and returning the tags & keys in the same order? $label1..5 will naturally be at the beginning of the list in most situations, and will always be in order, in any case. I'm a little bit reluctant to add an other per tag pref, though I'm willing to be convinced.

Comment 4

[Alexander Kluss]

I would sort the Tags 1-5 in the same order like in the current and older releases. This is better for the usability. Most user would be missed the feature to set the first five Tags by hotkey and in the same order like today.

The other Tags could be sorted by the user and the userdefined Tags 6-9 get the hotkeys 6-9.

Comment 5

[Karsten Düsterloh]

Re comment #3:
> What about just sorting the keys alphabetically, and returning the tags & 
> keys in the same order? $label1..5 will naturally be at the beginning of the
> list in most situations, and will always be in order, in any case.

The key questions are: 
1. Do we want users to rename their tags?
2. Do we want users to control the order of their tags? 

If 1. is true, then sorting tags by a key that might have nothing in common with the actual "tag label" and, even worse, is *totally invisible* to a mere user is a bad idea. Such an order would look as arbitrary as it is now.
Furthermore, the key "folding" (" ()/{%*<>\\\"" -> '_' in AddTag) can lead to inobvious sort orders: "(tag)" < "tag" < "{tag}", but the keys are "_tag_", "tag", "_tag_"...

If 2. is true, then sorting by key would require the key to change when the user changes his tag order. This is not an option, since that would require us to change that key on every message it's used. :-/

We could, of course, sort by tag (not key) and leave that problem to the user ("if you want them ordered, rename them appropriately"), but that would be truly gruesome, IMO.

So we do need to store some sort of index with the tag definitions. We could save a pref by internally coding this index into the mailnews.tags.<key>.tag _value_ and strip it off before every usage, but that'd be quite messy; I'd rather not do that. :-/


Re comment #4:
> I would sort the Tags 1-5 in the same order like in the current and older
> releases.

As long as you don't add new tags, you won't even see any difference. As David mentioned, the keys of new tags will be almost always alphabetically after the old labels and new tags would be added to the end of the indexed list anyway.

Comment 6

[OpenMacNews]

as it's related ...

i'm using TB 20 nightly on OSX.

a feature which was available in TB 1.5.x, but missing (?) in TB 2.0.x, is the key-mapping of tags -- whether or not user-definable or orderable -- to keystrokes (e.g., cmd-1 -> "tag1")

i admit i can't current state categorically whether this HAD been provided by a now-missing extension ...

my $0.02.

thx.

Comment 7

[Alexander Kluss]

(In reply to comment #6)

> a feature which was available in TB 1.5.x, but missing (?) in TB 2.0.x, is the
> key-mapping of tags -- whether or not user-definable or orderable -- to
> keystrokes (e.g., cmd-1 -> "tag1")

This is a bug in the current nightlies.
https://bugzilla.mozilla.org/show_bug.cgi?id=341173

Comment 8

[Adam Guthrie]

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

Comment 9

[Peter Lairo]

IMO, Thunderbird needs the ability and UI to rearrange Tags.

The main reason (for me) are consistency, predictability, and user-control. Including the ability to predictably use the number keys (1 to n) to un-/assign tags. Unfortunately, the numbers are allotted according to the order in which the tags were created (newest = 1, right?) and thus they change.

The user should be able to control the order of his tags. That way it is much easier to find a tag in a menu and to remember which tag has which number.

This UI could be added to the "Tools / Options / Display / Tags" dialog via [Move Up] and [Move Down] buttons:

+-------------------------------------+
| 1 Work             |    [ Add ]     |
| 2 Personal         |   [ Delete ]   |
| 3 Computers        |                |
| 4 Politics         |   [ Move Up ]  |  <-- Add "Move Up" button
| 5 ToDo             |  [ Move Down ] |  <-- Add "Move Down" button
| 6 Important        |                |
+--------------------+                |
|                                     |
|                    [OK ] [ Cancel ] |
+-------------------------------------+

BTW: It would be even better if a column was added to show the current shortcut key (and use a tooltip to explain!) (see above)

PS: The current "selection" of a tag makes the tag description nearly unreadable. 

Comment 10

[Karsten Düsterloh]

Attachment: tag sorting (backend patch v0)

Back-end changes:
- spin off tag data into a separate nsIMsgTagInfo which contains key, name, color and ordinal (for sorting), plus shortcut functions for read/write/delete
- provide enumerator nsIMsgTagEnumerator for nsIMsgTagInfo objects
- use above stuff in nsIMsgTagService

Comment 11

[Karsten Düsterloh]

Attachment: tag sorting (frontend patch v0 for both TB and SM)

Front-end changes made necessary by the above back-end changes.
Mostly killing keyEnumerator and changed enumerator signature.

Comment 12

[Karsten Düsterloh]

Attachment: tag sorting (backend patch v1)

Removed two unneeded includes.
Slightly more useful sort order: don't priorize the mere existence of ordinal strings, just use them instead of the key as comparison strings.

Comment 13

[[e-mail]]

Comment on attachment 235651 [details] [diff] [review]
tag sorting (backend patch v1)

>+NS_GENERIC_FACTORY_CONSTRUCTOR(nsMsgTagInfo)
>+NS_GENERIC_FACTORY_CONSTRUCTOR(nsMsgTagEnumerator)
(etc. etc...) Nobody actually needs to "construct" these.

>+  void Read();    // read tag info for this key from preferences
>+  void Write();   // write tag info for this key to preferences
>+  void Delete();  // delete tag info for this key from preferences
IDL style is to use lower case.

>+  // enumerate all tags
>+  readonly attribute nsIMsgTagEnumerator tagEnumerator; // of nsIMsgTagInfo
You might find it easier to have a method
void getAllTags([retval, array, size_is(count)] out nsIMsgTag tagArray,
                out PRUint32 count);

>+ *   Karsten DŸsterloh <[e-mail]>
Aren't we supposed to use UTF-8? Or at least ISO-8859-1.

>+  rv = prefBranch->GetChildList(TAG_PREF_BRANCH, &count, &prefList);
If you use a real pref branch then the results won't include the pref name which you may find useful.

>+    rv = AddTagForKey(labelKey, ucsval, csval, NS_LITERAL_CSTRING(""));
EmptyCString()

>+ *   Karsten Düsterloh <[e-mail]>
Ah, that's better :-)

Comment 14

[[e-mail]]

Comment on attachment 235574 [details] [diff] [review]
tag sorting (frontend patch v0 for both TB and SM)

>                 children[5].selectedItem = newMenuItem;
>-                children[5].value = key;
>+                children[5].value = taginfo.key;
Actually the selectedItem setter should already copy the value... and the value setter also sets the selected item, too!

Comment 15

[Karsten Düsterloh]

Attachment: tag sorting (backend patch v2)

Changes to v1:
- renamed nsMsgTagInfo to nsMsgTag, made all attributes readonly, dropped public functions
- killed nsMsgTagEnumerator, return an array of nsMsgTag objects instead (btw: retval arguments need to be last in the function signature)
- changed nsMsgDBView::AppendKeywordProperties to always return the most important tag

Comment 16

[Karsten Düsterloh]

Attachment: tag sorting (frontend patch v2 for both TB and SM)

Updated frontend to backend changes.

Comment 17

[David :Bienvenu]

Comment on attachment 235758 [details] [diff] [review]
tag sorting (backend patch v2)

you don't need these, right? They're only ever constructed internally.

 //
+// nsMsgTagInfo
+//
+#define NS_MSGTAGINFO_CONTRACTID \
+  "@mozilla.org/messenger/taginfo;1"
+
+#define NS_MSGTAGINFO_CID \
+{ /* 84d593a3-5d8a-45e6-96e2-9189acd422e1 */ \
+ 0x84d593a3, 0x5d8a, 0x45e6, \
+ { 0x96, 0xe2, 0x91, 0x89, 0xac, 0xd4, 0x22, 0xe1}}
+
+//

+  const char* element1 = *NS_STATIC_CAST(const char* const *, aTagPref1);
+  const char* element2 = *NS_STATIC_CAST(const char* const *, aTagPref2);
+  // don't compare the pref branch
+  return strcmp(element1, element2);


no need for the local variables...

Comment 18

[Karsten Düsterloh]

(In reply to comment #17)
> +// nsMsgTagInfo
...
> no need for the local variables...

Uh, yeah, both remainders of the reconstruction. :-/ 

Comment 19

[[e-mail]]

Comment on attachment 235759 [details] [diff] [review]
tag sorting (frontend patch v2 for both TB and SM)

No pref-tags changes?

>+              var taginfo = tagArray[i];
>               var newMenuItem = document.createElement('menuitem');
>-              newMenuItem.setAttribute('label', tag);
>-              newMenuItem.setAttribute('value',  key);
>+              newMenuItem.setAttribute('label', taginfo.tag);
>+              newMenuItem.setAttribute('value', taginfo.key);
I don't mind either way but I would have inlined taginfo myself.

>+  var tagCount = {};
>+  var tagArray = tagService.getAllTags(tagCount);
>+  tagCount = tagCount.value;
Rather than doing this I think it's more readable if you use
var tagArray = tagService.getAllTags({});
and use tagArray.length instead of tagCount.value everywhere.
(I particularly don't like tagCount = tagCount.value;)

>   var index = 0;
>-  while (allTags.hasMore())
>+  while (index < tagCount)
>   {
>-    var tag = allTags.getNext();
>-    var key = allKeys.getNext();
>+    var taginfo = tagArray[index];
>     // TODO we want to either remove or "check" the tags that already exist
>     var newMenuItem = document.createElement("menuitem");
>-    SetMessageTagLabel(newMenuItem, ++index, tag);
>-    newMenuItem.setAttribute("value", key);
>+    SetMessageTagLabel(newMenuItem, ++index, taginfo.tag);
This is confusing. Please convert this to a for loop instead.

Comment 20

[Karsten Düsterloh]

> No pref-tags changes?

SM pref panel for tags is bug 342560.

> >+              var taginfo = tagArray[i];
...
> >+              newMenuItem.setAttribute('label', taginfo.tag);
> >+              newMenuItem.setAttribute('value', taginfo.key);
> I don't mind either way but I would have inlined taginfo myself.

How smart is the JS engine? I usually shun requesting an array element twice if I can get it once and use it twice...

> >+  var tagCount = {};
> >+  var tagArray = tagService.getAllTags(tagCount);
> >+  tagCount = tagCount.value;
> Rather than doing this I think it's more readable if you use
> var tagArray = tagService.getAllTags({});
> and use tagArray.length instead of tagCount.value everywhere.

tagArray.length is not necessarily equal to tagCount, tagArray.length is the number of tag preferences. See the backend patch. I really welcome reasonable ways to cut the array down to the mere tag count there, though...

> (I particularly don't like tagCount = tagCount.value;)

Okay, probably worth its own var (or rather: const). Again, a question of JS engine smartness: does it recognize that several uses of the same expression do in fact have the same value?
 
> >+    SetMessageTagLabel(newMenuItem, ++index, taginfo.tag);
> This is confusing. Please convert this to a for loop instead.

Okay. 

Comment 21

[[e-mail]]

Comment on attachment 235758 [details] [diff] [review]
tag sorting (backend patch v2)

>+  // don't compare the pref branch
Obsolete comment?

>+  nsresult rv = m_prefBranch->GetChildList(TAG_PREF_BRANCH, &count, &prefList);
You might find it easier to make m_prefBranch the TAG_PREF_BRANCH branch which would save you from accounting for TAG_PREF_BRANCH everywhere. Or maybe that's too big a change for this patch.

>+    if (StringEndsWith(nsDependentCString(prefList[i]), NS_LITERAL_CSTRING(TAG_PREF_SUFFIX_TAG)))
>     {
>       nsAutoString curTag;
>       GetUnicharPref(prefList[i], curTag);
>       if (aTag.Equals(curTag))
>       {
>         aKey = Substring(nsDependentCString(prefList[i]), 14, strlen(prefList[i]) - 18);
If you write nsDependentCString foo(prefList[i]); then you could write foo.Length() which would save calling strlen again.

>+  return m_prefBranch->SetCharPref(prefName.get(), PromiseFlatCString(ordinal).get());
These are annoying, but I guess you don't want to use string idl types.

>-/* readonly attribute nsIStringEnumerator tagEnumerator; */
>-NS_IMETHODIMP nsMsgTagService::GetTagEnumerator(nsIStringEnumerator * *aTagEnumerator)
>+/* void getAllTags (out unsigned long count, [array, size_is (count), retval] out nsIMsgTag tagArray); */
>+NS_IMETHODIMP nsMsgTagService::GetAllTags(PRUint32 *count, nsIMsgTag ***tagArray)
You should use the a prefix i.e. aCount, aTagArray.

>+  // build an array of nsIMsgTag elements from the orderered list
>+  // it's at max the same size as the preflist, but usually only about half
>+  *tagArray = new (nsIMsgTag*)[prefCount];
I think you should use the nsMemory allocator (or NS_Alloc).

>+    nsDependentCString keyInfo = nsDependentCString(prefList[i]);
>+    keyInfo.BeginReading(keyStart);
>+    keyInfo.EndReading(keyEnd);
>+    if (RFindInReadable(NS_LITERAL_CSTRING("."), keyStart, keyEnd))
Mightn't it be easier to use strrchr?

>+        // new key found: read the respective pref values
>+        nsMsgTag tag;
>+        rv = tag.Read(key);
>+        if (NS_SUCCEEDED(rv))
>+        {
>+          // store the tag info read in our array
>+          (*tagArray)[currentTagIndex++] = new nsMsgTag(tag);
>+        }
This is a bit clumsy. You should use nsMsgTag *tag = new nsMsgTag(); [or possibly new nsMsgTag(key);] then additionally tag->Read(key, this); [tag->Read(this)] would save you from getting the service inside Read(). You also don't handle an allocation failure, and finally you need to NS_ADDREF all your tags (I think that would explain the crash I saw).

>+  // and clear the rest of the array
Not 100% sure that's necessary, nobody will know ;-)

>+  nsCOMPtr<nsIMsgTagService> mMsgTagService;
You don't need this to be a member, you only use it the once. (And as I mentioned you could pass it in as a parameter).

Comment 22

[Karsten Düsterloh]

Attachment: tag sorting (backend patch v3)

Changes to v2:
- removed nsMsgTag constructor factory entries
- grab the tag pref branch on init, so that I can use just relative prefnames later on
- use of NS_Alloc and added missing NS_ADDREF
- several suggested code simplifications
I did not change the basic way of filling the nsMsgTag array, because I prefer constructing new elements only when needed over deleting uselessly created elements.

Comment 23

[Karsten Düsterloh]

Attachment: tag sorting (frontend patch v3 for both TB and SM)

Updated frontend to backend changes v3, in particular:
- using tagArray.length instead of grabbing the tagCount out value
- for loop instead of weird while loop

Carrying over granted r+.

Comment 24

[[e-mail]]

(In reply to comment #20)
>tagArray.length is not necessarily equal to tagCount
Actually it is, XPConnect is forced to use your count to create the array.

Comment 25

[[e-mail]]

Comment on attachment 237010 [details] [diff] [review]
tag sorting (backend patch v3)

>+  nsCAutoString prefName(key);
>   return m_prefBranch->DeleteBranch(prefName.get());
m_prefBranch->DeleteBranch(PromiseFlatCString(key).get()); perhaps?

>+      nsCAutoString key(Substring(prefName, 0, info - prefList[i]));
You can write Substring(prefList[i], info); and you don't then need prefName at all. In fact I believe you can declare
const nsACString& key(Substring(prefList[i], info));
or something... biesi says it's an nsCSubstring but whatever works ;-)

[I still don't like the tag reading and copying...
I'm guessing you're doing it to make error handling easier]

Comment 26

[[e-mail]]

Comment on attachment 237010 [details] [diff] [review]
tag sorting (backend patch v3)

Sorry, I didn't read the previous comment... an alternative to uselessly creating nsMsgTag objects would be to fetch the tag prefs in GetAllTags and only create the nsMsgTag if that succeeds.

Comment 27

[David :Bienvenu]

Comment on attachment 237010 [details] [diff] [review]
tag sorting (backend patch v3)

thx for doing this, Karsten!

Comment 28

[Karsten Düsterloh]

Attachment: tag sorting (backend patch v4)

Addressed Neil's final comments, especially inlining the unnecessary nsMsgTag::Read function. No further front end changes needed.
Carrying over granted review flags.

Comment 29

[Karsten Düsterloh]

Comment on attachment 237012 [details] [diff] [review]
tag sorting (frontend patch v3 for both TB and SM)

Checked in into trunk.

Comment 30

[Karsten Düsterloh]

Comment on attachment 237847 [details] [diff] [review]
tag sorting (backend patch v4)

Checked in into trunk.
Both the backend patch v4 and the frontend patch v3 are needed to fix this on the 1.8.1 branch, too.

Comment 31

[Mike Schroepfer]

David B/MScott - are you good with this for the 1.8.1 branch?

Comment 32

[David :Bienvenu]

yes, fine with me, after a little baking on the trunk.

Comment 33

[[e-mail]]

Comment on attachment 237847 [details] [diff] [review]
tag sorting (backend patch v4)

>+            rv = GetColorForKey(key, color);
>+            if (NS_FAILED(rv))
>+              color = EmptyCString();
>+            // .ordinal MAY exist
>+            rv = GetOrdinalForKey(key, ordinal);
>+            if (NS_FAILED(rv))
>+              ordinal = EmptyCString();
Nit: should use .Truncate() to clear a string.

>+  nsMsgTag(const nsACString &aKey,
>+           const nsAString  &aTag,
>+           const nsACString &aColor,
>+           const nsACString &aOrdinal);
Nit: more efficient to use the non-A versions here.

Comment 34

[Mike Schroepfer]

Comment on attachment 237847 [details] [diff] [review]
tag sorting (backend patch v4)

a=schrep for TBird/SM patch.

Comment 35

[Mike Schroepfer]

Comment on attachment 237012 [details] [diff] [review]
tag sorting (frontend patch v3 for both TB and SM)

a=schrep for 181drivers for SM/TBird only patch

Comment 36

[Mike Cowperthwaite]

This is much better!  Thank you Karsten!

Comment 37

[Karsten Düsterloh]

Attachment: fix variable name typo in nsMsgTagService::AddTagForKey

Correct the meaning of the if clause.

Comment 38

[Karsten Düsterloh]

Attachment: better AddTagForKey fix

Fixing the typo in AddTagForKey revealed that it will throw an exception when deleting a non-existing ordinal...

Comment 39

[Karsten Düsterloh]

Attachment: better AddTagForKey fix, v2

Kill erroneous color value check also.

Comment 40

[Karsten Düsterloh]

Comment on attachment 238823 [details] [diff] [review]
better AddTagForKey fix, v2

Checked in into trunk.

Comment 41

[Mike Cowperthwaite]

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

Comment 42

[Karsten Düsterloh]

Checked in into MOZILLA_1_8_BRANCH.
Please note that SM's pref panel needs bug 342560 to be fully functional; any regressions should be filed as new bugs.

Comment 43

[Scott MacGregor]

thanks Karsten. adding the fixed1.8.1 keyword

Comment 44

[Frédéric Buclin]

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

Comment 45

[Frédéric Buclin]

verified fixed on the 1.8.1 branch using Tb2.0b1 20060926.

Comment 46

[Scott MacGregor]

clearing the blocking flag. This patch is already on the branch.

Comment 47

[Daniel Kabs, reporting bugs since 2002]

(In reply to comment #9)
> IMO, Thunderbird needs the ability and UI to rearrange Tags.

Moved this idea to bug #528034.