Closed Bug 435079 Opened 12 years ago Closed 12 years ago

Migrate Composer's New Page Settings prefs to the new prefpane

Categories

(SeaMonkey :: Composer, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.0a1

People

(Reporter: stefanh, Assigned: stefanh)

References

Details

Attachments

(1 file, 6 obsolete files)

Filing this since I've started to work on it...
Attached patch WIP patch (obsolete) — Splinter Review
Just attaching a wip patch. Needs some testing - I think it should work, though. This patch also removes the pref-publish panel and its .dtd file - not sure when the panel was used (has it ever been used?)- it's commented out in the overlay file.

This version of the patch uses a broadcaster in order to disable/enable labels/buttons when customColors radio values are changed. It also improves the handling of locked prefs a bit. That said, I've spent some time thinking of what to do with locked "editor.*_color" prefs, but I haven't found any solution that seems reasonable.
OK, so I tested this (attachment #323735 [details] [diff] [review]) on mac with instantApply set to "false" in about:config, and everything seems to work. I still like to think a bit more on this, though.
Target Milestone: --- → seamonkey2.0alpha
Another question that probably should be discussed: Do we want to move everything in editor/ui to some other location? I can imagine that stuff that both seamonkey and thunderbird uses could go to one place (should be settled with thunderbird folks) and the seamonkey-specific pref files could go to another place (suite/somewhere).
Comment on attachment 323735 [details] [diff] [review]
WIP patch

>+  toggleElement("editorAuthor");
>+  toggleElement("useCustomColors");
>+  toggleElement("backgroundImageInput");

As IanN points out, I don't need those (and the function)
I started to look into how one should be able to honor the locked editor color prefs. I have a version that doesn't work 100% yet, I'll investigate more when I'm back (from vacation).
Hmm, I can't use the broadcaster if I want to honor locked editor.*_color" prefs...
Attached patch New version (obsolete) — Splinter Review
Here's another WIP. This should work 100%. This one also uses new styling on the default attribute. MailNews not tested yet. Comments are welcome - not sure if my strategy is the best...
(In reply to comment #7)

> MailNews not tested yet.

Forget that, not sure why I thought that mattered...


+  for (var i = 0; i < buttons.length; i++)
+  {
+    let isLocked = isLocked(buttons[i].getAttribute("id"));

Oops, this doesn't work (of course).
Attached patch Another version (obsolete) — Splinter Review
Some points of interest:

1) Removes pref-publish.xul and pref-publish.dtd
2) No more dependencies on editor js
3) One global var left
4) Honors locked prefs (even the editor.*_color ones). 
5) In order to make the win/nix ui respond to browser.display* (etc) colour changes in about:config, "instantapply="true" is set on the browser colours preference elements. I'm not sure if this is considered hacky, but it works - for some reason I couldn't make it work without this.

6) In order to remove the color well colour (spacer bg), the editor version of setColorwell uses the "default" attribute and removes the style attribute. It seems to me that should be enough to use the "opacity: 0;" on 'default="true"' in toggleElements, executed before any colour is set. If a editor color pref is locked and you switch to use custom colors, colors will be set but they won't show up because of the opacity rule set in toggleElements. That said, I wonder if there's a better way of doing this than what is done in toggleElements (previousSibling etc)...

7) selectImageFile is without the hack in http://mxr.mozilla.org/seamonkey/source/editor/ui/dialogs/content/EdDialogCommon.js#210. It seems to work fine for me without it on mac (I'm not sure how to test it, but I haven't noticed anything), but I don't know about other OS.

8) We could get rid of the getColorAndUpdatePref function if we added a new color picker file for the composer prefs. I'm not 100% satisfied with my solution of passing id and types (looks ugly)...
Comment on attachment 327330 [details] [diff] [review]
Another version

Please see comment #10 for more info.
Attachment #327330 - Flags: superreview?(neil)
Attachment #327330 - Flags: review?(neil)
Status: NEW → ASSIGNED
Comment on attachment 327330 [details] [diff] [review]
Another version

Eww, our editor default colours aren't the same as our browser default colours. That got me rather confused!

>-  // Setting to blank color will remove color from buttons,
>-  setColorWell("textCW",       "");
>-  setColorWell("linkCW",       "");
>-  setColorWell("activeCW",     "");
>-  setColorWell("visitedCW",    "");
>-  setColorWell("backgroundCW", "");
If you don't want to do that, then put !important on the CSS, not opacity.

>+  if ((previewID == "normalText" || previewID == "ColorPreview"))
((Double vision))

>+                  instantApply="true"
This is no good, I managed to change my browser colours without hitting OK :-(
[open preferences, open new page preferences, open appearance/colours, change]
Attachment #327330 - Flags: superreview?(neil)
Attachment #327330 - Flags: review?(neil)
Attachment #327330 - Flags: review-
Hmm, it also seems that using the onchange event on the same preference elements in different panels is not a god idea... Open preferences, open new page prefs, open appearance/colours and then change the "Use system colors" prefs and notice the little js warning from pref-colors.js ("Warning: Empty string passed to getElementById().") I can make that worse if I first load both panels (new page prefs first) and then change the pref in about:config:

Error: document.getElementById(element.getAttribute("preference")) is null
Source File: chrome://communicator/content/pref/pref-colors.js
Line: 53

As Karsten points out, you can't really have the same id's in the 2 panels... I have to use nsIPrefBranch2 instead.
Some changes:

- Now using browser pref observers in order to make the ui respond to about:config changes in browser colors

- function names now starts with uppercase. Not sure if I like it, but it looks like it was the original style and it will make Mnyromyr happy

- changed a few id's in the xul file. All id's except "ColorPreview" (which depends on css style rules) now begins with lowercase

I'll have a -w version up in a few minutes
Attachment #323735 - Attachment is obsolete: true
Attachment #327282 - Attachment is obsolete: true
Attachment #327330 - Attachment is obsolete: true
Attachment #328536 - Flags: superreview?(neil)
Attachment #328536 - Flags: review?(neil)
Comment on attachment 328536 [details] [diff] [review]
Take 2, use browser pref observers etc

+{ // add browser prefs observers

Whoops, ignore that (should start with uppercase there)
Attached patch Take 2, -w version (obsolete) — Splinter Review
Comment on attachment 328536 [details] [diff] [review]
Take 2, use browser pref observers etc

Did one of the versions update the editor preview if you updated the browser colours even in non-instant-apply mode, or have I been dreaming?

>+const browserPrefsObserver = 
Nit: trailing space

>+    switch(aData)
Nit: space before (

>+  if(customColors)
Nit: space before (

>+    let isLocked = CheckLocked(buttons[i].getAttribute("id"));
Nit: .id

>+    gPreviewBGColor = color;
>+    UpdateBgPreview(document.getElementById("editor.default_background_image").value);
UpdateBgPreview is the only user of gPreviewBGColor, so you should pass it in as a parameter rather than abusing a global. r+sr=me with that fixed.

>+    document.getElementById(previewID).setAttribute("style","color: " + color + ";");
Nit: space after ,
Attachment #328536 - Flags: superreview?(neil)
Attachment #328536 - Flags: superreview+
Attachment #328536 - Flags: review?(neil)
Attachment #328536 - Flags: review+
If you're OK with it, I'd like to do what I did in bug 411226:

1) I land the re-named file
2) I then land #328536 (with updated paths and addressed comments), and at the same time I remove the old file
3) I post a "this is what I landed" patch
Attachment #328567 - Flags: superreview?(neil)
Attachment #328567 - Flags: review?(neil)
Comment on attachment 328567 [details] [diff] [review]
re-name pref-composer.js to pref-editing.js

I think you should a) land the patch using the wrong filenames b) wait for us to switch to hg c) create a new patch for the rename.
Attachment #328567 - Flags: superreview?(neil)
Attachment #328567 - Flags: superreview-
Attachment #328567 - Flags: review?(neil)
Attachment #328567 - Flags: review-
Ah, right - that makes sense.
(In reply to comment #18)
> UpdateBgPreview is the only user of gPreviewBGColor
You're right, it isn't, I didn't think to look in the XUL. Sorry.
Hmm, maybe I could split UpdateBgPreview somehow and get rid of the global...
Comment on attachment 328536 [details] [diff] [review]
Take 2, use browser pref observers etc

I'll attach a new patch once I've solved comment #23
Attachment #328536 - Attachment is obsolete: true
- Due to usage of style.backgroundColor etc gPreviewBGColor is now gone (thanks to Neil who suggested it)
- Parameter names are now prefixed with "a"
- A few function names/comments have been changed, also added a few blank lines
Attachment #328541 - Attachment is obsolete: true
Attachment #328567 - Attachment is obsolete: true
Attachment #329081 - Flags: superreview?(neil)
Attachment #329081 - Flags: review?(neil)
Filed bug 444761 for the rename (comment #20)
Comment on attachment 329081 [details] [diff] [review]
Take 3, without gPreviewBGColor

>+      default:
>+        SetBgAndFgColors(gPrefService.getBoolPref("browser.display.use_system_colors"))
Nit: you don't actually need to call SetBgAndFgColors in the system colour case because they're already the correct colours.

>+  colorPreview.style.backgroundImage = !aImage ? "" : "url(" + aImage + ")";
Nit: aImage && "url(" + aImage + ")";
Attachment #329081 - Flags: superreview?(neil)
Attachment #329081 - Flags: superreview+
Attachment #329081 - Flags: review?(neil)
Attachment #329081 - Flags: review+
(In reply to comment #27)
>(From update of attachment 329081 [details] [diff] [review])
>>+      default:
>>+        SetBgAndFgColors(gPrefService.getBoolPref("browser.display.use_system_colors"))
>Nit: you don't actually need to call SetBgAndFgColors in the system colour case
>because they're already the correct colours.
You're right, you need this in case it was that preference that changed.

>>+  colorPreview.style.backgroundImage = !aImage ? "" : "url(" + aImage + ")";
>Nit: aImage && "url(" + aImage + ")";
Instead of !aImage ? "" : "url(" + aImage + ")";
Landed (cvs trunk) with 'aImage && "url(" + aImage + ")";':

Checking in editor/ui/jar.mn;
/cvsroot/mozilla/editor/ui/jar.mn,v  <--  jar.mn
new revision: 1.24; previous revision: 1.23
done
Checking in editor/ui/composer/content/editorPrefsOverlay.xul;
/cvsroot/mozilla/editor/ui/composer/content/editorPrefsOverlay.xul,v  <--  editorPrefsOverlay.xul
new revision: 1.16; previous revision: 1.15
done
Checking in editor/ui/composer/content/pref-composer.js;
/cvsroot/mozilla/editor/ui/composer/content/pref-composer.js,v  <--  pref-composer.js
new revision: 1.18; previous revision: 1.17
done
Checking in editor/ui/composer/content/pref-editing.xul;
/cvsroot/mozilla/editor/ui/composer/content/pref-editing.xul,v  <--  pref-editing.xul
new revision: 1.41; previous revision: 1.40
done
Removing editor/ui/composer/content/pref-publish.xul;
/cvsroot/mozilla/editor/ui/composer/content/pref-publish.xul,v  <--  pref-publish.xul
new revision: delete; previous revision: 1.7
done
Checking in editor/ui/locales/jar.mn;
/cvsroot/mozilla/editor/ui/locales/jar.mn,v  <--  jar.mn
new revision: 1.8; previous revision: 1.7
done
Removing editor/ui/locales/en-US/chrome/composer/pref-publish.dtd;
/cvsroot/mozilla/editor/ui/locales/en-US/chrome/composer/pref-publish.dtd,v  <--  pref-publish.dtd
new revision: delete; previous revision: 1.2
done
Checking in suite/themes/classic/editor/EditorDialog.css;
/cvsroot/mozilla/suite/themes/classic/editor/EditorDialog.css,v  <--  EditorDialog.css
new revision: 1.28; previous revision: 1.27
done
Checking in suite/themes/modern/editor/EditorDialog.css;
/cvsroot/mozilla/suite/themes/modern/editor/EditorDialog.css,v  <--  EditorDialog.css
new revision: 1.42; previous revision: 1.41
done

Note: I'll try and get this in to mozilla-central in the coming week.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
reopening to ensure editor/ui lands in mozilla-central; adding checkin-needed per stefanh on IRC.
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---
Comment on attachment 329081 [details] [diff] [review]
Take 3, without gPreviewBGColor

Adding to http://wiki.mozilla.org/Sheriff_Duty/PendingCheckins for sdwilsh.

Checkin Comment:
Bug 435079 - Migrate Composer's New Page Settings prefs to the new prefpane
NPOTB (FF/XulRunner)
r+sr=NeilAway, p=stefanh

feel free to edit comment as you see best :-)
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/c32a70debffa
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
The two obsoleted files, pref-publish.xul and pref-publish.dtd, was not removed in the mozilla-central landing.
(In reply to comment #33)
> The two obsoleted files, pref-publish.xul and pref-publish.dtd, was not removed
> in the mozilla-central landing.
> 
Thanks Hasse - this is now fixed (also fixed Neils last nit in comment #27):

http://hg.mozilla.org/index.cgi/mozilla-central/rev/5f8f1247789f
You need to log in before you can comment on or make changes to this bug.