Closed Bug 341010 Opened 15 years ago Closed 14 years ago

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

Categories

(SeaMonkey :: MailNews: Message Display, defect)

x86
All
defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bugzille, Assigned: mnyromyr)

References

Details

(Keywords: fixed-seamonkey1.1b, fixed1.8)

Attachments

(1 file, 3 obsolete files)

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
Version: unspecified → Trunk
does applying this help? https://bugzilla.mozilla.org/attachment.cgi?id=223992
Status: UNCONFIRMED → NEW
Ever confirmed: true
(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 :-(
That attachment is the un-reviewed patch at bug 114656 for Seamonkey.
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.
Assignee: mail → mnyromyr
Summary: spinoff to bug-ID 114656 no Labels, no Messagefilter properties → No labels, no actions in message filters dialog after check-in of bug 114656
Attached patch Fix SM breakage (obsolete) — Splinter Review
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.
Attachment #225210 - Flags: superreview?(neil)
Attachment #225210 - Flags: review?(iann_bugzilla)
Oh, and:
- the threadpane column still reads "Label"
- only the first tag is found in searches via the search dialog
Comment on attachment 225210 [details] [diff] [review]
Fix SM breakage

The "Tag" menuitems need to be disabled if no message is selected.
Blocks: 207550
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.
Attachment #225210 - Flags: superreview?(neil) → superreview-
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.
[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...
Attached patch SM tags breakage fix, v2 (obsolete) — Splinter Review
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
Attachment #225210 - Attachment is obsolete: true
Attachment #225454 - Flags: superreview?(neil)
Attachment #225454 - Flags: review?(iann_bugzilla)
Attachment #225210 - Flags: review?(iann_bugzilla)
Depends on: 114656
(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 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.
Attachment #225454 - Flags: superreview?(neil) → superreview+
> 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);
Attached patch Additional cleanup, v3 (obsolete) — Splinter Review
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?.
Attachment #225454 - Attachment is obsolete: true
Attachment #225659 - Flags: superreview+
Attachment #225659 - Flags: review?(iann_bugzilla)
Attachment #225454 - Flags: review?(iann_bugzilla)
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.
Attachment #225659 - Flags: review?(iann_bugzilla) → review+
Attachment #225659 - Attachment is obsolete: true
Attachment #225783 - Flags: superreview+
Attachment #225783 - Flags: review+
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).
Attachment #225783 - Attachment description: Final version for check-in, v4 → Final version for check-in, v4 [checked in on trunk]
(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?
> 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.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.