341010 - No labels, no actions in message filters dialog after check-in of bug 114656

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Ruediger Lahl]

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

The patch for Bug-ID 114656 cause two bugs.

The menues Message->Label and Tools->Message Filters->new 'perform these action' are empty.

It's not possible to build new filters, or edit old filters.

Reproducible: Always

Steps to Reproduce:
1. open Message->Labels
2. open Tools->Message Filters->new

Actual Results:  
broken menues

Expected Results:  
ability to select Labels and to build filters

Comment 1

[David :Bienvenu]

does applying this help? https://bugzilla.mozilla.org/attachment.cgi?id=223992

Comment 2

[Ruediger Lahl]

(In reply to comment #1)
> does applying this help? https://bugzilla.mozilla.org/attachment.cgi?id=223992

Sorry David. I can find bugs, but can't read code :-(

Comment 3

[Mike Cowperthwaite]

That attachment is the un-reviewed patch at bug 114656 for Seamonkey.

Comment 4

[Karsten Düsterloh]

The SM patch in bug 114656 isn't "complete enough" to fix both problems mentioned here, it only fixes (part of) the first. New patch upcoming.

Comment 5

[Karsten Düsterloh]

Attachment: Fix SM breakage

Update SM to the TB implementation of bug 114656 to make it usable again, including Neil's recommended fixes. 
We still need to fix:
- labels/tags pref panel
- label/tag threadpane colouring
- label/tag shortcut keys (0-5)
- message views
I'd probably like to attack those here, too, but in another patch, thus requesting r/sr for the immediate breakage fixes.

Comment 6

[Karsten Düsterloh]

Oh, and:
- the threadpane column still reads "Label"
- only the first tag is found in searches via the search dialog

Comment 7

[[e-mail]]

Comment on attachment 225210 [details] [diff] [review]
Fix SM breakage

The "Tag" menuitems need to be disabled if no message is selected.

Comment 8

[[e-mail]]

Comment on attachment 225210 [details] [diff] [review]
Fix SM breakage

Mostly fine except for no disabling of the tag menus.

>+    content/messenger/pref-junk.xul                                            (base/prefs/resources/content/pref-junk.xul)
>+    content/messenger/pref-junk.js                                             (base/prefs/resources/content/pref-junk.js)
>+    locale/en-US/messenger/pref-junk.dtd                                (base/prefs/resources/locale/en-US/pref-junk.dtd)
Not part of this patch, right? The pref-junk files aren't even checked in.

>+function TagCurMessage(key)
>+{
>+  // ###need to do all selected messsages
Please use XXX it's "standard" ;-)
Note: this will be tricky in virtual search folders!

>+    var command = ((keySet) ? "Un" : "") + "TagCurMessage(" + "'" + key + "');";
This isn't going to work for tags with a ' in the name... maybe a shared event handler on the menupopup? (The checked state will have been toggled by the time the event handler fires.)

>-    <menuseparator id="messagePaneContext-sep-labels-1"/>
>+    <menuseparator id="messagePaneContext-sep-tags-1"/>
Don't forget -sep-labels-2

>+    <menu id="messagePaneContext-tags" label="&tagMenu.label;" accesskey="&tagMenu.accesskey;">
>+      <menupopup id="messagePaneContext-tagpopup" onpopupshowing="InitMessageTags('messagePaneContext')">
>+        <menuitem id="messagePaneContext-tagNew"
These probably need to be tagMenu, menuPopup-tagpopup and something else.

>   window.alert(alertText);
window.alert is deprecated, the title [JavaScript Application] is hardcoded.

>+  dialog.OKButton.disabled = !/^\S+$/.test(dialog.nameField.value);
Ho hum. "To Do" has a space. And we split tags on spaces...

Note: the new tag dialog's <separator> shouldn't be necessary, but I guess the dialog will need to be expanded to work with colours anyway.

Comment 9

[[e-mail]]

Comment on attachment 225210 [details] [diff] [review]
Fix SM breakage

>+  dialog.OKButton.disabled = !/^\S+$/.test(dialog.nameField.value);
Forget this test, it looks as if the tag service internally normalizes tags.

Comment 10

[Karsten Düsterloh]

[junk lines in jar.mn|
> Not part of this patch, right? 

Yep. Sorry.

> >+  // ###need to do all selected messsages
> Please use XXX it's "standard" ;-)
> Note: this will be tricky in virtual search folders!

Okay, even though it's just copied promise from the TB code. ;-)
(I found no immediate solution else I'd fixed it here.)

> >+    var command = ((keySet) ? "Un" : "") + "TagCurMessage(" + "'" + key + "');";
> This isn't going to work for tags with a ' in the name...

Yeah, but luckily nsMsgTagService::AddTag strips "s. I think I'll fold those two functions and get rid of that hack altogether.

> >+  dialog.OKButton.disabled = !/^\S+$/.test(dialog.nameField.value);
> Ho hum. "To Do" has a space. And we split tags on spaces...

We split _keys_ on spaces, not _tags_. See the comment in nsIMsgTagService.idl.

> Note: the new tag dialog's <separator> shouldn't be necessary, but I guess the
> dialog will need to be expanded to work with colours anyway.

Actually, I don't like that "New Tag..." dialog at all, but it's already there and for the moment will do. I'd rather see a "Customize..." there, directly linked to the pref panel where you can add/remove tags and edit their colours...

Comment 11

[Karsten Düsterloh]

Attachment: SM tags breakage fix, v2

Updated to Neil's comments, plus some clean-up:
* disable tag context menus when no message is selected
* fixed some broken ids 
* folded (Un)TagCurMessage functions into ToggleMessageTag, eliminating name construction voodoo
* use promptservice instead of window.alert
* undo an unintentional ellipsis elimination

Comment 12

[[e-mail]]

(In reply to comment #10)
>>>+    var command = ((keySet) ? "Un" : "") + "TagCurMessage(" + "'" + key + "');";
>> This isn't going to work for tags with a ' in the name...
>Yeah, but luckily nsMsgTagService::AddTag strips "s. I think I'll fold those
>two functions and get rid of that hack altogether.
>>>+  dialog.OKButton.disabled = !/^\S+$/.test(dialog.nameField.value);
>> Ho hum. "To Do" has a space. And we split tags on spaces...
>We split _keys_ on spaces, not _tags_. See the comment in nsIMsgTagService.idl.
Yeah, I realised that afterwards, thus comment #9 ;-)

Comment 13

[[e-mail]]

Comment on attachment 225454 [details] [diff] [review]
SM tags breakage fix, v2

>+    newMenuItem = document.createElement("menuitem");
>+    newMenuItem.setAttribute("label", tag);
>+    newMenuItem.setAttribute("value", key);
>+    newMenuItem.setAttribute("type", "checkbox");
>+    // if we already have this tag, change the command to "untag"
>+    var addKey  = (" " + curKeys + " ").indexOf(" " + key + " ") == -1;
>+    // key doesn't contain quote chars, see nsMsgTagService::AddTag
>+    var command = 'ToggleMessageTag("' + key + '", ' + addKey + ')';
>+    newMenuItem.setAttribute('oncommand', command);
>+    newMenuItem.setAttribute('checked', !addKey);
Knowing that the key doesn't contain quote characters makes it not quite so bad, but then the merging of the toggle code isn't quite so necessary.
What I had actually considered was a ToggleMessageTag(event.target); which would extract the value and checked attributes to figure out what to do.

>+  promptService.alert(window, alertText, alertText);
I think alert(window, document.title, alertText) would be more useful.

>+  dialog.OKButton.disabled = !/^\S+$/.test(dialog.nameField.value);
You reminded me that spaces are allowed but forgot to change this to !dialog.nameField.value :-P sr=me with this fixed.

Comment 14

[Karsten Düsterloh]

> What I had actually considered was a ToggleMessageTag(event.target);

Yeah, that'd even better.

> >+  dialog.OKButton.disabled = !/^\S+$/.test(dialog.nameField.value);
> You reminded me that spaces are allowed but forgot to change this to
> !dialog.nameField.value :-P sr=me with this fixed.

<note2self>Remember That Case Matters<note2self>:
Actually, we should disallow tags made of whitespaces only, so I'd rather set
dialog.OKButton.disabled = /^\s*$/.test(dialog.nameField.value);

Comment 15

[Karsten Düsterloh]

Attachment: Additional cleanup, v3

Changes:
* using ToggleMessageTagCmd(event.target) to avoid string foo now
* using document.title for new tag warning now
* fixed wrong callback for new tag
* reconsidered new tag value check: if the user wants to have a tag made up of whitespace, let him do that (he'll be able to change that later in the preferences panel once that is working again)

Keeping sr+, rerequesting r?.

Comment 16

[Ian Neal]

Comment on attachment 225659 [details] [diff] [review]
Additional cleanup, v3

>Index: mailnews/base/resources/content/mailWindowOverlay.js
>===================================================================
>+function InitMessageTags(menuType)
>+{
>+  var tagService = Components.classes["@mozilla.org/messenger/tagservice;1"]
>+                             .getService(Components.interfaces.nsIMsgTagService);
>+  var allTags = tagService.tagEnumerator;
>+  var allKeys = tagService.keyEnumerator;
>+  // remove any existing non-static entries...
>+  var menuItemId = menuType + "-tagpopup";
>+  var menupopupNode = document.getElementById(menuItemId);
>+  for (var i = menupopupNode.childNodes.length; i > 2; --i)
>+    menupopupNode.removeChild(menupopupNode.firstChild);
>+  var menuseparator = menupopupNode.firstChild;
>+  // now rebuild the list
>+  var msgHdr = gDBView.hdrForFirstSelectedMessage;
>+  var curKeys = msgHdr.getStringProperty("keywords");
>+  var newMenuItem;
Either move the var above into the while loop or the ones in the loop out.

>+
>+  while (allTags.hasMore())
>+  {
>+    var tag = allTags.getNext();
>+    var key = allKeys.getNext();
>+    // TODO we want to either remove or "check" the tags that already exist
>+    newMenuItem = document.createElement("menuitem");
>+    newMenuItem.setAttribute("label", tag);
>+    newMenuItem.setAttribute("value", key);
>+    newMenuItem.setAttribute("type", "checkbox");
>+    var removeKey = (" " + curKeys + " ").indexOf(" " + key + " ") > -1;

r=me with that change.

Comment 17

[Karsten Düsterloh]

Attachment: Final version for check-in, v4 [checked in on trunk]

Comment 18

[Karsten Düsterloh]

Comment on attachment 225783 [details] [diff] [review]
Final version for check-in, v4 [checked in on trunk]

Checked in on trunk; leaving bug open until all regressions from 114656 have been fixed (see dependencies there).

Comment 19

[Ruediger Lahl]

(In reply to comment #18)
> (From update of attachment 225783 [details] [diff] [review] [edit])
> Checked in on trunk; leaving bug open until all regressions from 114656 have
> been fixed (see dependencies there).

Are they fixed meanwhile, so we can mark this bug as FIXED?

Comment 20

[Karsten Düsterloh]

> Are they fixed meanwhile, so we can mark this bug as FIXED?

Yes, the only bigger "bug" remaining is bug 349991, but that one shouldn't block this here.