Closed Bug 416548 Opened 12 years ago Closed 12 years ago

Migrate MailNews preference subpanes to the new prefwindow.

Categories

(SeaMonkey :: Preferences, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mnyromyr, Assigned: mnyromyr)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
(In reply to comment #0)
> Addressing

Already covered by bug 408613
Depends on: 408613
Attached patch Tags, v1 (obsolete) — Splinter Review
Let's start with the tags panel, plus some minor additional ID cleanup.
Attachment #311378 - Flags: superreview?(neil)
Attachment #311378 - Flags: review?(iann_bugzilla)
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?
Attachment #311378 - Flags: superreview?(neil) → superreview+
(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 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
Attachment #311378 - Flags: review?(iann_bugzilla) → review+
Depends on: 427365
I landed this on trunk.
It contains addressed review comments and also includes changes for and the move from pref-labels.* to pref-tags.*.
Attachment #311378 - Attachment is obsolete: true
Attachment #314387 - Flags: superreview+
Attachment #314387 - Flags: review+
Depends on: 428594
Depends on: 428705
Depends on: 429015
Depends on: 429143
Depends on: 445009
Depends on: 445010
Depends on: 445011
Depends on: 445012
I've just fixed the last remaining blocker to this bug, so this is now fixed :-)
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.