349991 - tag mailview names out of sync with tags

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Tuukka Tolvanen (sp3000)]

I had customized label names previously. With the move to tags, the mailviews filtering dropdown still lists the old customized label names, which don't match the current tag names. That part of the dropdown is not editable, so presumably it should use the current list of tags.

tbird trunk 20060820 linux

Comment 1

[Karsten Düsterloh]

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

Comment 2

[Chris Ilias [:cilias]]

I'm assuming this is a dup of what I was going to report. Which is that when adding a new tag, a new mailview should automatically be created for that tag.

Comment 3

[Karsten Düsterloh]

Scott, 
are you actively working on this?

Comment 4

[Karsten Düsterloh]

Attachment: Tags submenu for view picker and menus (SM and TB)

Just replacing the fixed number of labels with a potentially large number of tag menuitems would make the view picker menus very long, outside of control by the user. Thus I moved the dynamic tag list in its own submenu.
Furthermore, I merged the code for menu creation, so while the patch seems to be quite large, it's actually a win wrt lines of code... ;-)
(Observing tag prefs is useless due to bug 343600.) 

Neil, since we will have SM1.1 L10N freeze soon (and I'd like to see this fix in SM 1.1), I'd like to *add* the needed entities to SM's msgViewPickerOverlay.dtd even before this patch as a whole has been reviewed. Is that okay for you?

Stealing bug.

Comment 5

[[e-mail]]

Comment on attachment 243866 [details] [diff] [review]
Tags submenu for view picker and menus (SM and TB)

>+        <menupopup id="viewPickerPopup" onpopupshowing="RefreshViewPopup(this);">
This will get called every time the tags popup is opened, which is less than optimal. At the very least you should check that event.target == this first but alternatively you could have two handlers one for the tags and one for the custom views. One thought was to move the custom views to a submenu each with its own refresh.

>+          <menuitem id="viewPickerAll" value="0" type="radio" label="&viewAll.label;" accesskey="&viewAll.accesskey;"/>
No accesskey or type="radio" in menulists please.

>-const kLabelOffset = 1;  // 1=2-1, from msgViewPickerOveraly.xul, <menuitem value="2" id="labelMenuItem1"/>
Don't you still need this?

>+  // Mac View menu menuitems don't have XBL bindings
That was the original reason, but there's an outstanding bug on implementing all the properties.

>+dump("ViewChangeByValue: aValue="+aValue+"\n")
These aren't permanent right ;-)

>+      val = folderInfo.getUint32Property(kViewCurrent, kViewItemAll);
>+    }
>   }
>+  return String(val);
Nit: there's only one case where val isn't already a string.
(Also personally I prefer .toString())

>+    // we can"t map tags back to labels in general,
" isn't an apostrophe :-P

>-          setTimeout('ViewChangeByValue('+result+')', 0);
>+          setTimeout('ViewChangeByValue("'+result+'")', 0);
Note: not our code but that's not how you parametrise a timer callback.

I don't see why you can't land the .dtd changes now and see how the rest of the patch fares when I finish reviewing it ;-)

Comment 6

[Karsten Düsterloh]

> One thought was to move the custom views to a submenu each with its own
> refresh.

That might be best indeed.

> >+          <menuitem id="viewPickerAll" value="0" type="radio" label="&viewAll.label;" accesskey="&viewAll.accesskey;"/>
> No accesskey or type="radio" in menulists please.

Why no radio?

> >-const kLabelOffset = 1;  // 1=2-1, from msgViewPickerOveraly.xul, <menuitem value="2" id="labelMenuItem1"/>
> Don't you still need this?

No. It wasn't even used before: the old code uses a hardcoded -1 when processing labels...

> >+dump("ViewChangeByValue: aValue="+aValue+"\n")
> These aren't permanent right ;-)

No, but I hardly get non-trivial patches "through" on first try anyway. ;-)

> >+    // we can"t map tags back to labels in general,
> " isn't an apostrophe :-P

Doh. I did a s/'/"/g to make all strings use the same delimiters... :)

> >-          setTimeout('ViewChangeByValue('+result+')', 0);
> >+          setTimeout('ViewChangeByValue("'+result+'")', 0);
> Note: not our code but that's not how you parametrise a timer callback.

Yeah, I'd have used setTimeout(ViewChangeByValue, 0, result) instead.

> I don't see why you can't land the .dtd changes now and see how the rest
> of the patch fares when I finish reviewing it ;-)

Done so, including entities for a possible Custom Views submenu.

Comment 7

[[e-mail]]

Comment on attachment 243866 [details] [diff] [review]
Tags submenu for view picker and menus (SM and TB)

>+    // but maybe it"s a former label view we can migrate (label views used values 2-6)
>+    if ((kViewItemTags <= numval) && (numval < kViewItemVirtual))
>+    {
>+      tagkey = "$label" + (numval - 1);
>+      numval = kViewItemTags;
>+      aValue = kViewTagMarker + tagkey;
>+    }
Isn't this only possible when reading the saved view from the database, in which case you should convert it there?

The tags and views don't seem to mix very well, I'm wondering whether ViewChange should take three parameters (view, key, label).

Comment 8

[Karsten Düsterloh]

Attachment: Tags and CustomViews submenus for view picker and menus (SM and TB)

Implemented most of Neil's comments, especially moving the custom views in their own submenu. I did not split up the parameters passed to ViewChange as it'd only complicate matters - with the move of the old keyword migration I only needed to check for isNaN...
This patch is against 1.8 branch, but should work almost (modulo the already happened SM string check-in) identically on trunk - Mac trunk is kinda hard to test with these days. :|

Comment 9

[[e-mail]]

Comment on attachment 245388 [details] [diff] [review]
Tags and CustomViews submenus for view picker and menus (SM and TB)

>+  if (isNaN(aValue))
>+  {
>+    // split off the tag key
>+    var tagkey = String(aValue).substr(kViewTagMarker.length);
If it's not a number, then it must already be a string, no?
I think I saw this in a couple of other places too.

>+    // we can't map tags back to labels in general,
>+    // so set view to none for backwards compatibility in this case
>+    folderInfo.setUint32Property (kViewCurrent, isNaN(aValue) ? kViewItemNone : aValue);
Unfortunately kViewItemNone is -1, which isn't a Uint...
Is there any reason not to use kViewItemAll everywhere instead?

>+// recreate the entries for tags and custom views
>+// and mark the current view's menuitem
>+function RefreshViewPopup(aViewPopup, aIsMenulist)
>+{
>+  // refresh the tags submenu
Unnecessary comment?

>+  var menupopups = aViewPopup.getElementsByTagName("menupopup");
>+  if (menupopups && menupopups.length)
Check for length = 2 perhaps?
Also note that getElementsByTagName never returns null.

>+  // mark default views if selected
>+  var viewAll = aViewPopup.getElementsByAttribute("value", kViewItemAll)[0];
>+  viewAll.setAttribute("checked", gCurrentViewValue == kViewItemAll);
>+  var viewUnread = aViewPopup.getElementsByAttribute("value", kViewItemUnread)[0];
>+  viewUnread.setAttribute("checked", gCurrentViewValue == kViewItemUnread);
Not for menulists surely?

>Index: themes/classic/global/mac/menu.css
I think the other classic menu.css files might need tweaking too.

Comment 10

[Karsten Düsterloh]

Attachment: View picker and menus (SM and TB), v3

Fixed Neil's comments except the styles for Unix. Mac is included, Windows is already okay, OS2 looks similar to Windows (though I can't test), but I can't test Unix atm (and it doesn't have neither a menu.css nor a similar rule set, afaics). 
This patch, again, is against 1.8 branch (where it should land anyway, too), but should apply to trunk as well.

Comment 11

[[e-mail]]

I just found a regression :-(
View/Threads no longer works.
Error: ViewMessagesBy is not defined
Source File: chrome://messenger/content/commandglue.js
Line: 381

Comment 12

[Karsten Düsterloh]

Attachment: View picker and menus (SM and TB), v4 (checked in)

Fixed the missing callers. Patch still against 1.8 branch.

Comment 13

[David :Bienvenu]

Comment on attachment 246501 [details] [diff] [review]
View picker and menus (SM and TB), v4 (checked in)

thx for the patch...

Comment 14

[Karsten Düsterloh]

Attachment: trunk version of v4 (checked in)

Landed trunk version.

Comment 15

[Karsten Düsterloh]

Comment on attachment 246501 [details] [diff] [review]
View picker and menus (SM and TB), v4 (checked in)

Needing SM 1.1 approvals, too.

Comment 16

[Ian Neal]

(In reply to comment #15)
> (From update of attachment 246501 [details] [diff] [review] [edit])
> Needing SM 1.1 approvals, too.
> 
a=me for SM 1.1, adding late-l10n keyword due to string changes

Comment 17

[Karsten Düsterloh]

> a=me for SM 1.1, adding late-l10n keyword due to string changes

SM String changes were already checked in before L10N freeze, see <http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/mailnews/extensions/mailviews/resources/locale/en-US&command=DIFF_FRAMESET&file=msgViewPickerOverlay.dtd&rev1=1.3.76.1&rev2=1.3.76.2&root=/cvsroot>. This patch will only remove the cruft there.

Comment 18

[Karsten Düsterloh]

Comment on attachment 246501 [details] [diff] [review]
View picker and menus (SM and TB), v4 (checked in)

Landed on MOZILLA_1_8_BRANCH.