427365 - Migrate Message Display prefpane

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect)

Description

[Bruno 'Aqualon' Escherl]

This bug is for the migration of MailNews' message display prefpane.

Comment 1

[Bruno 'Aqualon' Escherl]

Attachment: Migration of the prefpane v1

The patch migrates the message display pane to the new prefwindow. The license block from pref-viewing_messages.xul is missing cause I didn't know what exactly to write for initial developer and contributors. I asked about that in the mozilla.legal newsgroup and will include it when there's an answer to my question (see http://groups.google.com/group/mozilla.legal/browse_thread/thread/649f8c7eb336792f#)

Comment 2

[Bruno 'Aqualon' Escherl]

Attachment: diff -w version of patch v1

Comment 3

[[e-mail]]

Comment on attachment 313927 [details] [diff] [review]
diff -w version of patch v1

>+let checkbox;
>+let textbox;
Does that even make sense here? But I wouldn't bother with saving them anyway.

>+  textbox.disabled = (!checkbox.checked || isPrefLocked(textbox));
Note: you'll need to modify this to switch back to Startup.

>+  if (!textbox.disabled && !startingUp)
>+    textbox.focus();
Sadly this is unlikely to be safe in instantApply mode.

>+  if (!elem.hasAttribute("preference"))
>+    return false;
I'd rather hope that we could be sure that this would never happen ;-)

>-    function Startup() {
I'd prefer to keep Startup if possible.

>+                  oncommand="enableMarkMessagesReadAfter()"
This doesn't belong here. See the paragraph I just added to the How To ;-)

Comment 4

[Bruno 'Aqualon' Escherl]

Attachment: Migration of the prefpane v2

(In reply to comment #3)
> (From update of attachment 313927 [details] [diff] [review])
> >+  if (!textbox.disabled && !startingUp)
> >+    textbox.focus();
> Sadly this is unlikely to be safe in instantApply mode.

The code looks slightly different now, but it works in instantApply mode.

Comment 5

[Bruno 'Aqualon' Escherl]

Attachment: diff -w version of patch v2

Comment 6

[[e-mail]]

(In reply to comment #3)
>(From update of attachment 313927 [details] [diff] [review])
>>+  if (!textbox.disabled && !startingUp)
>>+    textbox.focus();
>Sadly this is unlikely to be safe in instantApply mode.
Steps to reproduce problem:
1. Open about:config
2. If necessary, set instant apply to true
3. Set mailnews.mark_message_read.delay to false (and leave it selected)
4. Open Preferences and view message display preferences (do not close Preferences!)
5. Switch back to the about:config window and press Enter to toggle the preference
Note that the focus is lost from about:config - if you switch back to Preferences you'll find that the focus went to the delay in seconds textbox (even if you selected a different panel but in that case it's harder to tell!)

Comment 7

[Bruno 'Aqualon' Escherl]

Attachment: Migration of the prefpane v3

This patch additionally checks, if the markMessagesRead checkbox is the focused element.

Comment 8

[[e-mail]]

Comment on attachment 315315 [details] [diff] [review]
Migration of the prefpane v3

Nice attachment number ;-)
315315 = 3 * 3 * 5 * 7 * 7 * 11 * 13

>+        <label value="&seconds.label;"
>+               id="secondsLabel"/>
<label id="secondsLabel" value="&seconds.label;"> on one line is fine.

Comment 9

[[e-mail]]

Comment on attachment 315315 [details] [diff] [review]
Migration of the prefpane v3

>+  let textbox = document.getElementById(aTextboxId);
>+  let pref = document.getElementById("mailnews.mark_message_read.delay.interval");
I've just realised that this line is wrong. The old line was
>-      textbox.disabled = (!checkbox.checked ||
>-                          parent.hPrefWindow.getPrefIsLocked(textbox.getAttribute("prefstring")));
In other words you need to use textbox.getAttribute("preference") instead of the literal string.

Comment 10

[Ian Neal]

Comment on attachment 315315 [details] [diff] [review]
Migration of the prefpane v3

>Index: mailnews/base/prefs/resources/content/pref-viewing_messages.js
>===================================================================
>+function Startup()
>+{
>+  let value = document.getElementById("mailnews.mark_message_read.delay").value;
>+  EnableTextbox("markMessagesReadAfter", value, false);
>+}
>+
>+function EnableMarkMessagesReadAfter(aValue)
>+{
>+  let focus = (document.getElementById("markMessagesRead") == document.commandDispatcher.focusedElement);
>+  EnableTextbox("markMessagesReadAfter", aValue, focus);
>+}
>+
>+function EnableTextbox(aTextboxId, aValue, aFocus)
As you are passing the same value for aTextboxId from both calls, just drop that argument and use "markMessagesReadAfter" directly.
>+{
>+  let textbox = document.getElementById(aTextboxId);
>+  let pref = document.getElementById("mailnews.mark_message_read.delay.interval");
>+  let enable = aValue && !pref.locked;
>+
>+  textbox.disabled = !enable;
>+
>+  if (!textbox.disabled && aFocus)
As you know what !textbox.disabled is already (enable) just use that instead.
>+    textbox.focus();
>+}

Comment 11

[Karsten Düsterloh]

Comment on attachment 315315 [details] [diff] [review]
Migration of the prefpane v3

Basically okay, given that Neil's and Ian's issues are fixed, plus

>Index: mailnews/base/prefs/resources/content/pref-viewing_messages.xul
>===================================================================
> <?xml-stylesheet href="chrome://communicator/skin/" type="text/css"?>
> <?xml-stylesheet href="chrome://messenger/skin/prefPanels.css" type="text/css"?>

You don't need these stylesheets.

>+  <!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd"> %brandDTD;

You don't need this dtd.


Minusing for the number of changes.

Comment 12

[Bruno 'Aqualon' Escherl]

(In reply to comment #8)
> (From update of attachment 315315 [details] [diff] [review])
> Nice attachment number ;-)
> 315315 = 3 * 3 * 5 * 7 * 7 * 11 * 13
Could we somehow reserve 431612 for the landing of all mailpref patches? ;-)

Comment 13

[Bruno 'Aqualon' Escherl]

Attachment: Migration of the prefpane v4

Patch has all review comments addressed.

Note: relies on patch for bug 429667

Comment 14

[Karsten Düsterloh]

Comment on attachment 316425 [details] [diff] [review]
Migration of the prefpane v4

>Index: mailnews/base/prefs/resources/content/pref-viewing_messages.xul
>===================================================================
>+<!DOCTYPE overlay [
>+  <!ENTITY % prefViewingMessagesDTD SYSTEM "chrome://messenger/locale/pref-viewing_messages.dtd"> %prefViewingMessagesDTD;
>+]>

This should be folded back into a single DOCTYPE line:

<!DOCTYPE overlay SYSTEM "chrome://messenger/locale/pref-viewing_messages.dtd">

r=me with that.

Comment 15

[Bruno 'Aqualon' Escherl]

Attachment: Migration of the prefpane v5

Patch ready for checkin after bug 429667 landed.

Comment 16

[Karsten Düsterloh]

Landed v5 on trunk.

Comment 17

[Bruno 'Aqualon' Escherl]

Thx for landing -> RESOLVED FIXED