The Firefox applications pref pane would make a fine addition to trunk SeaMonkey as the new helper app pane in the new prefwindow, I have a working first version of this in my tree already :)

This ports the Firefox applications pane to SeaMonkey. I locally have porting/copying WebContentConverter in the tree as well, but it's not needed for a first step of this, we are used to having only beginnings of feed support in our tree anyways. I did add the prefs already that will only come to full use with that additional port though (enabling feed handling by web handlers). I also removed obsolete styles from prefpanels.css in this patch.

Here are the icons to go into mozilla/suite/themes/classic/communicator/icons - I know I left out Modern for the moment, I will cover this once the rest of this patch looks good.

Comment on attachment 303370 [details] communicator/icons for default theme to go with this Also, not sure if Callek has a different icon upcoming for livemarks or if we should look into using one of our feed icons here.

To Robert Kaiser(Bug opener & Assigned To: person) : (Q1) Does this change mean Seamonkey also will loose "New Type" capability? (manual addition of mime-type and/or file extension.) (Q2) Does this change mean Seamonkey also will loose "Edit" capability? (manual correction of mime-type <=> file extension relation etc.) (in case of incorrect server side setting or unexpected setting.)

WADA: Yes and yes. Those are actually not really needed. New types are added either because a new plugin supports them or because we run across them on the web and pop up the default "ask" dialog for handling them. You can always change the handler application in the pref pane, MIME type to extension relations are taken from the OS, from what I know - a website has no possibility to tell us about such a relation anyways, it only tells us a MIME type, nothing else. Deleting from the list would be the same as setting to "always ask", so there's not much use for that as well.

(In reply to comment #5) > Deleting from the list would be the same as setting to "always ask" How can I delete? I guess that "delete" key because you say "delete from list" in your answer to my question, but not obvious, because no button, no menu, no context menu. > Those are actually not really needed. When Web Browser only, improper relation between mime-type/file extension is set only by incorrectly prepared site by mistake, or badly prepared site intensionally. However, when Tb, many spammers can easily set relation between mime-type/file extension in mimeTypes.rdf very badly, because current Tb has capability to set mimeTypes.rdf via dialog only as Fx does. When Seamonkey, situation is same as Tb, because mailer is consolidated. I think "Edit" feature is required for Seamonkey, in order to reduce unwanted bugs due to spams. (I don't care Tb case) I believe at least feature to see "file extension" for the mime-type is required when Seamonkey. ("File Extension" column can be a solution) Please note that current "Edit" feature can be used to check current mimeTypes.rdf settings.

missing header for unified diff at line 2621 of patch

WADA: 1) I'm not sure if you did read my comment in detail. 2) If you code that extension to the dialog, we can talk about it, though I still think it's unneeded. Neil: Umm, hand-editing diffs sucks, obviously ;-)

Comment on attachment 303368 [details] [diff] [review] add new applications pref pane as a port of the FF pane >-/* -*- Mode: Java; tab-width: 2; c-basic-offset: 2; -*- >- * >- * ***** BEGIN LICENSE BLOCK ***** >+# -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- >+# ***** BEGIN LICENSE BLOCK ***** Please don't change the formatting of the licence block. >+// XXX-SeaMonkey: once bug 389322 is fixed and we make this a normal pane script, Which 1.7 features does this use? I'd rather s/let/var/ to make this a normal pane. >+#ifndef XP_MACOSX (This is something to do with the way the menus work on the Mac Firefox.) >+var Cc = Components.classes; >+var Ci = Components.interfaces; >+var Cr = Components.results; I'd rather you used the full names, except for interfaces, where you could define shortcut contstants for each interface. >+const ICON_URL_APP = "chrome://communicator/skin/icons/application.png"; >+ >+// For CSS. Can be one of "ask", "save", "plugin" or "feed". If absent, the icon URL >+// was set by us to a custom handler icon and CSS should not try to override it. >+const APP_ICON_ATTR_NAME = "appHandlerIcon"; This comment is bogus; the default icon should be defined in CSS. >+ return Cc["@mozilla.org/network/io-service;1"]. >+ getService(Ci.nsIIOService). >+ newFileURI(aFile). >+ QueryInterface(Ci.nsIURL). >+ fileName; What's wrong with aFile.leafName? >+ _handlerSvc: Cc["@mozilla.org/uriloader/handler-service;1"]. >+ getService(Ci.nsIHandlerService), These should be global. >+ // Retrieve this as nsIPrefBranch and then immediately QI to nsIPrefBranch2 >+ // so both interfaces are available to callers. >+ _prefSvc: Cc["@mozilla.org/preferences-service;1"]. >+ getService(Ci.nsIPrefBranch). >+ QueryInterface(Ci.nsIPrefBranch2), The comment is bogus. Just .getService it as nsIPrefBranch2 in the first place. And I don't like the firefox trailing . style. >+ element: function(aID) { >+ return document.getElementById(aID); >+ }, This one is silly. Just inline it. >+ _type: null, >+ get type() { >+ return this._type; >+ }, type: null, >+ var possibleApps = this.possibleApplicationHandlers.enumerate(); >+ while (possibleApps.hasMoreElements()) { >+ if (possibleApps.getNext().equals(aNewHandler)) >+ return; >+ } indexOf >+ for (var i = 0; i < handlers.length; ++i) { >+ var handler = handlers.queryElementAt(i, Ci.nsIHandlerApp); >+ if (handler.equals(aHandler)) { >+ handlers.removeElementAt(i); >+ break; >+ } >+ } The docs for removeElementAt even *suggest* indexOf. Sigh... >+ if (this.wrappedHandlerInfo.hasDefaultHandler) >+ return Ci.nsIHandlerInfo.useSystemDefault; >+ else >+ return Ci.nsIHandlerInfo.saveToDisk; ?: >+ return this.wrappedHandlerInfo.primaryExtension Missing ; >+ if (types != "") >+ return types.split(","); >+ >+ return []; return types ?: >+ _possibleApplicationHandlers: null, >+ >+ get possibleApplicationHandlers() { possibleApplicationHandlers: { >+ __defaultApplicationHandler: undefined, >+ get _defaultApplicationHandler() { >+ if (typeof this.__defaultApplicationHandler != "undefined") if this.__defaultApplicationHandler !== undefined >+ get primaryExtension() { >+ return "xml"; >+ }, primaryExtension: "xml", >+ get smallIcon() { >+ return this._smallIcon; >+ }, >+ >+ get largeIcon() { >+ return this._largeIcon; > } Remove these, and the underscores from the overrides. (Might need to put __proto__ last?) >+ _mimeSvc : Cc["@mozilla.org/mime;1"]. >+ getService(Ci.nsIMIMEService), >+ >+ _helperAppSvc : Cc["@mozilla.org/uriloader/external-helper-app-service;1"]. >+ getService(Ci.nsIExternalHelperAppService), >+ >+ _handlerSvc : Cc["@mozilla.org/uriloader/handler-service;1"]. >+ getService(Ci.nsIHandlerService), >+ >+ _ioSvc : Cc["@mozilla.org/network/io-service;1"]. >+ getService(Ci.nsIIOService), More globals. >+ // Observe preferences that influence what we display so we can rebuild >+ // the view when they change. I wonder why they don't use <preference> elements for this... >+ // By doing this in a timeout, we let the preferences dialog resize itself Except it doesn't need to, does it? >+ // If this is the feed type, add a Live Bookmarks item. ??? >+ // On Windows, selecting an application to open another application >+ // would be meaningless so we special case executables. Debugger anyone ;-) >+ window.openDialog("chrome://global/content/appPicker.xul", null, >+ "chrome,modal,centerscreen,titlebar,dialog=yes", >+ params); Bah, why don't we have a real app picker on Windows ;-) >+ // Unfortunately we can't use the favicon service to get the favicon, Mainly because we don't have one. >+ if (/^https?/.test(uri.scheme)) >+ return uri.prePath + "/favicon.ico"; >+ >+ return ""; ? uri.resolve("/favicon.ico") : ""; >+ <keyset> >+ <key key="&focusSearch1.key;" modifiers="accel" oncommand="gApplicationsPane.focusFilterBox();"/> >+ <key key="&focusSearch2.key;" modifiers="accel" oncommand="gApplicationsPane.focusFilterBox();"/> >+ </keyset> Don't need these. >+ >+ <hbox align="center"> >+ <label accesskey="&filter.accesskey;" control="filter">&filter.label;</label> >+ <textbox id="filter" flex="1" oninput="gApplicationsPane.onFilterInput();" >+ onkeypress="gApplicationsPane.onFilterKeyPress(event);"/> This should be a timed textbox. >+ <richlistbox id="handlersView" orient="vertical" persist="lastSelectedType" >+ preference="pref.downloads.disable_button.edit_actions" >+ onselect="gApplicationsPane.onSelectionChanged();"> >+ <listheader equalsize="always" style="border: 0; padding: 0; -moz-appearance: none;"> >+ <treecol id="typeColumn" label="&typeColumn.label;" value="type" >+ accesskey="&typeColumn.accesskey;" persist="sortDirection" >+ flex="1" onclick="gApplicationsPane.sort(event);" >+ sortDirection="ascending"/> >+ <treecol id="actionColumn" label="&actionColumn2.label;" value="action" >+ accesskey="&actionColumn2.accesskey;" persist="sortDirection" >+ flex="1" onclick="gApplicationsPane.sort(event);"/> >+ </listheader> >+ </richlistbox> Sigh... <listbox flex="1"> <listhead> <listheader> You'll also need to change the XBL to use listcell instead of the inner hbox (the outer hbox won't be necessary in a listbox). Conveniently listcells already include icons and text. >+/* ::::: Fonts ::::: */ Can you avoid moving this? >+.typeIcon, >+.actionIcon { >+ -moz-margin-start: 3px; >+ -moz-margin-end: 3px; > } Pointless. Just use margin: 0px 3px; >+#handlersView richlistitem label { >+ -moz-margin-start: 1px; >+ margin-top: 2px; > } > >+#handlersView richlistitem { >+ min-height: 22px; >+} Where are all your child selectors? >+.actionsMenu .menulist-icon { >+ -moz-margin-end: 3px; >+} >+ >+.actionsMenu > menupopup > menuitem > .menu-iconic-left { >+ -moz-padding-start: 0px; >+ -moz-padding-end: 2px; >+} > >+.actionsMenu > menupopup > menuitem { >+ -moz-padding-start: 4px; > } Eww... what does this solve? >+/** >+ * Make the icons appear. >+ * Note: we display the icon box for every item whether or not it has an icon >+ * so the labels of all the items align vertically. >+ */ >+.actionsMenu > menupopup > menuitem > .menu-iconic-left { >+ display: -moz-box; >+ min-width: 16px; >+} This should already work, unless our themes are busted perhaps? >+ oncommand="gApplicationsPane.onSelectAction(event.originalTarget)"> This should be an event handler in the XUL. >+ gApplicationsPane.rebuildActionsMenu(); This should be called from a select handler in the XUL. >Index: mozilla/suite/common/pref/pref-applicationManager.xul I didn't see this dialog, so I won't try to review it. >+# ***** BEGIN LICENSE BLOCK ***** /* */ licences please. >+#ifdef XP_MACOSX As above. >+# in descriptionApplications, %S will be replaced by one of the 3 following strings I would just write out the three strings.

(In reply to comment #9) > >+// XXX-SeaMonkey: once bug 389322 is fixed and we make this a normal pane script, > Which 1.7 features does this use? I'd rather s/let/var/ to make this a normal > pane. Possibly only let, yes. I'll try that replacement and see about errors. > >+const ICON_URL_APP = "chrome://communicator/skin/icons/application.png"; > >+ > >+// For CSS. Can be one of "ask", "save", "plugin" or "feed". If absent, the icon URL > >+// was set by us to a custom handler icon and CSS should not try to override it. > >+const APP_ICON_ATTR_NAME = "appHandlerIcon"; > This comment is bogus; the default icon should be defined in CSS. Good point. APP_ICON_ATTR_NAME is still needed, I introduced an "app" value now for cases where we want the default app icon. > >+ var possibleApps = this.possibleApplicationHandlers.enumerate(); > >+ while (possibleApps.hasMoreElements()) { > >+ if (possibleApps.getNext().equals(aNewHandler)) > >+ return; > >+ } > indexOf > > >+ for (var i = 0; i < handlers.length; ++i) { > >+ var handler = handlers.queryElementAt(i, Ci.nsIHandlerApp); > >+ if (handler.equals(aHandler)) { > >+ handlers.removeElementAt(i); > >+ break; > >+ } > >+ } > The docs for removeElementAt even *suggest* indexOf. Sigh... This might be true if possibleApplicationHandlers would have a full nsIMutableArray implementation, but it only has a very partial one, see possibleApplicationHandlers() in FeedHandlerInfo in this patch. Looks like indexOf() is not implemented there. > >+ _possibleApplicationHandlers: null, > >+ > >+ get possibleApplicationHandlers() { > possibleApplicationHandlers: { I'm a bit at a loss how to rewrite that whole fuction for this, probably not possible in this case. > >+ get primaryExtension() { > >+ return "xml"; > >+ }, > primaryExtension: "xml", This has only a getter, it can't be set. > >+ get smallIcon() { > >+ return this._smallIcon; > >+ }, > >+ > >+ get largeIcon() { > >+ return this._largeIcon; > > } > Remove these, and the underscores from the overrides. > (Might need to put __proto__ last?) Maybe this is what you wanted to say with the __proto__ comment, but the HandlerInfoWrapper that this is derived from does only have a getter for smallIcon and largeIcon and no setter... > >+ // Observe preferences that influence what we display so we can rebuild > >+ // the view when they change. > I wonder why they don't use <preference> elements for this... No idea - and not sure if I'm able to rewrite that... > >+ // By doing this in a timeout, we let the preferences dialog resize itself > Except it doesn't need to, does it? Good question. > >+ // If this is the feed type, add a Live Bookmarks item. > ??? Sure, we don't support livemarks yet, but Callek still plans to work on this, so I didn't want to rip it out from here. Should I comment out adding this menu item here for now? > >+ // On Windows, selecting an application to open another application > >+ // would be meaningless so we special case executables. > Debugger anyone ;-) Should I remove this exception? > >+ window.openDialog("chrome://global/content/appPicker.xul", null, > >+ "chrome,modal,centerscreen,titlebar,dialog=yes", > >+ params); > Bah, why don't we have a real app picker on Windows ;-) I think it was originally even planned to write a nice one for all platforms but that ended up being too much work for 1.9 > >+ > >+ <hbox align="center"> > >+ <label accesskey="&filter.accesskey;" control="filter">&filter.label;</label> > >+ <textbox id="filter" flex="1" oninput="gApplicationsPane.onFilterInput();" > >+ onkeypress="gApplicationsPane.onFilterKeyPress(event);"/> > This should be a timed textbox. I don't understand what a "timed textbox" is :( > >+ <richlistbox id="handlersView" orient="vertical" persist="lastSelectedType" > >+ preference="pref.downloads.disable_button.edit_actions" > >+ onselect="gApplicationsPane.onSelectionChanged();"> > >+ <listheader equalsize="always" style="border: 0; padding: 0; -moz-appearance: none;"> > >+ <treecol id="typeColumn" label="&typeColumn.label;" value="type" > >+ accesskey="&typeColumn.accesskey;" persist="sortDirection" > >+ flex="1" onclick="gApplicationsPane.sort(event);" > >+ sortDirection="ascending"/> > >+ <treecol id="actionColumn" label="&actionColumn2.label;" value="action" > >+ accesskey="&actionColumn2.accesskey;" persist="sortDirection" > >+ flex="1" onclick="gApplicationsPane.sort(event);"/> > >+ </listheader> > >+ </richlistbox> > Sigh... > <listbox flex="1"> > <listhead> > <listheader> > You'll also need to change the XBL to use listcell instead of the inner hbox > (the outer hbox won't be necessary in a listbox). Conveniently listcells > already > include icons and text. http://developer.mozilla.org/en/docs/XUL:listitem says "If you wish to create a list with non-text content, you should instead use the richlistbox element." - and the selected row contains a menulist (see handler-selected XBL) which I'd count as "non-text content". Is that a wrong assumption? > >+/* ::::: Fonts ::::: */ > Can you avoid moving this? I did not really move this, but with the deletions above and below and the additions in that file, diff seems to think it should display this to you as if it was moved :( > >+#handlersView richlistitem { > Where are all your child selectors? Sorry, that was more a fast hack for my first try on doing this, as the FF CSS has just |richlistitem| there. > >+.actionsMenu .menulist-icon { > >+ -moz-margin-end: 3px; > >+} > >+ > >+.actionsMenu > menupopup > menuitem > .menu-iconic-left { > >+ -moz-padding-start: 0px; > >+ -moz-padding-end: 2px; > >+} > > > >+.actionsMenu > menupopup > menuitem { > >+ -moz-padding-start: 4px; > > } > Eww... what does this solve? Probably what the comment above all the applications entries does state? Note that I copied this from winstripe, so it might not even work 100% right on Mac and Linux yet (icons and texts in the list items still slightly jump on my Linux box here when changing selections) > >+/** > >+ * Make the icons appear. > >+ * Note: we display the icon box for every item whether or not it has an icon > >+ * so the labels of all the items align vertically. > >+ */ > >+.actionsMenu > menupopup > menuitem > .menu-iconic-left { > >+ display: -moz-box; > >+ min-width: 16px; > >+} > This should already work, unless our themes are busted perhaps? Maybe it should, but it doesn't. Just tested this on Linux. > >+ oncommand="gApplicationsPane.onSelectAction(event.originalTarget)"> > This should be an event handler in the XUL. > > >+ gApplicationsPane.rebuildActionsMenu(); > This should be called from a select handler in the XUL. Ugh, if I only knew how to do that :( > >Index: mozilla/suite/common/pref/pref-applicationManager.xul > I didn't see this dialog, so I won't try to review it. Add two or more applications for handling a file type, then a "Application Details..." item will be available, calling this dialog for enabling you to remove them again. Everything I have not commented on here is fixed in a local patch - I also found that we had still references to chrome://browser icons and I moved those as well (also feed stuff, which might go better into navigator than communicator, but I consider those parts fine-tuning).

(In reply to comment #10) > > This should be a timed textbox. > > I don't understand what a "timed textbox" is :( OK, figured out after some help on IRC and devmo/MXR digging :) > <listbox flex="1"> > <listhead> > <listheader> Also done now - though some slight problem remain, see comments coming with next patch. > >+ oncommand="gApplicationsPane.onSelectAction(event.originalTarget)"> > This should be an event handler in the XUL. Erm, which XUL? The XBL is the nearest equivalent we have to XUL, the menuitems are all compiled in the JS via DOM operations. > >+ gApplicationsPane.rebuildActionsMenu(); > This should be called from a select handler in the XUL. Does not work - the XBL (content) isn't applied yet when onselect fires, but we need it to be applied for that function. And WRT JS 1.7 - the patch for enabling that in XBL is in, so let works fine now even in a normal pane script :) That said, new patch is arriving shortly!

Here's a reworked patch for the previous comments. The remaining two problems is the listbox scroll position jumping when selecting any line at or near the bottom when its already scrolled - and the columns changing width when selecting rows with different content in the right-hand (action) column.

I did find some chrome://browser icons I didn't see the first time, now added to this zip.

These comments have probably already been addressed on IRC ;-) (In reply to comment #10) >>The docs for removeElementAt even *suggest* indexOf. Sigh... >This might be true if possibleApplicationHandlers would have a full >nsIMutableArray implementation, but it only has a very partial one, see >possibleApplicationHandlers() in FeedHandlerInfo in this patch. Looks like >indexOf() is not implemented there. Sorry, I didn't realise this was the dummy mutable array - but just adding indexOf would be a big win, because the dummy array can just forward that to its internal array. >>>+ _possibleApplicationHandlers: null, >>>+ >>>+ get possibleApplicationHandlers() { >>possibleApplicationHandlers: { >I'm a bit at a loss how to rewrite that whole fuction for this, probably not >possible in this case. Ah yes, I seem to remember looking at it later that it wasn't that simple, but I the forgot to go up and remove the edit. Sorry. >>>+ get primaryExtension() { >>>+ return "xml"; >>>+ }, >>primaryExtension: "xml", >This has only a getter, it can't be set. I know, but that doesn't mean you have to write it using getter syntax. (Same goes for overriding the icons.) >>>+ // If this is the feed type, add a Live Bookmarks item. >>??? >Sure, we don't support livemarks yet, but Callek still plans to work on this, >so I didn't want to rip it out from here. Should I comment out adding this menu >item here for now? Probably a good idea, yes. >>>+ // On Windows, selecting an application to open another application >>>+ // would be meaningless so we special case executables. >>Debugger anyone ;-) >Should I remove this exception? No, it was only a joke. >>This should be a timed textbox. >I don't understand what a "timed textbox" is :( <textbox type="timed"> as used in about:config et. al. >http://developer.mozilla.org/en/docs/XUL:listitem says "If you wish to create a >list with non-text content, you should instead use the richlistbox element." - >and the selected row contains a menulist (see handler-selected XBL) which I'd >count as "non-text content". Is that a wrong assumption? Well, mail has been using non-text content in listitems for years... the only real reason to use a richlistbox is if you want variable-height rows. >>>+ gApplicationsPane.rebuildActionsMenu(); >>This should be called from a select handler in the XUL. >Ugh, if I only knew how to do that :( Something like <listbox onselect="gApplicationsPane.rebuildActionsMenu();">

Comment on attachment 303775 [details] [diff] [review] new applications pref pane, v2 >+ <listbox id="handlersView" persist="lastSelectedType" flex="1" >+ preference="pref.downloads.disable_button.edit_actions" The columns didn't display at all right for me, but I think I know what's missing: <listcols> <listcol width="1" flex="1"/> <listcol width="1" flex="1"/> </listcols> (Don't forget to adjust _rebuildView of course!) The other issue is probably something to do with XBL and listboxes. Let me see what we can do to work around it.

(In reply to comment #15) > missing: > <listcols> Yes, that work fine! Thanks! > The other issue is probably something to do with XBL and listboxes. > Let me see what we can do to work around it. After adding <listcols> this only happens with the last item in the list, which is already much better :) (In reply to comment #14) > >>>+ get primaryExtension() { > >>>+ return "xml"; > >>>+ }, > >>primaryExtension: "xml", > >This has only a getter, it can't be set. > I know, but that doesn't mean you have to write it using getter syntax. > (Same goes for overriding the icons.) But using |primaryExtension: "xml",| gives me an error message about exactly that (same for the icons, which is why I reverted both in the current patch). > >Sure, we don't support livemarks yet, but Callek still plans to work on this, > >so I didn't want to rip it out from here. Should I comment out adding this menu > >item here for now? > Probably a good idea, yes. OK, doing that - actually, adding | && false| in the if and a comment looks even better to me in this case. > >>This should be a timed textbox. > >I don't understand what a "timed textbox" is :( > <textbox type="timed"> as used in about:config et. al. Yes, I have that in the current patch. Note that I also removed the onkeypress as it only did clear the textbox on ESC but the same key also closes the whole prefwindow here, so no use in clearing the box just before closing the window. > >>>+ gApplicationsPane.rebuildActionsMenu(); > >>This should be called from a select handler in the XUL. > >Ugh, if I only knew how to do that :( > Something like <listbox onselect="gApplicationsPane.rebuildActionsMenu();"> Yes, I realized this and tried it - see comment #11 for why I that didn't work (added a comment to rebuildActionsMenu() to stzate that).

>>>+ gApplicationsPane.rebuildActionsMenu(); >>This should be called from a select handler in the XUL. >Does not work - the XBL (content) isn't applied yet when onselect fires, but we >need it to be applied for that function. Bah. So much for that idea. >And WRT JS 1.7 - the patch for enabling that in XBL is in, so let works fine >now even in a normal pane script :) Hmm, the new patch seems to have an odd mixture of let and var...

(In reply to comment #16) >But using |primaryExtension: "xml",| gives me an error message about exactly that Ah, you'll have to put __proto__: last for that to work, probably. (In reply to comment #11) >>>+ oncommand="gApplicationsPane.onSelectAction(event.originalTarget)"> >>This should be an event handler in the XUL. >Erm, which XUL? I was thinking of <listbox oncommand="onSelectAction(event.originalTarget);">

This is to fix the scrolling issue once and for all. The second CSS selector needs to be changed to .handler-action[selected="true"] rebuildActionsMenu needs to get the menulist in two steps. Note: there are still further review comments to follow.

Comment on attachment 303775 [details] [diff] [review] new applications pref pane, v2 >+#ifdef HAVE_SHELL_SERVICE You need to set this up in the Makefile. Or do what pref-navigator.js does instead. >+ var localHandlerApp = Components.classes["@mozilla.org/uriloader/local-handler-app;1"] >+ .createInstance(nsILocalHandlerApp); Components.classes[...] .createInstance(...); >+ return document.getElementById("bundlePrefApplications") >+ .getFormattedString("fileEnding", [extension]); document.getElementById(...) .getFormattedString(...); etc. >+ if (prefSvc.prefHasUserValue(PREF_DISABLED_PLUGIN_TYPES)) Set that pref to the empty string by default to avoid this check? >+ return "moz-icon://goat." + this.primaryExtension + "?size=" + aSize; I'm sorry, but I think we should sacrifice the goats... >+FeedHandlerInfo.prototype = { >+ __proto__: HandlerInfoWrapper.prototype, Put this last to make my primaryExtension: "xml", trick work. (Same goes for the other __proto__ invocations.) >+ // A minimal implementation of nsIMutableArray. It only supports the two >+ // methods its callers invoke, namely appendElement and nsIArray::enumerate. HandlerInfoWrapper would like it to implement indexOf too: indexOf: function indexOf(startIndex, element) { return mInner.indexOf(element, startIndex); }, Yes, the arguments are reversed. IMHO nsIArray got it wrong. >+ if (!defaultApp || !defaultApp.equals(preferredApp)) I'd write this as if (!preferredApp.equals(defaultApp)) but unfortunately myk/biesi made this throw when defaultApp is null; remind me to file a bug on fixing this. >+var gApplicationsPane = { document.getElementById('applications_pane') works just fine, we don't need an extra variable. Also, internally, it avoids having to prefix "this." on lots of properties, although I guess you still need a pref observer object (and possibly unload handler too, I'm not sure whether adding destroy directly as an unload handler will see the globals or not.) >+ prefSvc.addObserver(PREF_FEED_SELECTED_APP, this, false); >+ prefSvc.addObserver(PREF_FEED_SELECTED_WEB, this, false); >+ prefSvc.addObserver(PREF_FEED_SELECTED_ACTION, this, false); >+ prefSvc.addObserver(PREF_FEED_SELECTED_READER, this, false); You're making extra work here; since one of these prefs is a leading substring of all the others it will observe them all at once. (Same goes for audio/video pref observers.) >+ >+ One blank line is enough ;-) >+ // XXX should we be using the XUL sort service instead? Not unless you want to reimplement this in RDF ;-) >+ if (document.getElementById("actionColumn").hasAttribute("sortDirection")) { >+ this._sortColumn = document.getElementById("actionColumn"); >+ // The typeColumn element always has a sortDirection attribute, >+ // either because it was persisted or because the default value >+ // from the xul file was used. If we are sorting on the other >+ // column, we should remove it. >+ document.getElementById("typeColumn").removeAttribute("sortDirection"); >+ } > else >+ this._sortColumn = document.getElementById("typeColumn"); Two alternatives here: 1. Set this._sortColumn = document.getElementById("typeColumn"); first, then you don't need an else clause. 2. Set this._sortColumn = document.getElementById("actionColumn"); first, then you can reuse it in the test (and lose the {}s). >+ var hidePluginsWithoutExtensions = >+ prefSvc.getBoolPref(PREF_HIDE_PLUGINS_WITHOUT_EXTENSIONS); I think I prefer a 4-space indent for lines wrapped like this. >+ var name; >+ if (preferredApp instanceof nsILocalHandlerApp) >+ name = getDisplayNameForFile(preferredApp.executable); >+ else >+ name = preferredApp.name; ?: perhaps >+ if (this.isValidHandlerApp(aHandlerInfo.preferredApplicationHandler)) >+ return aHandlerInfo.preferredApplicationHandler.name; >+ >+ return aHandlerInfo.defaultDescription; ?: perhaps >+ _selectLastSelectedType: function() { Unless the list is disabled then I think this should try to select something, and it should also ensure the selected item is visible. >+ var menu = >+ document.getAnonymousElementByAttribute(typeItem, "class", "actionsMenu"); class is a bad attribute to use generally. (I know, I didn't fix my .xml file either.) Better to use an explicit anonid attribute, themers understand that. >+ var askMenuItem = document.createElement("menuitem"); >+ askMenuItem.setAttribute("alwaysAsk", "true"); >+ let label; Mixing var and let !? >+ askMenuItem.setAttribute("tooltiptext", label); >+ askMenuItem.setAttribute(APP_ICON_ATTR_NAME, "ask"); Dunno why, but it seems odd to have a const for that one attribute. >+ menuItem.setAttribute("oncommand", "document.getElementById('applications_pane').gApplicationsPane.chooseApp(event)"); I'd prefer menuItem.addEventListener("command", chooseApp, false); but I'm not sure whether you can see the globals if you do that. >+ if (!this._sortColumn) >+ return; This can never be true, can it? >+ return t._describeType(a).toLowerCase(). >+ localeCompare(t._describeType(b).toLowerCase()); t._describeType .localeCompare >+ focusFilterBox: function() { >+ this._filter.focus(); >+ this._filter.select(); >+ }, Not used. >+#handlersView > listitem { >+ min-height: 22px; >+} I don't think you need this. >+listitem[appHandlerIcon="app"], >+menuitem[appHandlerIcon="app"] { We might be able to avoid the duplication here, by inheriting the appHandlerIcon attribute to the second listcell, and setting an additional handler-action class on the menuitem. (The XBL style rule would have to be tweaked as well.) >Index: mozilla/suite/common/pref/handlers.css I'm not sure it's worth a separate file for this css (or xbl) I don't know whether the prefwindow already has a css file though. >+ * Make the icons appear. This is theme-specific, so doesn't belong here (icons work in Modern, and there's a forgotten bug about making them work in Classic.) >+.handler-action > image, >+.handler-type > image { Should use the .listcell-icon class instead of the image tag. >+<!DOCTYPE overlay [ >+ <!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd"> >+ %brandDTD; >+ <!ENTITY % applicationsDTD SYSTEM "chrome://communicator/locale/pref/pref-applications.dtd"> >+ %applicationsDTD; >+]> >+ >+<bindings id="handlerBindings" <!DOCTYPE bindings> >+ image="moz-icon://goat?size=16" crop="end" Do we need this image at all? Doesn't it get set already? >Index: mozilla/suite/common/pref/pref-applicationManager.xul Still haven't got this far :-(

>+var gApplicationsPane = { Does window.gApplicationsPane = { work?

Alternatively Mnyromyr thinks that you shouldn't need the document.getElementById('applications_pane'). prefix at all.

(In reply to comment #20) > >+ if (prefSvc.prefHasUserValue(PREF_DISABLED_PLUGIN_TYPES)) > Set that pref to the empty string by default to avoid this check? You mean, so that we'd rely on it being always set? It's currently not set by default, and I always feel safer if we can handle the case of a pref not being set. > >+ prefSvc.addObserver(PREF_FEED_SELECTED_APP, this, false); > >+ prefSvc.addObserver(PREF_FEED_SELECTED_WEB, this, false); > >+ prefSvc.addObserver(PREF_FEED_SELECTED_ACTION, this, false); > >+ prefSvc.addObserver(PREF_FEED_SELECTED_READER, this, false); > You're making extra work here; since one of these prefs is a leading > substring of all the others it will observe them all at once. > (Same goes for audio/video pref observers.) Actually, there's only one that has one other as the leading substring, and that one isn't here. Note the handler/handlers difference of the prefs. Whatever reason that has, I think we don't want to deviate from FF wrt pref names. > Two alternatives here: > 1. Set this._sortColumn = document.getElementById("typeColumn"); first, > then you don't need an else clause. > 2. Set this._sortColumn = document.getElementById("actionColumn"); first, > then you can reuse it in the test (and lose the {}s). The common case is sorting by type, so 1. makes more sense. > >+ _selectLastSelectedType: function() { > Unless the list is disabled then I think this should try to select something, Why? I see no harm in nothing being selected. > and it should also ensure the selected item is visible. Umm, possibly, but 1) again I see no harm in that not being the case and 2) how would I do that? > >+ var askMenuItem = document.createElement("menuitem"); > >+ askMenuItem.setAttribute("alwaysAsk", "true"); > >+ let label; > Mixing var and let !? That's actually intentional as I found out. The let variable is only needed in this scope while the var one is needed below for setting menu.selectedItem (same for the other that are set by var). > >+ askMenuItem.setAttribute(APP_ICON_ATTR_NAME, "ask"); > Dunno why, but it seems odd to have a const for that one attribute. Hmm, possibly - though, in that case, the const declaration allows us to add a comment about what this is at the beginning of the file. Where should I put that instead? > >+ menuItem.setAttribute("oncommand", "document.getElementById('applications_pane').gApplicationsPane.chooseApp(event)"); > I'd prefer menuItem.addEventListener("command", chooseApp, false); I get "chooseApp is not defined" when trying that. > >+ if (!this._sortColumn) > >+ return; > This can never be true, can it? Not when invoked by sort() but possibly when invoked by init() or observe(), I guess. > >Index: mozilla/suite/common/pref/handlers.css > I'm not sure it's worth a separate file for this css (or xbl) > I don't know whether the prefwindow already has a css file though. prefwindow is set by communicator.css, there's no other such file there atm. > >Index: mozilla/suite/common/pref/pref-applicationManager.xul > Still haven't got this far :-( In testing or in reviewing? (In reply to comment #22) > Alternatively Mnyromyr thinks that you shouldn't need the > document.getElementById('applications_pane'). prefix at all. Interesting, when I started work on all this, I remember that didn't work. Now it does - I still need the prefix in the XBL and JS-created menuitems but not in the XUL. :) New patch coming up as soon as I have a diff for it :)

OK, here's the patch addressing all the recent review comments (unless my post above stated otherwise). Note that I also reworked the type icons in a way that we don't have to hardcode URLs for default types in the JS but can set them through the CSS - basically going the same way as action icons. Along with that, I reduced us to not adding the whole feed icon collection but only those we use currently, we can easily modify the theme when we once have large icons in use or add different icons for (video) podcasts.

This are the icons that fit with the latest patch, still those should go into suite/themes/classic/communicator/icons but I only included those that we really need in this patch - and added an extra one for "save", which is just a resized version of the composer save icon.

Neil just pointed out on IRC that I attached the HAVE_SHELL_SERVICE ifdef to the wrong bug, the following change should be part of this patch (and review): Index: mozilla/suite/common/Makefile.in =================================================================== include $(topsrcdir)/config/rules.mk + +# DEFINES for preprocessing +# once we support the shell service, define this for supported platforms +#ifneq (,$(filter windows gtk2 mac cocoa, $(MOZ_WIDGET_TOOLKIT))) +#DEFINES += -DHAVE_SHELL_SERVICE=1 +#endif Note that this is all comments anyways, so I left the defines pointing to all platforms the FF shell service supports for now.

Comment on attachment 305777 [details] [diff] [review] new applications pref pane, v3 Fallout from my comments, sorry about these: >+ if (this.possibleApplicationHandlers.indexOf(0, aNewHandler)) >+ return; >+ this.possibleApplicationHandlers.appendElement(aNewHandler, false); Oddly, indexOf can throw an exception, so you have to write this: try { if (this.possibleApplicationHandlers.indexOf(0, aNewHandler) != -1) this.possibleApplicationHandlers.appendElement(aNewHandler, false); } catch (e) { } >+ var handlerIdx = this.possibleApplicationHandlers.indexOf(0, aHandler); >+ this.possibleApplicationHandlers.removeElementAt(handlerIdx); Similarly you need to wrapthis in a try/catch. >+ let actionsMenu = >+ document.getAnonymousElementByAttribute(typeItem, "class", "actionsMenu"); This needs to be changed to the two-step version too.

(In reply to comment #23) >>>+ prefSvc.addObserver(PREF_FEED_SELECTED_APP, this, false); >>>+ prefSvc.addObserver(PREF_FEED_SELECTED_WEB, this, false); >>>+ prefSvc.addObserver(PREF_FEED_SELECTED_ACTION, this, false); >>>+ prefSvc.addObserver(PREF_FEED_SELECTED_READER, this, false); >>You're making extra work here; since one of these prefs is a leading >>substring of all the others it will observe them all at once. >>(Same goes for audio/video pref observers.) >Actually, there's only one that has one other as the leading substring, and >that one isn't here. Note the handler/handlers difference of the prefs. >Whatever reason that has, I think we don't want to deviate from FF wrt pref >names. The pref service isn't that sophisticated, so it will ping you twice for a change to PREF_FEED_SELECTED_READER because PREF_FEED_SELECTED_ACTION is a prefix and I'm fairly sure it will ping you for the others too. >>>+ _selectLastSelectedType: function() { >>Unless the list is disabled then I think this should try to select something, >Why? I see no harm in nothing being selected. For discoverability of the menulist. (And accessibility too, probably.) >>and it should also ensure the selected item is visible. >Umm, possibly, but 1) again I see no harm in that not being the case and 2) how >would I do that? Forget that, it doesn't seem to work reliably (async stuff, I guess). >>>+ var askMenuItem = document.createElement("menuitem"); >>>+ askMenuItem.setAttribute("alwaysAsk", "true"); >>>+ let label; >>Mixing var and let !? >That's actually intentional as I found out. The let variable is only needed in >this scope while the var one is needed below for setting menu.selectedItem >(same for the other that are set by var). Bah, it's the "var isn't scoped" that's making this so unreadable :-( >>>+ askMenuItem.setAttribute(APP_ICON_ATTR_NAME, "ask"); >>Dunno why, but it seems odd to have a const for that one attribute. >Hmm, possibly - though, in that case, the const declaration allows us to add a >comment about what this is at the beginning of the file. Where should I put >that instead? Well, if it isn't clear from the CSS, then it isn't clear for typeClass either. >>>+ menuItem.setAttribute("oncommand", "document.getElementById('applications_pane').gApplicationsPane.chooseApp(event)"); >>I'd prefer menuItem.addEventListener("command", chooseApp, false); >I get "chooseApp is not defined" when trying that. After this patch is checked in remind me to look at de-uglifying that code. >>>+ if (!this._sortColumn) >>>+ return; >>This can never be true, can it? >Not when invoked by sort() but possibly when invoked by init() or observe(), I >guess. Ah, but init() always sets it. >>>Index: mozilla/suite/common/pref/handlers.css >>I'm not sure it's worth a separate file for this css (or xbl) >>I don't know whether the prefwindow already has a css file though. >prefwindow is set by communicator.css, there's no other such file there atm. Any reason you didn't call it pref-applications.css? >>>Index: mozilla/suite/common/pref/pref-applicationManager.xul >>Still haven't got this far :-( >In testing or in reviewing? Either. But I tested it this time. >Interesting, when I started work on all this, I remember that didn't work. Now >it does - I still need the prefix in the XBL and JS-created menuitems but not >in the XUL. :) Remind me to investigate as part of de-uglification ;-)

Comment on attachment 305777 [details] [diff] [review] new applications pref pane, v3 >Index: mozilla/suite/common/pref/handlers.css OK, so looking around the pref panels, the old panels sometimes had to include specific CSS, because they loaded into frames. I don't think it works for the new panels to load CSS on demand, because it might end up getting loaded multiple times. So, I think prefwindow.xul should load prefpanels.css from both content and skin (renaming handlers.* to prefpanels.*). >+const nsIWebContentHandlerInfo = Components.interfaces.nsIWebContentHandlerInfo; (Needs to be commented out for now.) >+/* ::::: Applications ::::: */ >+/** >+ * Line up the actions menu with action labels above and below it. >+ * Equalize the distance from the left side of the action box to the left side >+ * of the icon for both the menu and the non-menu versions of the action box. >+ * Also make sure the labels are the same distance away from the icons. >+ */ >+.actionsMenu { >+ margin: 0; > } > >+.handler-action > .listcell-icon { >+ margin-top: 1px; >+ margin-bottom: 1px; >+ -moz-margin-start: 5px; >+ -moz-margin-end: 3px; >+} >+.handler-type > .listcell-icon { >+ margin: 1px 3px; >+} >+.handler-action > .listcell-label { >+ -moz-padding-start: 0; >+ padding-top: 1px; > } > >+.actionsMenu > .menulist-label-box { >+ -moz-margin-start: 2px; > } > >+.handler-action > .menu-iconic-left { >+ -moz-padding-start: 0px; >+ -moz-padding-end: 2px; Would you mind either not checking these bits in or waiting for me to fiddle around with the styles until I get something I like? >+ <script type="application/javascript" >+ src="chrome://communicator/content/pref/pref-applicationManager.js"/> >+ <script type="application/javascript" >+ src="chrome://communicator/content/pref/pref-applications.js"/> IMHO pref-applications.js comes first because of its consts. >+ <key id="delete" keycode="VK_DELETE" command="cmd_remove"/> That's confusing because delete is normally cmd_delete ;-) What about Mac? >+ item.setAttribute("image", gApplicationsPane._getIconURLForHandlerApp(app)); >+ item.className = "listitem-iconic"; Nit: swap please. >+<!ENTITY appManager.style "width: 30em; min-height: 20em;"> ch width perhaps?

Comment on attachment 305777 [details] [diff] [review] new applications pref pane, v3 >+.handler-type[typeClass="webFeed"] { >+ list-style-image: url("chrome://communicator/skin/icons/feedIcon16.png"); >+} >+ >+.handler-type[typeClass="videoPodcastFeed"] { >+ list-style-image: url("chrome://communicator/skin/icons/feedIcon16.png"); >+} > >+.handler-type[typeClass="audioPodcastFeed"] { >+ list-style-image: url("chrome://communicator/skin/icons/feedIcon16.png"); > } Nit: more efficient is .handler-type[typeClass="webFeed"], .handler-type[typeClass="videoPodcastFeed"], .handler-type[typeClass="audioPodcastFeed"] { list-style-image: url("chrome://communicator/skin/icons/feedIcon16.png"); }

(In reply to comment #28) > (In reply to comment #23) > >>>+ prefSvc.addObserver(PREF_FEED_SELECTED_APP, this, false); > >>>+ prefSvc.addObserver(PREF_FEED_SELECTED_WEB, this, false); > >>>+ prefSvc.addObserver(PREF_FEED_SELECTED_ACTION, this, false); > >>>+ prefSvc.addObserver(PREF_FEED_SELECTED_READER, this, false); > >>You're making extra work here; since one of these prefs is a leading > >>substring of all the others it will observe them all at once. > >>(Same goes for audio/video pref observers.) > >Actually, there's only one that has one other as the leading substring, and > >that one isn't here. Note the handler/handlers difference of the prefs. > >Whatever reason that has, I think we don't want to deviate from FF wrt pref > >names. > The pref service isn't that sophisticated, so it will ping you twice for a > change to PREF_FEED_SELECTED_READER because PREF_FEED_SELECTED_ACTION is a > prefix and I'm fairly sure it will ping you for the others too. Ah, right, I missed that _READER is there as well. I'm not sure about the others, the nsIPrefBranch2 IDL only talk of branches. > After this patch is checked in remind me to look at de-uglifying that code. OK, sure. Maybe we you can find some way for that. Would be really nice. > >>>+ if (!this._sortColumn) > >>>+ return; > >>This can never be true, can it? > >Not when invoked by sort() but possibly when invoked by init() or observe(), I > >guess. > Ah, but init() always sets it. OK, as as we're always calling init(), it should always be set, right. (In reply to comment #29) > Would you mind either not checking these bits in or waiting for me to fiddle > around with the styles until I get something I like? OK, can do that for now - we should figure out some CSS to make things line up though, ideally on all platforms equally. > >+ <key id="delete" keycode="VK_DELETE" command="cmd_remove"/> > That's confusing because delete is normally cmd_delete ;-) > What about Mac? Actually, I think those Macs that do not have a delete key can easily resort to using the accesskey of the "Remove" button. (In reply to comment #30) > (From update of attachment 305777 [details] [diff] [review]) > >+.handler-type[typeClass="webFeed"] { > >+ list-style-image: url("chrome://communicator/skin/icons/feedIcon16.png"); > >+} > >+ > >+.handler-type[typeClass="videoPodcastFeed"] { > >+ list-style-image: url("chrome://communicator/skin/icons/feedIcon16.png"); > >+} > > > >+.handler-type[typeClass="audioPodcastFeed"] { > >+ list-style-image: url("chrome://communicator/skin/icons/feedIcon16.png"); > > } > Nit: more efficient is > .handler-type[typeClass="webFeed"], > .handler-type[typeClass="videoPodcastFeed"], > .handler-type[typeClass="audioPodcastFeed"] { > list-style-image: url("chrome://communicator/skin/icons/feedIcon16.png"); > } Sure, I just had it the other way as I expected us to add different icons for the podcast types before SM2 final - but we can easily split it up again.

Fixing more nits, carrying forward r+, re-requesting sr. Two problems keep occurring to me with this patch (I start to get unsure if it's my build or the code): 1) The delete key in the applicationManager doesn't work, but the "Remove" button does. 2) The item selected with _selectLastSelectedType() does not appear as selected and cannot be selected with the mouse.

Comment on attachment 306521 [details] [diff] [review] new applications pref pane, v4 >+ <command id="cmd_delete" >+ oncommand="gAppManagerDialog.remove();" >+ disabled="true"/> Does this actually get enabled anywhere?

I randomly see problem 2 with previous versions of the patch, but with v4 the problem is consistenly reproducible ;-) Anyway, this fixes it for me: remove _selectLastSelectedType and add this to the handler XBL implementation: <constructor> <![CDATA[ if (!this.parentNode.disabled && this.type == this.parentNode.getAttribute("lastSelectedType")) this.parentNode.selectedItem = this; ]]> </constructor>

Comment on attachment 306521 [details] [diff] [review] new applications pref pane, v4 These changes should fix the delete key. >+ if (!list.selectedItem) { >+ document.getElementById("remove").disabled = true; document.getElementById("cmd_delete").setAttribute("disabled", "true"); >+ return; >+ } >+ document.getElementById("remove").disabled = false; document.getElementById("cmd_delete").removeAttribute("disabled");

OK, fixed the last few nits (I hope), everything seems to work fine now, carrying forward r+, re-requesting sr

Comment on attachment 307255 [details] [diff] [review] new applications pref pane, v5 Note to self: when fixing theme, also ensure 16x16 reserved on menulist itself

Comment on attachment 305780 [details] communicator/icons for default theme, v3 I'm not convinced about the feed icon, it doesn't look the same as our other feed icon.

Checked in main patch and new icons, and opened up a couple of followups on Modern, icon alignment, and help.

I have noticed that for content type of Application, the menu that you get under Action ends in a separator. Do you want that spinning off into a separate bug?