Bug 394522 (prefpane_migration)

Migrate SeaMonkey preferences panes to use <preferences>

RESOLVED FIXED in seamonkey2.0a1

Status

SeaMonkey
Preferences
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: Karsten Düsterloh, Assigned: Karsten Düsterloh)

Tracking

(Blocks: 1 bug)

Trunk
seamonkey2.0a1
Dependency tree / graph
Bug Flags:
blocking-seamonkey2.0a1 +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(8 attachments, 14 obsolete attachments)

44.29 KB, patch
Callek
: review+
neil@parkwaycc.co.uk
: superreview+
Details | Diff | Splinter Review
36.88 KB, patch
Karsten Düsterloh
: review+
Karsten Düsterloh
: superreview+
Details | Diff | Splinter Review
8.42 KB, patch
Karsten Düsterloh
: review+
Karsten Düsterloh
: superreview+
Details | Diff | Splinter Review
4.10 KB, patch
neil@parkwaycc.co.uk
: review+
neil@parkwaycc.co.uk
: superreview+
Details | Diff | Splinter Review
41.22 KB, patch
Karsten Düsterloh
: review-
Details | Diff | Splinter Review
38.77 KB, patch
standard8
: review+
Details | Diff | Splinter Review
654 bytes, patch
Karsten Düsterloh
: review+
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
45.84 KB, patch
Karsten Düsterloh
: review+
Karsten Düsterloh
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

11 years ago
Now that bug 342087 is fixed, we finally can use toolkit's new preferences architecture without losing our distinctive preferences UI.
Callek started work on this in attachment 229635 [details] [diff] [review], which we need to continue here.

This will mean we need to 
(a) rewrite pref.xul to use <prefwindow> and <prefpane> (and kill preftree.xul)
(b) rewrite *all* our pref panels to use <preferences> etc. 
(c) kill nsPrefWindow.js and nsWidgetStateManager.js
(Assignee)

Comment 1

11 years ago
Created attachment 283452 [details] [diff] [review]
WIP: main pref window, global interim changes

This is a first WIP patch for the new style preferences framework in SM:
- The new preferences main window will be named preferences.xul (as in TB or FF). This will allow us to keep the old preferences window around until all panels are migrated. The patch includes some then-to-be-deleted code for that.
- To ease the pain of having to specify both a prefpane element and a treeitem in the main preferences window, this patch makes prefwindow.xml create such prefpanes automatically.

This WIP patch does not include panel migration, but any such attempt at least needs to make the pref file's main element be an <overlay> including the actual <prefpane> element, whose id must match those specified in the prefpane/url attribute in preferences.xul. I'll provide a demo panel migration soon.
Assignee: prefs → mnyromyr
Status: NEW → ASSIGNED
(Assignee)

Updated

11 years ago
Depends on: 397095, 397135
(Assignee)

Comment 2

11 years ago
Comment on attachment 283452 [details] [diff] [review]
WIP: main pref window, global interim changes

Requesting review to get some thoughts if the general direction is acceptable.
Attachment #283452 - Flags: review?(neil)

Comment 3

11 years ago
Comment on attachment 283452 [details] [diff] [review]
WIP: main pref window, global interim changes

>+    openWindow(null, "chrome://communicator/content/pref/preferences.xul",
>+               "chrome,titlebar,dialog=no,resizable", "");
This will reselect the last selected pane, right? Presumably we can preset lastSelected so that it will default to navigator on a new profile?

>-  var resizable;
The new pref window is always resizable? Hmm...

>+    <resources>
>+      <stylesheet src="chrome://communicator/skin/preferences.css"/>
>+    </resources>
Out of interest, what's the intention here?

>Index: suite/themes/classic/jar.mn
Modern? ;-)

Think of this as an sr+ rather than an r+ :-)
Attachment #283452 - Flags: review?(neil) → review+

Updated

11 years ago
Blocks: 399031

Comment 4

11 years ago
Comment on attachment 283452 [details] [diff] [review]
WIP: main pref window, global interim changes

All in all I like the direction this took, even better than my initial concept if you can believe it... anyway brief review of this WiP

>Index: suite/common/bindings/prefwindow.xml
>+          // add the class "prefnavtree", so that themes can set defaults
>+          aTreeElement.className += ' prefnavtree';

Nit: when |initNavigationTree| is called with another tree should we remove this className somehow? (p.s. I may be simply missing it, but do we hide extra <tree>'s that get pulled in to there? [if we do, ignore the nit])

>+            if (!pane && autopanes)
>+            {
>+              // if we have a url, create a <prepane> for it
>+              var paneURL = node.getAttribute('url');

Nit: <prefpane> not <prepane>

>-              this.showPane(pane);
>+              try
>+              {
>+                this.showPane(pane);
>+              }
>+              catch (e)
>+              {
>+                dump('***** broken prefpane: ' + pane + '\n' + e + '\n');
>+                pane = null;
>+              }

Without knowing top-of-my-head why showPane would throw (xml parsing error for overlay loading?) would a better error message here make sense? [maybe paneURL instead for one]

>Index: suite/themes/classic/jar.mn
>+  skin/classic/communicator/preferences.css                             (communicator/preferences.css)

This file missing from patch, and from modern (as neil noted)
Attachment #283452 - Flags: review+
(Assignee)

Comment 5

11 years ago
Created attachment 286063 [details] [diff] [review]
main pref window, global interim changes, v1

Architecture patch, introducing the new main pref dialog preferences.xul and temporary changes to allow having both pref dialogs at once for a transition period, plus some functionality to ease the panel migration.

(In reply to comment #3)
> This will reselect the last selected pane, right? Presumably we can preset
> lastSelected so that it will default to navigator on a new profile?

The last selected panel is only relevant, if the dialog does not get a panel argument on startup, but that is usually the case anyway.

> >-  var resizable;
> The new pref window is always resizable? Hmm...

Yes. I see no point in prohibiting (temporary) adjustments by the user here.

> >+    <resources>
> >+      <stylesheet src="chrome://communicator/skin/preferences.css"/>
> >+    </resources>
> Out of interest, what's the intention here?

Otherwise, the dialog elements like tree or header would stick to the borders. Doesn't look very nice...

> >Index: suite/themes/classic/jar.mn
> Modern? ;-)

Added.

(In reply to comment #4)
> >Index: suite/common/bindings/prefwindow.xml
> >+          // add the class "prefnavtree", so that themes can set defaults
> >+          aTreeElement.className += ' prefnavtree';
> 
> Nit: when |initNavigationTree| is called with another tree should we remove
> this className somehow?

I don't think this is worth the effort to solve, calling this with another tree should be rare enough to pack that burden upon those who do so.

> Nit: <prefpane> not <prepane>

Heh! Done.

> Without knowing top-of-my-head why showPane would throw (xml parsing error for
> overlay loading?) 

Or trying to load a pane twice, etc. pp.

> >+  skin/classic/communicator/preferences.css                             (communicator/preferences.css)
> 
> This file missing from patch, and from modern (as neil noted)

preferences.xul was missing also, added them.
Attachment #283452 - Attachment is obsolete: true
Attachment #286063 - Flags: superreview?(neil)
Attachment #286063 - Flags: review?(bugspam.Callek)
(Assignee)

Comment 6

11 years ago
Created attachment 286064 [details] [diff] [review]
migration of main browser pref panel and main security panel, v1

First two panel migrations, as a demo.
Attachment #286064 - Flags: superreview?(neil)
Attachment #286064 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 7

11 years ago
Created attachment 286065 [details] [diff] [review]
same as above, without whitespace changes
(Assignee)

Comment 8

11 years ago
Hmpf, the changes to mozilla/suite/common/pref/preftree.xul belong into the migration patch, not into the architecture one. Just in case anyone wonders.

But both patches should be applied together anyway...

Comment 9

11 years ago
(In reply to comment #5)
>(In reply to comment #3)
>>This will reselect the last selected pane, right? Presumably we can preset
>>lastSelected so that it will default to navigator on a new profile?
>The last selected panel is only relevant, if the dialog does not get a panel
>argument on startup, but that is usually the case anyway.
Usually != always. Anyway, I couldn't get -preferences to default to a pane. The old dialog does at least load browser preferences even though it doesn't highlight the right tree item.

Comment 10

11 years ago
Comment on attachment 286063 [details] [diff] [review]
main pref window, global interim changes, v1

>-  var resizable;
Oh, and I'm still not convinced about the resizability.
Attachment #286063 - Flags: superreview?(neil) → superreview+

Updated

11 years ago
Attachment #286064 - Flags: superreview?(neil) → superreview+

Comment 11

11 years ago
Comment on attachment 286063 [details] [diff] [review]
main pref window, global interim changes, v1

>+          // list of scripts to load
>+          var scripts = aPane.getAttribute('script')
>+                             .replace(/\s+/g, ' ').split(' ');
>+          var count = scripts.length;
>+          if (!count)
>+            return;
I would have pointed this out before but I thought I was superreviewing the other patch :-\ split always returns an array of at least one item, that may be empty. You probably want something like this:
var scripts = aPane.getAttribute('script').match(/\S+/g);
if (!scripts)
  return;
var count = scripts.length;
Comment on attachment 286064 [details] [diff] [review]
migration of main browser pref panel and main security panel, v1

>Index: suite/common/pref/pref-navigator.xul
>+  <prefpane id="navigator_pane"
onpaneload="this.parentNode.initPane(this);">

Is there someway around needing to explicitly init the pane's? can't we overload "onepaneload" or something, or *somehow* get our event handler for it to fire before the default one?

I won't press us to block on that alone, but it just seems like overkill w/ this.
Attachment #286064 - Flags: review+
Comment on attachment 286063 [details] [diff] [review]
main pref window, global interim changes, v1

Looks good.

Nit: regarding overall design placed in c#12
Attachment #286063 - Flags: review?(bugspam.Callek) → review+
(Assignee)

Comment 14

11 years ago
(In reply to comment #9)
> Anyway, I couldn't get -preferences to default to a pane.
> The old dialog does at least load browser preferences even though it doesn't
> highlight the right tree item.

Ah, -preferences. I'll look into this, but why should it default to *browser* preferences? Or the other way round: why aren't browser preferences the first section then? (The dialog as in this patch should default to the first panel.)


(In reply to comment #10)
> Oh, and I'm still not convinced about the resizability.

Why? I never understood why it wasn't...
The resizability mustn't be an excuse for overstuffed panels, of course...


(In reply to comment #12)
> onpaneload="this.parentNode.initPane(this);">
> 
> Is there someway around needing to explicitly init the pane's? can't we
> overload "onepaneload" or something, or *somehow* get our event handler for it
> to fire before the default one?

I'm not particularly happy with that either. I tried to derive the <prefpane> binding and specify a handler there, but it didn't quite work. I'm not quite sure if this was before or after I solved some loadOverlay issues, though, so I'll have a look at that again.
(Assignee)

Comment 15

11 years ago
Created attachment 286197 [details] [diff] [review]
main pref window, global interim changes, v2 
[checked in]

- Introduced a news <prefpane> derivation with just a paneload event handler which does our subscript loading incl. the eventual call to Startup(), so prefpanes don't need to care for that. Any given onpaneload attribute will still work, of course.
- When starting the preferences with -preferences, the first panel is selected (but does not quite work because it's not migrated yet). I see no pane in selecting the browser panel here. Opening the preferences from the browser menu will open the right panel, of course.
Attachment #286063 - Attachment is obsolete: true
Attachment #286197 - Flags: superreview?(neil)
Attachment #286197 - Flags: review?(bugspam.Callek)
(Assignee)

Comment 16

11 years ago
Created attachment 286198 [details] [diff] [review]
migration of main browser pref panel and main security panel, v2 [checked in]

Updated to match architecture patch v2:
- dropped onpaneload attribute
- added missing diff for preftree.xul
Taking over reviews, since there are no fundamental changes here.
Attachment #286064 - Attachment is obsolete: true
Attachment #286065 - Attachment is obsolete: true
Attachment #286198 - Flags: superreview+
Attachment #286198 - Flags: review+
Attachment #286064 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 17

11 years ago
Errata to comment #15:
> - Introduced a news <prefpane> derivation

... a new <prefpane> ...

> I see no pane in

I see no point in ...
Comment on attachment 286198 [details] [diff] [review]
migration of main browser pref panel and main security panel, v2 [checked in]

>Index: suite/common/pref/pref-navigator.xul
>+  <prefpane id="navigator_pane"
>+            script="  zzzz
>+                    chrome://communicator/content/pref/pref-navigator.js">

zzzz? :-P

Also, after re-looking at this, why is the logic in "OnDialogAccept" needed?
Comment on attachment 286197 [details] [diff] [review]
main pref window, global interim changes, v2 
[checked in]

does our prefpane handler fire before the handler on <prefpane onpaneload=""> ?

If not, I think we _need_ to document that (or change some stuff around).

Second, as a followup, we might want to trim the (very) lengthy comment at the top of preferences.xml to be a MDC doc or something. :-)
Attachment #286197 - Flags: review?(bugspam.Callek) → review+

Updated

11 years ago
Attachment #286197 - Flags: superreview?(neil) → superreview+
(Assignee)

Comment 20

11 years ago
(In reply to comment #18)
> >+  <prefpane id="navigator_pane"
> >+            script="  zzzz
> >+                    chrome://communicator/content/pref/pref-navigator.js">
> 
> zzzz? :-P

It's been rather late then. :-[
We need some kind "do a visual interdiff between attached patch x and new-patch-to-attach". ;-|

> Also, after re-looking at this, why is the logic in "OnDialogAccept" needed?

We can have an arbitrary number of homepages in a homepage group. OnDialogAccept is needed to get rid of any unused preferences - they wouldn't actually do harm, but cost disk space. (Imagine having your homepage set to a group of 100 tabs, then changing it to just one - .count would say 1, only 1 homepage will be seen by the dialogue, but 99 will still be dangling in prefs.js...)

(In reply to comment #19)
> does our prefpane handler fire before the handler on <prefpane onpaneload=""> ?

paneload events descend from the top, hit our prefwindow.paneload handler, descend further down, hit our prefpane.paneload, hit toolkit's prefpane.paneload, finally evaluate any given onpaneload attribute and then bubble up again.
(See general Mozilla event handling and XBL event handling in MDC, plus toolkit's prefwindow._fireEvent method.)

> Second, as a followup, we might want to trim the (very) lengthy comment at the
> top of preferences.xml to be a MDC doc or something. :-)

Supposing you mean prefwindow.xml: I don't think so. Websites can get lost. ;-)
But it would be more visible in DevMo or the wiki, true.

Comment 21

11 years ago
(In reply to comment #20)
>(In reply to comment #19)
>>does our prefpane handler fire before the handler on <prefpane onpaneload=""> ?
>paneload events descend from the top, hit our prefwindow.paneload handler,
>descend further down, hit our prefpane.paneload, hit toolkit's
>prefpane.paneload, finally evaluate any given onpaneload attribute and then
>bubble up again.
That's not quite true. All of the event listeners we use are bubbling event listeners. I'm not sure whether our or toolkit's paneload event handler fires first, but our prefwindow's handler fires third and onpaneload fires last.
(Assignee)

Comment 22

11 years ago
(In reply to comment #21)
> >paneload events descend from the top, hit our prefwindow.paneload handler,
> >descend further down, hit our prefpane.paneload, hit toolkit's
> >prefpane.paneload, finally evaluate any given onpaneload attribute and then
> >bubble up again.
> That's not quite true. All of the event listeners we use are bubbling event
> listeners.

Yeah, true. Actually, I even did make a testrun last night and still wrote the nonsense above... :-[

> I'm not sure whether our or toolkit's paneload event handler fires
> first, but our prefwindow's handler fires third and onpaneload fires last.

If an XBL derivation and its ancestor both handle an event, the derived handler gets called first. See <http://developer.mozilla.org/en/docs/XBL:XBL_1.0_Reference:Event_Handlers>.

_fireEvent then calls the onpaneload after the event processing.
>>If an XBL derivation and its ancestor both handle an event, the derived handler
gets called first.<<

ok, good; thats (part of) the bit I wasn't sure about. [and the onpaneload attrib, but I suspected that part was right]
(Assignee)

Comment 24

11 years ago
Debug dump for opening preferences from browser (some warnings cut out):


########## sm.prefpane.paneload navigator_pane ##########
...
WARNING: empty langgroup: file /home/kd/projekte/mozilla/mozilla.org/src/trunk/mozilla/gfx/thebes/src/gfxFont.cpp, line 871
########## tk.prefpane.paneload navigator_pane ##########
########## sm.prefwindow.paneload navigator_pane ##########
########## onpaneload navigator_pane ##########
(Assignee)

Updated

11 years ago
Attachment #286197 - Attachment description: main pref window, global interim changes, v2 → main pref window, global interim changes, v2 [checked in]
(Assignee)

Updated

11 years ago
Attachment #286198 - Attachment description: migration of main browser pref panel and main security panel, v2 → migration of main browser pref panel and main security panel, v2 [checked in]
(Assignee)

Comment 25

11 years ago
I checked in the architecture patch and the first two "demonstration" panel migrations on trunk.
Any further panel migration can work independently of each other now, but we need every helping hand - it's like 50 panels we need to migrate!

So, if you have the time to help out, please visit the wiki page <http://wiki.mozilla.org/SeaMonkey:Toolkit_Transition:PrefwindowPanes> where we coordinate the efforts and where I will put together some notes on How To Migrate A Pref Panel. ;-)

Patches should be attached here, thus leaving the bug open for now.
Keywords: helpwanted
(Assignee)

Comment 26

11 years ago
Bah, I checked in the zzzz from comment #18 - and thus had to remove it; got r=Kairo over IRC for that.

Comment 27

11 years ago
Note: when migrating mouse wheel preferences, don't forget to add full zoom.

Comment 28

11 years ago
Came looking for a "Mac Preferences are broken" bug, and found this one. The comments suggest this is a work in progress. I'll just note that, in 2007110701, Pref panes aren't painting properly. Or, at all, in come cases. I haven't tested extensively, but I'll be happy to take thorough notes, if someone would find them valuable.

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O 10.4; en-US; rv:1.9a9pre) Gecko/2007110701 SeaMonkey/2.0a1pre

Comment 29

11 years ago
David:
The problems are known, only 2 preferences panes are already migrated and working properly, the rest still needs to be done and therefore doesn't work/render correctly.

Comment 30

11 years ago
(In reply to comment #28)
> Came looking for a "Mac Preferences are broken" bug, and found this one. The
> comments suggest this is a work in progress.

I bet you haven't seen the new menuitem at the bottom of the "Edit" menu :-)

Comment 31

11 years ago
(In reply to comment #30)
> (In reply to comment #28)
> > Came looking for a "Mac Preferences are broken" bug, and found this one. The
> > comments suggest this is a work in progress.
> 
> I bet you haven't seen the new menuitem at the bottom of the "Edit" menu :-)
> 

Okay, Stefan, I'll play the straight man: "No, I didn't! Which new item on the bottom of the "Edit" menu?"

;-)

(In reply to comment #31)
[...]
> Okay, Stefan, I'll play the straight man: "No, I didn't! Which new item on the
> bottom of the "Edit" menu?"
> 
> ;-)
> 

Edit -> (Legacy Prefwindow)

That's where you find all the pref panes which haven't yet been migrated to use the new toolkit. The two (Migrated) items should be interpreted as meaning "Navigator" (or "Browser") and "Privacy & Security" respectively. They still have non-migrated subpanes.
(Assignee)

Comment 33

11 years ago
Created attachment 288235 [details] [diff] [review]
deconfuse pref dialogs

We've had quite some confusion among nightly users about what happened to the pref dialog, because the new dialog is only reachable from the browser and contains some not yet working panels. 
This patch adds some temporary explanation panel to the new panel, hides the unmigrated ones and reincludes the original labels to the "(migrated)" entries.
Attachment #288235 - Flags: superreview?(neil)
Attachment #288235 - Flags: review?(neil)

Comment 34

11 years ago
Comment on attachment 288235 [details] [diff] [review]
deconfuse pref dialogs

Some spelling nits:
>+      The SeaMonkey preferences are rewritten to use the same backend code as
are being rewritten
>+      other applications using the Mozilla XUL toolkit, eg. Firefox or
e.g.
>+      Thunderbird. We have like 50 panels to migrate, which we'll do in small
We have over 40 panels to migrate,
>+      chunks to guarentee we don't break anything. During this transition,
to guarantee
>+      this dialog here will only contain those panels which have already been
>+      ported to new format. You can reach the old dialog via the menuitem
migrated. You
>+      Edit→(Legacy Prefwindow...) in a browser window or via the button below.
or by clicking the button below.

>+    <description>
>+      This panel will go away once all panels have been migrated.
>+      See <a xmlns="http://www.w3.org/1999/xhtml"
>+      href="https://bugzilla.mozilla.org/show_bug.cgi?id=394522"
>+      style="text-decoration: underline;" target="_blank">Bugzilla bug 394522</a>
>+      for details and progress.
>+    </description>
Ideally you would use a <label class="text-link"> here. Maybe if you add the "This panel will go away" text to the top description, and just have a "More information..." link here?
Attachment #288235 - Flags: superreview?(neil)
Attachment #288235 - Flags: superreview+
Attachment #288235 - Flags: review?(neil)
Attachment #288235 - Flags: review+
(Assignee)

Comment 35

11 years ago
Created attachment 288865 [details] [diff] [review]
deconfuse pref dialogs, v2 [checked in]

Check-in version of attachment 288235 [details] [diff] [review], incorporating Neil's comments.
Landed on trunk.
Attachment #288235 - Attachment is obsolete: true
Attachment #288865 - Flags: superreview+
Attachment #288865 - Flags: review+
(Assignee)

Comment 36

11 years ago
Created attachment 288900 [details] [diff] [review]
allow mixing static and dynamic panels

The introduction of the static info panel dragged a flaw in the initial panel selection out of the dark: on startup, toolkit only sees the static panels and selects one of them instead of a requested dynamic one...

(Under Windows, the OS integration overlay is broken, which results in an incorrectly selected treeitem. I'll fix that in a separate patch later.)
Attachment #288900 - Flags: superreview?(neil)
Attachment #288900 - Flags: review?(neil)
Comment on attachment 288900 [details] [diff] [review]
allow mixing static and dynamic panels

>+              this._initialized = false;

Why are we setting this false.  I can't see a need for us to.
(Assignee)

Comment 38

11 years ago
Setting this._initialized to false is the crucial fix of this patch!
Without it, when passing a specific pane id (eg. navigator_pane), toolkit's _selectPane code will resize the prefwindow to height 0 here: <http://mxr.mozilla.org/seamonkey/source/toolkit/content/widgets/preferences.xml#765>.
_initialized is only used in _selectPane and will (for us) effectively just call window.sizeToContent(); if needed.
(Assignee)

Comment 39

11 years ago
To be "more" precise:
If we have both static panels and autogenerated panels, toolkit will initialize and select a static panel before our prefwindow constructor gets called. We then autogenerate panels like navigator_pane and try to show them, but their height is still 0, so we end up with a very tiny dialog...

Comment 40

11 years ago
Comment on attachment 288900 [details] [diff] [review]
allow mixing static and dynamic panels

>-          if (!this.currentPane)
>+          if (selectPane)
I don't like this. I think it would be better if you calculated the pane you wanted (based on arguments etc.) and see if it's already current or not.
Attachment #288900 - Flags: superreview?(neil)
Attachment #288900 - Flags: superreview-
Attachment #288900 - Flags: review?(neil)
(Assignee)

Comment 41

11 years ago
Created attachment 289083 [details] [diff] [review]
allow mixing static and dynamic panels, v2 [checked in]

Like this?
(Most changes are whitespace changes due to the removed nesting level.)
Attachment #288900 - Attachment is obsolete: true
Attachment #289083 - Flags: superreview?(neil)
Attachment #289083 - Flags: review?(neil)

Updated

11 years ago
Attachment #289083 - Flags: superreview?(neil)
Attachment #289083 - Flags: superreview+
Attachment #289083 - Flags: review?(neil)
Attachment #289083 - Flags: review+
(Assignee)

Updated

11 years ago
Duplicate of this bug: 404108
(Assignee)

Comment 43

11 years ago
Comment on attachment 289083 [details] [diff] [review]
allow mixing static and dynamic panels, v2 [checked in]

Landed on trunk.
Attachment #289083 - Attachment description: allow mixing static and dynamic panels, v2 → allow mixing static and dynamic panels, v2 [checked in]
(Assignee)

Comment 44

11 years ago
Created attachment 289186 [details] [diff] [review]
fix browser panel issues, v1

This patch fixes various issues with the browser panel:
- repair default browser settings under Windows by moving the relevant parts from platformPrefOverlay into #ifdef sections in pref-navigator.xul/js and preferences.xul. (Left the remaining treeitem in there, so that the legacy dialog won't break; it can be deleted as soon as we migrate pref-winhooks.)
- make browser panel aware of instant-apply
Attachment #289186 - Flags: superreview?(neil)
Attachment #289186 - Flags: review?(bugzilla)

Comment 45

11 years ago
Comment on attachment 289186 [details] [diff] [review]
fix browser panel issues, v1

Just looking at the shell service bits:

>+// Windows integration
As the "SetAsDefault" behaviour is due to be implemented by the shell service, which could eventually exist on several platforms, I'd like the code to use detection rather than #ifdef, although obviously for the moment it will detect the windows hooks. Also I'd like the code to be platform neutral e.g.

>+const kWinIntDeckNotDefault = 0;
>+const kWinIntDeckDefault    = 1;
>+const kWinIntDeckPending    = 2;
kShellDeckXXX

>   // register our OK handler for the capturing(!) phase
>   window.addEventListener("dialogaccept", this.OnDialogAccept, true);
Only do this when the set default button is clicked, and we're not using instant apply. Then you can use ApplySetAsDefaultBrowser as the listener.

>+<!DOCTYPE overlay [
>+  <!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd"> %brandDTD;
>+  <!ENTITY % dtd1 SYSTEM "chrome://communicator/locale/pref/pref-navigator.dtd"> %dtd1;
>+  <!ENTITY % dtd2 SYSTEM "chrome://communicator-platform/locale/pref/platformPrefOverlay.dtd"> %dtd2;
>+]>
You'll need to copy the entities from win/platformPrefOverlay.dtd (and remove all the platform overlay files, although you don't have to show that).

>+          <vbox flex="1">
Nit: don't need flex on deck children

>+            <hbox>
>+              <spacer flex="1"/>
>+              <button label="&defaultBrowserButton.label;"
>+                      oncommand="SetAsDefaultBrowser()"/>
>+              <spacer flex="1"/>
>+            </hbox>
Nit: could use <hbox pack="center">. Or maybe forget the hbox and use <vbox align="center">. Or maybe put the button outside the deck.
Note: whichever way, it occurs to me that you're switching a focused button with a disabled button. Ideally you should therefore focus something else.

>+            <description pack="start">&defaultPendingText;</description>
pack="start" is the default. (Does pack actually work on a description?)

>+#ifdef XP_WIN
>+          <!- - Windows integration is (obviously) only applicable on Windows - ->
Don't bother with the winhooks panel.

Comment 46

11 years ago
Comment on attachment 289186 [details] [diff] [review]
fix browser panel issues, v1

It seems to me that if you had a group, then change the input field, then click cancel, that you'll still lose the other pages?
Attachment #289186 - Flags: superreview?(neil) → superreview-
(Assignee)

Updated

11 years ago
Depends on: 143065

Comment 47

11 years ago
Created attachment 290006 [details] [diff] [review]
Mouse wheel preferences

Also adds radiobuttons for the full zoom feature.
Attachment #290006 - Flags: review?

Comment 48

11 years ago
Created attachment 290008 [details] [diff] [review]
Mouse wheel preferences (-w for review)

Also supports the new full zoom prefrerence.
Attachment #290006 - Attachment is obsolete: true
Attachment #290008 - Flags: review?(bugzilla)
Attachment #290006 - Flags: review?

Comment 49

11 years ago
Created attachment 290010 [details] [diff] [review]
Mouse wheel preferences
Comment on attachment 290008 [details] [diff] [review]
Mouse wheel preferences (-w for review)

Looks good r=me (without the password manager change in the full patch).
Attachment #290008 - Flags: review?(bugzilla) → review+
(Assignee)

Comment 51

11 years ago
Comment on attachment 290010 [details] [diff] [review]
Mouse wheel preferences

>Index: suite/common/pref/pref-mousewheel.js
>===================================================================
>+  for (var i = 0; i < checkboxes.length; i++) {
>+    var cb = document.getElementById(checkboxes[i]);
>+    var pref = document.getElementById(cb.getAttribute("preference"));
>+    if (pref.value)
>+      document.getElementById(fields[i]).disabled = true;

You shouldn't need to ask the <preference> element, just make sure its ctor always sees the <checkbox> element: put the <preferences> block at the end of the <prefpane>!
(Assignee)

Comment 52

11 years ago
JFTR: Toolkit <preference>s try to initialize observing elements, but toolkit <prefpane>s' paneload listener also does this. But our prefpane.paneload handler is executed before the toolkit one, so Startup() sees no initialized elements if they are defined in XUL after the observed <preference>.

Comment 53

11 years ago
Created attachment 290088 [details] [diff] [review]
Move prefpane event handler

This moves our paneload handler from the prefpane element to the prefwindow element so that it can fire Startup after the prefpane has done its stuff.

The other change is that we can now read the checkbox directly of course.
Attachment #290088 - Flags: review?(mnyromyr)

Comment 54

11 years ago
Created attachment 290114 [details] [diff] [review]
Mouse wheel preferences, v2

This makes the enabling work in instant apply mode.
Attachment #290008 - Attachment is obsolete: true
Attachment #290010 - Attachment is obsolete: true
Attachment #290114 - Flags: review?(bugzilla)

Comment 55

11 years ago
Created attachment 290595 [details] [diff] [review]
Use fewer accesskeys

Given that bug 143065 is fixed, we can be consistent with our access keys.
Attachment #290114 - Attachment is obsolete: true
Attachment #290595 - Flags: review?(bugzilla)
Attachment #290114 - Flags: review?(bugzilla)
Comment on attachment 289186 [details] [diff] [review]
fix browser panel issues, v1

I've taken a brief look at this, but my main comment (like Neil said) is that we should auto detect what we've got for our platforms.
Attachment #289186 - Flags: review?(bugzilla) → review-
Comment on attachment 290595 [details] [diff] [review]
Use fewer accesskeys

r=me. Just one comment though:

+                    <textbox id="mousewheelWithNoKeyNumlines" size="3" 
+                             checkbox="mousewheel.withnokey.sysnumlines"
+                             preference="mousewheel.withnokey.numlines"/>

I realise this just updating what is there, but could we do a follow-up patch to change this (and the other boxes) to be a number textbox?
Attachment #290595 - Flags: review?(bugzilla) → review+

Comment 58

11 years ago
Mouse wheel preferences checked in.
Comment on attachment 290595 [details] [diff] [review]
Use fewer accesskeys

Please change the labels in the locales files when the content change. So the localizers will notice the change when there tinderbox  turn orange
(Assignee)

Comment 60

11 years ago
> Please change the labels in the locales files when the content change. So the
> localizers will notice the change when there tinderbox  turn orange

Which ones do you mean in particular here? 
(It makes no sense doing extensive renaming for stuff which may just live for only a limited time.)

Comment 61

11 years ago
(In reply to comment #59)
>(From update of attachment 290595 [details] [diff] [review])
>Please change the labels in the locales files when the content change. So the
>localizers will notice the change when there tinderbox turn orange
In that patch, there was one major content change, and it was a new entity, and if that doesn't turn your tinderbox orange then I don't know what will.
(In reply to comment #61)
> (In reply to comment #59)
> >(From update of attachment 290595 [details] [diff] [review])
> >Please change the labels in the locales files when the content change. So the
> >localizers will notice the change when there tinderbox turn orange
> In that patch, there was one major content change, and it was a new entity, and
> if that doesn't turn your tinderbox orange then I don't know what will.

true :-)

but there was also a (really) small change:

-<!ENTITY scrollLines.label               " lines">
-<!ENTITY scrollChars.label               " characters">
+<!ENTITY scrollLines.label               "lines">
+<!ENTITY scrollChars.label               "characters">

it's nothing (i now) but i want to make sure that every developer is aware of the fact that they need to change the label if the change the content.

Comment 63

11 years ago
(In reply to comment #62)
> but there was also a (really) small change:

...which was not semantic, just the removal of a space.

> it's nothing (i now) but i want to make sure that every developer is aware of
> the fact that they need to change the label if the change the content.

Only if it's a semantic change, i.e. changes the meaning or use of a string in a way that localizers need to do an according change.

That was probably not the case here.
Created attachment 291514 [details] [diff] [review]
Change the tree children id to help with migrating and sharing overlays between the two versions [checked in]

Neil said in bug 404263 comment 8 that we're happy to change the id for the main treechildren to aid with updating overlays - i.e. we can do two sets of treechildren in the same file - one for the old style window, one for the new (extensions can also use this to work with both versions as well).

I thought it best to separate it out to this bug, so here is the change. I decided adding "prefs" would be more descriptive than what it is now.
Attachment #291514 - Flags: review?(neil)
Attachment #291514 - Flags: review?(mnyromyr)

Updated

11 years ago
Attachment #291514 - Flags: review?(neil) → review+
(Assignee)

Updated

11 years ago
Attachment #291514 - Flags: review?(mnyromyr) → review+
Attachment #291514 - Attachment description: Change the tree children id to help with migrating and sharing overlays between the two versions → Change the tree children id to help with migrating and sharing overlays between the two versions [checked in]

Updated

11 years ago
Alias: prefpane_migration
(Assignee)

Comment 65

11 years ago
Created attachment 295170 [details] [diff] [review]
fix browser panel issues, v2

This update has quite some changes:
- fixed Mark's comment #56 and Neil's comment #45, especially:
  - (preparations for) autodetecting default browser settings,
  - rewrote default browser XUL
  - better instantApply handling
  - killed platformPrefOverlay.xul & Co. (that's the main noise here)
  - left out WinHooks panel
- fixed Neil's comment #46:
  - worked around newly found toolkit bug 410562 *sigh*
  - rewrote homepage group handling
- killed the accesskey workaround now that accesskeys-in-decks issues are fixed
Please test especially the Windows default browser settings, since I don't have a Windows dev environment (I squeezed it into a normal end user build to test, though, which seemed to work).
I left out the actual removal of the platformPrefOverlay.xul and .css files.
Attachment #289186 - Attachment is obsolete: true
Attachment #295170 - Flags: superreview?(neil)
Attachment #295170 - Flags: review?(iann_bugzilla)
(Assignee)

Updated

11 years ago
Attachment #295170 - Flags: review?(iann_bugzilla) → review?(bugzilla)

Comment 66

11 years ago
Comment on attachment 295170 [details] [diff] [review]
fix browser panel issues, v2

>Index: suite/locales/en-US/chrome/common/pref/pref.dtd
>===================================================================
>+<!ENTITY  prefWindowWin.size          "width: 52em; height: 41em;">
>+<!ENTITY  prefWindowMac.size          "width: 58em; height: 41em;">

Please,
1) leave this spread to three variants: Win/Mac/unix(GNOME), in the same way as it's done in http://mxr.mozilla.org/mozilla/source/browser/components/preferences/preferences.xul#85
2) As you're already touching this, please convert it to use ch units for width, see bug 380378

Comment 67

11 years ago
(In reply to comment #66)
>Please,
>1) leave this spread to three variants: Win/Mac/unix(GNOME), in the same way as
>it's done in
>http://mxr.mozilla.org/mozilla/source/browser/components/preferences/preferences.xul#85
>2) As you're already touching this, please convert it to use ch units for
>width, see bug 380378
Actually once you're using ch units you shouldn't need separate variants.

Comment 68

11 years ago
(In reply to comment #67)
> Actually once you're using ch units you shouldn't need separate variants.

Theoretically, that's true, but I'm not sure how it works out in practice, as prefs is a very special case of dialog, and I'd vote for keeping it as flexible as possible, esp. as long as we don't know if the same ch width actually does work fine in all languages on all platforms equally.

Comment 69

11 years ago
(In reply to comment #65)
>   - killed platformPrefOverlay.xul & Co. (that's the main noise here)
Sure, kill platformPerfOverlay.xul, but no need to kill the .dtd is there?
(Assignee)

Comment 70

11 years ago
(In reply to comment #66)
> 1) leave this spread to three variants: Win/Mac/unix(GNOME)

I didn't do this since in other places we also just differ between the different versions actually _needed_, not those theoretically possible (eg. <http://mxr.mozilla.org/seamonkey/source/toolkit/content/widgets/preferences.xml#512 > and following lines, which we copied (sanitized)). I don't mind which version we use, but it should be consistent.

> 2) As you're already touching this, please convert it to use ch units for
> width, see bug 380378

Okay, will do.

 
(In reply to comment #68)
> esp. as long as we don't know if the same ch width actually does
> work fine in all languages on all platforms equally.

I'm having trouble with the em dialog width under Linux/KDE/Raleigh anyway, but I can test at least with Linux vs. Mac.


(In reply to comment #69)
> >   - killed platformPrefOverlay.xul & Co. (that's the main noise here)
> Sure, kill platformPerfOverlay.xul, but no need to kill the .dtd is there?

What's the point of having these .dtd around if noone would use them? After moving out the default browser stuff to pref-navigator.xul and the prefWindow sizes to pref.dtd, the content is only used by pref-tabs.xul, so I don't see any need for keeping the platformPrefOverlay.dtd files...

Given that the ch issue is rather minor, I'd like to wait for other review comments before updating the patch (yeah, I know I forgot to remove an already commented out style hack (used in Raleigh since it doesn't draw groupbox outlines) ;-) ).

Comment 71

11 years ago
(In reply to comment #70)
> (In reply to comment #66)
> > 1) leave this spread to three variants: Win/Mac/unix(GNOME)
> 
> I didn't do this since in other places we also just differ between the
> different versions actually _needed_, not those theoretically possible

Sure, but we usually (from my experience) run into locales that need the pref window differently sized on Win/Linux (even Firefox came to need this) - I didn't debate the rest, though I don't particularly like calling things "Win" in key names when they get used for anything but Mac.

Comment 72

11 years ago
(In reply to comment #70)
>(In reply to comment #69)
> > >   - killed platformPrefOverlay.xul & Co. (that's the main noise here)
> > Sure, kill platformPerfOverlay.xul, but no need to kill the .dtd is there?
> What's the point of having these .dtd around if noone would use them? After
> moving out the default browser stuff to pref-navigator.xul and the prefWindow
> sizes to pref.dtd, the content is only used by pref-tabs.xul, so I don't see
> any need for keeping the platformPrefOverlay.dtd files...
Ah, I didn't realise that mousewheel prefs uses platformCommunicatorOverlay.dtd - would you mind moving tab browsing pref strings there too?

Comment 73

11 years ago
Having two pref windows is counted as "broken functionality" per  http://home.kairo.at/blog/2008-01/seamonkey_2_alpha_criteria as discussed in the Council - therefore this bug blocks Alpha.

Updated

11 years ago
Flags: blocking-seamonkey2.0a1+
(Assignee)

Comment 74

11 years ago
(In reply to comment #72)
> Ah, I didn't realise that mousewheel prefs uses platformCommunicatorOverlay.dtd

?
But they don't?!

Comment 75

11 years ago
(In reply to comment #74)
>(In reply to comment #72)
>>Ah, I didn't realise that mousewheel prefs uses platformCommunicatorOverlay.dtd
>?
>But they don't?!
suite/common/pref/pref-mousewheel.xul, line 139:
        <tab label="&usingWheelAndAlt.label;"/>
suite/locales/en-US/chrome/common/pref/pref-mousewheel.dtd, line 7:
<!ENTITY usingWheelAndAlt.label          "&altKey.label;">
suite/locales/en-US/chrome/common/mac/platformCommunicatorOverlay.dtd, line 59:
<!ENTITY altKey.label                    "Option">
(Assignee)

Comment 76

11 years ago
Sorry, I got confused by platformCommunicatorOverlay.dtd vs. platformPrefOverlay.dtd. 
So you want me to put the urlbar.* and middleclick.* entities into platformCommunicatorOverlay.dtd, thus avoiding the #ifdef in pref-tabs.dtd?
(Assignee)

Comment 77

11 years ago
(In reply to comment #76)
> So you want me to put the urlbar.* and middleclick.* entities into
> platformCommunicatorOverlay.dtd, thus avoiding the #ifdef in pref-tabs.dtd?

I forgot to mention that I don't think that this would be a good idea, because these strings clearly don't have any SM-wide use...

Comment 78

11 years ago
Comment on attachment 295170 [details] [diff] [review]
fix browser panel issues, v2

By code inspection, there seem to be some issues with this patch:
* currentGroupButton isn't disabled when the tabbrowser only has one tab
* SetHomePageToCurrentGroup doesn't work because it tries to stuff the whole home page group into a single-line textbox
* RestoreHomePageGroup restores to the previous home page group, but the old button restored to the default home page
I'd also prefer it if you used browserWindow.getBrowser()

(In reply to comment #76)
> So you want me to put the urlbar.* and middleclick.* entities into
> platformCommunicatorOverlay.dtd, thus avoiding the #ifdef in pref-tabs.dtd?
I'd prefer them in a platform overlay, yes.
(Assignee)

Comment 79

11 years ago
(In reply to comment #78)
> * currentGroupButton isn't disabled when the tabbrowser only has one tab

It's a group - just with one member. But, okay...

> * SetHomePageToCurrentGroup doesn't work because it tries to stuff the whole
> home page group into a single-line textbox

It's a multiline textbox now, so users can actually see what their homepage group is.

> * RestoreHomePageGroup restores to the previous home page group,

Yes.

> but the old button restored to the default home page

No, it didn't. It just restored the first homepage found on dialog startup (which already made me wonder when I first migrated that panel).

> I'd also prefer it if you used browserWindow.getBrowser()

Instead of what?

> > So you want me to put the urlbar.* and middleclick.* entities into
> > platformCommunicatorOverlay.dtd, thus avoiding the #ifdef in pref-tabs.dtd?
> I'd prefer them in a platform overlay, yes.

Then we probably shouldn't kill platformPrefOverlay.dtd in the first place (but platformPrefOverlay.xul still can go, since we don't have any different XUL stuff)?

Comment 80

11 years ago
(In reply to comment #79)
> It's a multiline textbox now
Ah! Sorry, I'd overlooked that.

> > but the old button restored to the default home page
> No, it didn't.
Yes it did, because it used the default pref.

> > I'd also prefer it if you used browserWindow.getBrowser()
> Instead of what?
document.getElementById("content")

> > I'd prefer them in a platform overlay, yes.
> Then we probably shouldn't kill platformPrefOverlay.dtd in the first place
New alternative: add an entity for accelKey.label = Cmd/Control in platformCommunicatorOverlay.dtd and reference that in pref-tabs.dtd

Updated

11 years ago
Depends on: 411028

Updated

11 years ago
Depends on: 411215

Updated

11 years ago
Depends on: 411226

Updated

11 years ago
Depends on: 411620

Comment 81

11 years ago
Comment on attachment 290088 [details] [diff] [review]
Move prefpane event handler

>-prefpane {
>-  -moz-binding: url("chrome://communicator/content/bindings/prefwindow.xml#prefpane");
>-}
It turns out I can't actually remove this because Modern needs it.
(Assignee)

Comment 82

11 years ago
Comment on attachment 290088 [details] [diff] [review]
Move prefpane event handler

The most severe issue with this is something else: currently, a 'paneload' event is dispatched to the <prefpane> element, first caught by SM's <prefpane> 'paneload' handler (where Startup is called, allowing dynamic creation of <preference> elements before they are used to init the UI), then by toolkit's <prefpane> 'paneload' handler (where UI elements get fed with their <preference> data), then by SM's <prefwindow>  'paneload' handler (syncing <tree> and <pane>), and finally UI element's 'onpaneload' attribute is evaluated. 
Dropping the <prefpane> 'paneload' handler would mean some event phase tinkering, which I think isn't exactly easier to comprehend. 
If you need to read a <preference> value via the UI element, put your function into the <prefpane>'s onpaneload handler (but it's probably better to just ask the <preference> directly in Startup()).
Attachment #290088 - Flags: review?(mnyromyr) → review-
(Assignee)

Comment 83

11 years ago
Created attachment 300479 [details] [diff] [review]
[wrong patch]

After some IRC discussion I decided to keep the platformPrefOverlay.dtd files while still dropping the platformPrefOverlay.xul ones - it's much cleaner this way. (The actual CVS removals are not shown in this patch.)

Fixed also Neil's nits about ch usage, button disabling, default homepage etc.
Attachment #295170 - Attachment is obsolete: true
Attachment #300479 - Flags: superreview?(neil)
Attachment #300479 - Flags: review?(bugspam.Callek)
Attachment #295170 - Flags: superreview?(neil)
Attachment #295170 - Flags: review?(bugzilla)
(Assignee)

Comment 84

11 years ago
Created attachment 300481 [details] [diff] [review]
fix browser panel issues, v3

Correct patch file this time; see comment #83 for details.
Attachment #300481 - Flags: superreview?(neil)
Attachment #300481 - Flags: review?(bugspam.Callek)
(Assignee)

Updated

11 years ago
Attachment #300479 - Attachment description: fix browser panel issues, v3 → [wrong patch]
Attachment #300479 - Attachment is obsolete: true
Attachment #300479 - Flags: superreview?(neil)
Attachment #300479 - Flags: review?(bugspam.Callek)

Comment 85

11 years ago
Comment on attachment 300479 [details] [diff] [review]
[wrong patch]

 <!-- LOCALIZATION NOTE : this is part of an inline-style attribute on the
      preference dialog's <window> node, which specifies the width and height
      in em units of the dialog. Localizers ONLY can increase these widths 
      if they are having difficulty getting panel content to fit. 1em = the 
      width of the letter 'm' in the selected font.
      XUL/FE DEVELOPERS: DO NOT MODIFY THIS VALUE. It represents the correct
      size of this window for en-US. -->
-<!ENTITY  prefWindow.size             "width: 58em; height: 41em;">
+<!ENTITY  prefWindow.title  "Preferences">
+<!ENTITY  prefWindow.size   "width: 80ch; height: 41em;">
+

68ch looks more reasonable on mac. Note that the localization comment is wrong now (no dialog, units are in ch and em).

Comment 86

11 years ago
sigh, wrong attachment - I ment to comment on attachment #300481 [details] [diff] [review], of course

Comment 87

11 years ago
Comment on attachment 300481 [details] [diff] [review]
fix browser panel issues, v3

Btw, I must say that I question the usage of the multiline textbox for the homepage prefs. I find it hard to see the benefits of a scrollable textfield with all your URI's (if you have a groupmark). It looks more to me like a power-user pref. Normal user will wonder why the textbox is so big. And if you did remembered a bunch of pages (or keywords) that you wanted to bookmark, wouldn't you rather just type/paste them in the url bar instead?
some notes for now...

This does indeed fix the Debug Panel (appearing blank), will probably correct MReimers overlay issue (so he should be able to remove his dirty hack; see newsgroup). YAY!

I'm not fond of the new layout for the homepage pref, I do feel that is an (highly?) advanced usage of it, that we probably don't need to expose here. [Besides it makes the prefwindow too short for it on windows].

for a spinoff bug (probably):
Error: _elementIDs is not defined
Source File: chrome://messenger-mapi/content/pref-mailnewsOverlay.xul
Line: 50

That is why the mailnews pane does not want to show for us now...

I'll give a proper review later...
(Assignee)

Comment 89

11 years ago
> Btw, I must say that I question the usage of the multiline textbox for the
> homepage prefs.

I'll try to explain my reasoning:
- If you want to set a homepage, it's usually (AFAICT) the current page or group you're viewing. It seems rather unlikely that someone went to Preferences and started typing, musing "let's see what urlbar history thinks I can have as a homepage". 
  => autocomplete is nice, but not essential
- If you have set a tab group as your homepage, there's no way currently to review that list, except by opening it (and thus causing a lot of poor electrons to wiggle around without real need). 
  => we need a way to display the group of homepages in the prefs
- (Enhancement: It'd be nice if you could set multiple keyword searches as your homepages.)

My current implementation tries to balance these points.


I want my homeIt seems extremely unlikely to say 



- If you want to change you homepage, you have to go to Preferences. There's no way to see your current homepage(s) before 




 I find it hard to see the benefits of a scrollable textfield
> with all your URI's (if you have a groupmark). It looks more to me like a
> power-user pref. Normal user will wonder why the textbox is so big. And if you
> did remembered a bunch of pages (or keywords) that you wanted to bookmark,
> wouldn't you rather just type/paste them in the url bar instead?
> 

(Assignee)

Comment 90

11 years ago
Uh, how I hate this mini textbox!
Ignore the last comment, read this one:

Comment #87:
> Btw, I must say that I question the usage of the multiline textbox for the
> homepage prefs.

I'll try to explain my reasoning:
- If you want to set a homepage, it's usually (AFAICT) the current page or
group you're viewing. It seems rather unlikely that someone went to Preferences
and started typing, musing "let's see what urlbar history thinks I can have as
a homepage". 
  => autocomplete is nice, but not essential
- If you have set a tab group as your homepage, there's no way currently to
review that list, except by opening it (and thus causing a lot of poor
electrons to wiggle around without real need). 
  => we need a way to display the group of homepages in the prefs
- (Enhancement: It'd be nice if you could set multiple keyword searches as your
homepages.)

My current implementation tries to balance these points.

Comment #88:
> [Besides it makes the prefwindow too short for it on windows].

Hm, shouldn't be, of course.
OTOH, when Ratty finishes his toolbar customization, we can get rid of the button prefs anyway...

Comment 91

11 years ago
(In reply to comment #90)
> Uh, how I hate this mini textbox!
> Ignore the last comment, read this one:
> 
> Comment #87:
> > Btw, I must say that I question the usage of the multiline textbox for the
> > homepage prefs.
> 
> I'll try to explain my reasoning:
> - If you want to set a homepage, it's usually (AFAICT) the current page or
> group you're viewing. It seems rather unlikely that someone went to Preferences
> and started typing, musing "let's see what urlbar history thinks I can have as
> a homepage". 
>   => autocomplete is nice, but not essential

Yeah, well - I guess I put more weight on "nice" than essential here ;-)

> - If you have set a tab group as your homepage, there's no way currently to
> review that list, except by opening it (and thus causing a lot of poor
> electrons to wiggle around without real need). 
>   => we need a way to display the group of homepages in the prefs

Yeah, but wouldn't the most useful way to review your homepage tabgroup be to to just hit the home button? Would it be better to look at a list of 10-15 URI's? Besides, most people that are interested in the homepage prefs would already have a browser window open, no?
> - (Enhancement: It'd be nice if you could set multiple keyword searches as your
> homepages.)

This is the most valid point of your reasoning, I think. Imo, the only argument against this would be that it's really a power-user feature.

Comment 92

11 years ago
Comment on attachment 300481 [details] [diff] [review]
fix browser panel issues, v3

>+<!DOCTYPE overlay [
>+  <!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd"> %brandDTD;
>+  <!ENTITY % dtd1 SYSTEM "chrome://communicator/locale/pref/pref-navigator.dtd"> %dtd1;
>+]>
Nit: I'd call this something like navigatorDTD rather than dtd1.

>+      <groupbox id="defaultBrowserGroup" flex="1" align="center" hidden="true">
Having seen what happens when your preferences window is too wide ;-) I see that this groupbox flexes depending on the current description... to reduce that effect I suggest increasing the flex here, say to 1000.

>+        <description id="defaultBrowserDesc"
>+                     desc0="&makeDefaultText;"
>+                     desc1="&alreadyDefaultText;"
>+                     desc2="&defaultPendingText;"/>
Hmm... the button also moves depending on the amount of text. Maybe flex here?

>+<!ENTITY  prefWindow.title  "Options">
What's this for? Fortunately the correct pref title is in pref.dtd; please remove this entity from the platform pref files.

>-<!ENTITY  prefWindow.size             "width: 52em; height: 41em;">
>+<!ENTITY  prefWindow.size   "width: 72ch; height: 41em;">
This is far too wide. 70ch would have been the absolute limit; by my calculations (which I might have written incorrectly or you may have misinterpreted) it should be 63 or 64, but obviously bump it up a little if the Mac needs it - I'd prefer all three sizes to be the same.
Attachment #300481 - Flags: superreview?(neil) → superreview+
(In reply to comment #91)
[...]
> Yeah, but wouldn't the most useful way to review your homepage tabgroup be to
> to just hit the home button? Would it be better to look at a list of 10-15
> URI's? Besides, most people that are interested in the homepage prefs would
> already have a browser window open, no?
[...]
In Suiterunner (Sm2) my homepage consists of over 30 tabs. AFAIK, the most useful way to review it (and without needless bandwidth consumption) currently consists of loading the about:config page, then typing "homepage" (without the quotes of course) in the Filter box. Yes, looking at a list of 30-some URIs, even with a need for scrolling, is better than waiting a sizable time for them to load, then walking the tabs all the way across the screen. Not to mention that I might have a non-default (non-"home") set of tabs open at the time, which I wouldn't want to lose.

IMHO it would be nice to be able to see the list (instead of "-- Home Page Group is Set --") in the "Browser" prefpane. Or maybe in an additional popup by clicking a button there.

Currently using "Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3pre) Gecko/2008013119 SeaMonkey/2.0a1pre".
(In reply to comment #91)
> [...] Imo, the only argument
> against this would be that it's really a power-user feature.
> 
So? Isn't SeaMonkey meant to be useful for everyone, even power users? ;-) Up to now, I thought they were more "at ease" with the Suite than with Fx&Tb (where the Preferences in particular, have a kind of "kindergarten" look & feel IYSWIM).
Comment on attachment 300481 [details] [diff] [review]
fix browser panel issues, v3

I'm still not ecstatic over the homepage changes, though a few people seem to think an alpha tryout is a good idea, so I'm not going to block on that.

Given Neil's nits, I'll add two more..

Please increase the height of the dialog to fit the toolbar customize stuff. [we can re-adjust whenever Ratty's customization patch lands]

>+function InitPlatformIntegration()
>+  // In future, we will ask the Shell Service about platform integration here,
>+  // for now, just check WinHooks.
>+  var showDefaultBrowserGroup = /Win/.test(navigator.platform);
>+  if (showDefaultBrowserGroup)
>   {

... can we loose an indent level here?
possibly by:

var showDefaultBrowserGroup = /Win/.test(navigator.platform);
document.getElementById("defaultBrowserGroup").hidden = !showDefaultBrowserGroup;
if (!showDefaultBrowserGroup)
  return;
Attachment #300481 - Flags: review?(bugspam.Callek) → review+
(Assignee)

Comment 96

11 years ago
Created attachment 302011 [details] [diff] [review]
fix browser panel issues, v4 [checked in]

Checked-in fix (plus CVS removal of the platformPrefOverlay.xul files), incorporating changes for the latest nits by Neil and Callek.
Since the dialog width values could not be computed to the same ch value on all platforms (and even differed between my and Neil's Windows), I left the width untouched; work on that will happen in bug 380378 (I think).
Attachment #300481 - Attachment is obsolete: true
Attachment #302011 - Flags: superreview+
Attachment #302011 - Flags: review+
(Assignee)

Updated

11 years ago
Depends on: 416548

Updated

11 years ago
Depends on: 417590

Comment 97

11 years ago
Bug 416274 is relevant to anyone porting proxy preferences.

Updated

11 years ago
Depends on: 419865

Updated

11 years ago
Blocks: 421092

Updated

11 years ago
No longer blocks: 399031
Depends on: 399031

Updated

11 years ago
Depends on: 421832
(Assignee)

Updated

10 years ago
Depends on: 427817

Updated

10 years ago
Depends on: 431061

Updated

10 years ago
Depends on: 435079

Updated

10 years ago
Depends on: 436709

Updated

10 years ago
Depends on: 436730

Updated

10 years ago
Depends on: 441403

Updated

10 years ago
Depends on: 444146

Updated

10 years ago
Depends on: 444157

Updated

10 years ago
Depends on: 444161

Updated

10 years ago
Depends on: 444167

Updated

10 years ago
Depends on: 444169

Updated

10 years ago
Depends on: 444170
Depends on: 444411

Updated

10 years ago
Depends on: 444582

Updated

10 years ago
Depends on: 445013

Updated

10 years ago
Depends on: 445014

Updated

10 years ago
Depends on: 445015

Updated

10 years ago
Depends on: 448105

Updated

10 years ago
Depends on: 448106

Updated

10 years ago
Depends on: 448107

Updated

10 years ago
Blocks: 453715

Updated

10 years ago
No longer blocks: 453715

Updated

10 years ago
Blocks: 436934

Updated

10 years ago
No longer depends on: 79676
(Assignee)

Comment 98

10 years ago
The code removal is/was blocked by this, not the other way around.
Marking this FIXED! 

Yay! \o/
Thanks to all who helped!
Blocks: 427817
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
No longer depends on: 427817
Keywords: helpwanted
Resolution: --- → FIXED
Target Milestone: seamonkey2.0beta → seamonkey2.0alpha
You need to log in before you can comment on or make changes to this bug.