416548 - Migrate MailNews preference subpanes to the new prefwindow.

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect)

Description

[Karsten Düsterloh]

Bug 404263 already migrated the MailNews main pref pane, but there's more work to be done:

Message Display
Notifications
Composition
Send Format
Addressing
Junk Mail
Tags 
Return Receipts
Character Encoding
Offline & Disk Space

Bug 394522 is rather crowded by now, so spinning this off; we probably need to spin off other bugs from here.

Comment 1

[Mark Banner (:standard8)]

(In reply to comment #0)
> Addressing

Already covered by bug 408613

Comment 2

[Karsten Düsterloh]

Attachment: Tags, v1

Let's start with the tags panel, plus some minor additional ID cleanup.

Comment 3

[[e-mail]]

Comment on attachment 311378 [details] [diff] [review]
Tags, v1

>+#tags_pane .listheader-label
It would be better to do #tagList > listhead

>+#tags_pane listcell
And #tagList > listitem > listcell

>+#tags_pane textbox
And #tagList > listitem > listcell > textbox
Or you could just set .flex = 1 in the script.

>+function BisectString(aPrev, aNext)
I think I see what this is for - you add a tag in the middle of the list, then start typing its name, so you don't know its key yet and you need to make up an ordinal, but I don't see why you can't do things the old way in the non-instant apply case.
I'm also worried that, in the instant apply case, if you could create the tag, start using it, then change its label, it would confuse the tag service, as the tag would be deleted and recreated.
Also, did you consider swapping the ordinals when you use MoveTag?

Comment 4

[Karsten Düsterloh]

(In reply to comment #3)
> >+function BisectString(aPrev, aNext)
> I think I see what this is for - you add a tag in the middle of the list,
> then start typing its name, so you don't know its key yet and you need to
> make up an ordinal, but I don't see why you can't do things the old way in
> the non-instant apply case.

Actually, the reason for this function is rather independent from instant vs. non-instant apply. I just need a way to generate ordinals between other ordinals.

> I'm also worried that, in the instant apply case, if you could create the tag,
> start using it, then change its label, it would confuse the tag service, as
> the tag would be deleted and recreated.

Why should it?
One tag is deleted and another one is created, usually even with a different name. In fact I didn't notice any oddities while playing around with it.

> Also, did you consider swapping the ordinals when you use MoveTag?

Hm, that way I'd only need new ordinals when adding tags, true, but I'd still need BisectString then. And it would probably mean having ordinals in cases where actually none would be needed else.

I'll update the CSS as soon as Ian has commented.

Comment 5

[Ian Neal]

Comment on attachment 311378 [details] [diff] [review]
Tags, v1

>Index: mailnews/base/prefs/resources/content/pref-labels.js
>===================================================================
> function AppendTagEntry(aTagInfo, aRefChild)
> {
>   // Creating a colorpicker dynamically in an onload handler is really sucky.
>   // You MUST first set its type attribute (to select the correct binding), then
>   // add the element to the DOM (to bind the binding) and finally set the color
>   // property(!) afterwards. Try in any other order and fail... :-(
You have a "var key = aTagInfo.key;" after this line, is it still needed?

>@@ -157,18 +125,19 @@ function AppendTagEntry(aTagInfo, aRefCh
> 
>   var colorCell = document.createElement('listcell');
>   var colorpicker = document.createElement('colorpicker');
>   colorpicker.setAttribute('type', 'button');
>   colorCell.appendChild(colorpicker);
> 
>   var entry = document.createElement('listitem');
>   entry.addEventListener('focus', OnFocus, true);
>+  entry.addEventListener('change', OnChange, false)
Nit: Missing ;

>+function RecalculateOrdinal(aEntry)
>+{
>+  // Calculate a new ordinal for the given entry, assuming that both its
>+  // predecessor's and successor's are correct, i.e. ord(p) < ord(s)!
>+  var tagInfo = aEntry.tagInfo;
>+  // get neighbouring ordinals
>+  var prevOrdinal = '', nextOrdinal = '';
>+  var prev = aEntry.previousSibling;
>+  if (prev && prev.nodeName == 'listitem') // first.prev == listhead
>+    prevOrdinal = GetTagOrdinal(prev.tagInfo);
>+  var next = aEntry.nextSibling;
>+  if (next)
>+  {
>+    nextOrdinal = GetTagOrdinal(next.tagInfo);
>+  }
>+  else
>+  {
>+    // ensure key < nextOrdinal if entry is the last/only entry
>+    nextOrdinal = prevOrdinal || tagInfo.key;
>+    nextOrdinal = String.fromCharCode(nextOrdinal.charCodeAt(0) + 2);
>+  }
>+
>+  var ordinal = tagInfo.key;
If you move this line further up then only need to retrieve tagInfo.key the once.

>+  if (prevOrdinal < ordinal && ordinal < nextOrdinal)
>+  {
>+    // no ordinal needed, just clear it
>+    SetTagOrdinal(tagInfo, '')
>+    return;
>+  }

> 
> function OnOK()
> {
>   var i;
Nit: This line is no longer needed.

>Index: mailnews/base/prefs/resources/content/pref-labels.xul
>===================================================================

>+<!--
> <?xul-overlay href="chrome://global/content/globalOverlay.xul"?>
> <?xul-overlay href="chrome://communicator/content/utilityOverlay.xul"?>
>-
>+-->
Nit: Remove these lines if they are no longer needed.

As discussed on IRC there's also the possible renaming of these files too.

r=me with those items addressed

Comment 6

[Karsten Düsterloh]

Attachment: Tags, v2 [checked in]

I landed this on trunk.
It contains addressed review comments and also includes changes for and the move from pref-labels.* to pref-tags.*.

Comment 7

[Mark Banner (:standard8)]

I've just fixed the last remaining blocker to this bug, so this is now fixed :-)