this means most if not all of: 1. meunulist has class="folderMenuItem" to use the icon and ensure box size uniformity. 2. menupopup has type="folder" to use the folder-menupopup binding. 3. menupopup has mode= for folders to display criteria, if any. 4. menupopup attributes showAccountsFileHere, showFileHereLabel, and fileHereLabel should be consistent. (imo folder name is better than 'choose this'). 5. oncommand should be either on menulist or menupopup, but consistent. the command should ensure the menupopup's _setCssSelectors method runs to get attributes set for the right icon. exceptions should be allowed, but only if they get approved by the bureau that never grants exceptions ;) aceman, i'll leave this for you if you want.

Thanks. I would also add 6. there should be a maximum width set on the picker, as some long account/folder names can expand the containing dialog to unexpected widths (I have seen this to happen). Some pickers already have this set.

7. in folderMenus.css, change .folderMenuItem[ServerType="rss"] to .folderMenuItem[IsFeedFolder="true"] to show the right icon for subscribed/not subscribed folder.

this addresses #s 1-5 and 7. sizes in menulists (which can be in a listcell or button) are better left to individual cases. folderpicker <menulist>s in Account Manager weren't addressed and are a part2 or followup.

Some of the oncommand functions are intended to be removed in bug 473009 and bug 802609. So I am not sure why they are redesigned here as it is not in the description of this bug (there is only consilidating of their placement in menulist or menupopup). If you want, you could move the getDisplayName method to bug 802609 and add fixes according to the discussion there. But surely in bug 802609 I did not want to set the label from via the oncommand functions, but internally in the folder picker code. I didn't like the passing of 'fileHere label' into the widget too, but I am not sure the proposed solution is better. It is a pity you don't do point 6. so we will need to change all the same menulists in other bug again. Other than that I like there is consolidation done on the picker widget, thanks.

The consistency part means all menulists look the same, after a folder has been selected, and that can only be done by using the binding's selectFolder() to set the attrs, ie replacing SetFolderPickerElement in oncommands. It doesn't appear to be done in the referenced bug's patch in oncommands nor the binding's command handler. Labels are set by the binding's setupParent() so I don't know what you mean. By the way, it's rather bad that selectFolder() throws, there's no reason for that. The label should be set at Choose Folder if a folder is null or not found in the menupopup's children. I have a patch for it in a followup.

Comment on attachment 8361713 [details] [diff] [review] folderpickers.patch Review of attachment 8361713 [details] [diff] [review]: ----------------------------------------------------------------- I'll try to actually run with this over the weekend, but the code here generally looks good. Comments below. ::: mailnews/base/content/folderWidgets.xml @@ +677,5 @@ > + - This function returns a formatted display name for a menulist > + - selected folder. The desired format is set as the 'displayformat' > + - attribute of the folderpicker's <menulist>, one of: > + - 'verbose' - Folder on Account > + - 'path' - Account/Folder/Subfolder Could you add another option here to show only the name? I see that in getDisplayName(), anything other than "verbose" or "path" just gives the name. Let's add an explicit option for that here like so: - 'name' (default) - Folder You don't need to change the code for getDisplayName at all, but this would make it clearer that there are actually 3 possible display formats. ::: mailnews/base/search/content/searchWidgets.xml @@ -11,5 @@ > -gFilter from FilterEditor.js > -gCustomActions from FilterEditor.js > -gFilterType from FilterEditor.js > -checkActionsReorder from FilterEditor.js > - -SetFolderPickerElement from msgFolderPickerOverlay.js It looks like there's one use of this function that you didn't remove: <http://mxr.mozilla.org/comm-central/source/mailnews/base/search/content/searchWidgets.xml#717>. Should we remove that too?

(In reply to Jim Porter (:squib) from comment #6) > ::: mailnews/base/content/folderWidgets.xml > @@ +677,5 @@ > > + - This function returns a formatted display name for a menulist > > + - selected folder. The desired format is set as the 'displayformat' > > + - attribute of the folderpicker's <menulist>, one of: > > + - 'verbose' - Folder on Account > > + - 'path' - Account/Folder/Subfolder > > Could you add another option here to show only the name? I see that in > getDisplayName(), anything other than "verbose" or "path" just gives the > name. Let's add an explicit option for that here like so: The plan in bug 802609 was that the picker should be able to determine which format to use automatically. But if you guys want a way to manually override it then I am fine with that. But don't forget the automatic path. --- a/mailnews/base/content/folderWidgets.xml +++ b/mailnews/base/content/folderWidgets.xml @@ -17,16 +17,22 @@ this._stringBundle = new this .StringBundle("chrome://messenger/locale/folderWidgets.properties"); + this._stringBundleMessenger = new this + .StringBundle("chrome://messenger/locale/messenger.properties"); I hope the "verboseFolderFormat" string can be moved to folderWidgets.properties after you convert all the users of the picker.

(In reply to Jim Porter (:squib) from comment #6) > Comment on attachment 8361713 [details] [diff] [review] > folderpickers.patch > > > - 'name' (default) - Folder sure. > > ::: mailnews/base/search/content/searchWidgets.xml > @@ -11,5 @@ > > -gFilter from FilterEditor.js > > -gCustomActions from FilterEditor.js > > -gFilterType from FilterEditor.js > > -checkActionsReorder from FilterEditor.js > > - -SetFolderPickerElement from msgFolderPickerOverlay.js > > It looks like there's one use of this function that you didn't remove: > <http://mxr.mozilla.org/comm-central/source/mailnews/base/search/content/ > searchWidgets.xml#717>. Should we remove that too? Yes, that's done in a followup. It's slightly more complex in this constructor as it requires a change to selectFolder() to handle a caller wanting to not set a default folder. In the case of this picker, the listcell menulist of a New Filter option should not be set to Account folder as a move/copy target, but should show Choose Folder and let the filter/user handle it. This is also true for the picker in the FilterListDialog; an Account folder is wrong as a run filter target (for feeds). It would be better to keep this patch simpler for the core changes and, given other work in this area, followup. (In reply to :aceman from comment #7) > The plan in bug 802609 was that the picker should be able to determine which > format to use automatically. But if you guys want a way to manually override > it then I am fine with that. But don't forget the automatic path. I think it's more flexible to have an attribute to pick the format. > > I hope the "verboseFolderFormat" string can be moved to > folderWidgets.properties after you convert all the users of the picker. Yes, that's where it belongs. The msgFolderPickerOverlay.js should go away. There are 2 remaining instances of SetFolderPicker in FilterEditor.js, which require gActionTargetElement, something which isn't set anywhere in the c-c codebase. I don't know if this was slipped in without documentation for an extension or not, but it's odd (and not good).

updated.

(In reply to alta88 from comment #8) > > I hope the "verboseFolderFormat" string can be moved to > > folderWidgets.properties after you convert all the users of the picker. > Yes, that's where it belongs. The msgFolderPickerOverlay.js should go away. > There are 2 remaining instances of SetFolderPicker in FilterEditor.js, which > require gActionTargetElement, something which isn't set anywhere in the c-c > codebase. I don't know if this was slipped in without documentation for an > extension or not, but it's odd (and not good). Yes, killing of msgFolderPickerOverlay.js is discussed in bug 802609 so there is not much opposition.

part2. 1. convert remaining pickers (non Account Manager) to selectFolder(), remove msgFolderPickerOverlay.js from includes, but not yet the tree 2. strings 3. fix FilterListDialog.js to select run target properly part3 is Account Manager. aceman, hopefully we don't clobber each other too much, functionally there doesn't seem to be overlap but files touched are a bit..

tweaks to make showAccountsFileHere=false work, make nntp folders selection better in FilterListDialog.

Comment on attachment 8362108 [details] [diff] [review] folderpickers.patch Review of attachment 8362108 [details] [diff] [review]: ----------------------------------------------------------------- UI wise this looks better than the "choose this folder" etc. so ui-r+ Under Win7 now the first menuitem in submenus is taller than the others. This comes from http://mxr.mozilla.org/comm-central/source/mail/themes/windows/mail/mailWindow1-aero.css#218. If you have to change this or an other patch in this bug, could you remove this rule? If not we need a follo-up bug for this. I still see in Messages menu "Move To..." and "Copy To..." a "File Here" and a "Copy Here". Are they addressed in one of the bugs mentioned in comment 4?

Comment on attachment 8362150 [details] [diff] [review] folderpickers2.patch With this patch applied I get this in console Sun Jan 19 2014 14:22:55 Error: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIStringBundle.formatStringFromName] Source file: XStringBundle Line: 37 When I go in Account Manager to "Copies & Folders".

(In reply to Richard Marti (:Paenglab) from comment #13) > Comment on attachment 8362108 [details] [diff] [review] > folderpickers.patch > > Review of attachment 8362108 [details] [diff] [review]: > ----------------------------------------------------------------- > > UI wise this looks better than the "choose this folder" etc. so ui-r+ > > Under Win7 now the first menuitem in submenus is taller than the others. > This comes from > http://mxr.mozilla.org/comm-central/source/mail/themes/windows/mail/ > mailWindow1-aero.css#218. If you have to change this or an other patch in > this bug, could you remove this rule? If not we need a follo-up bug for this. Will do in an update. > > I still see in Messages menu "Move To..." and "Copy To..." a "File Here" and > a "Copy Here". Are they addressed in one of the bugs mentioned in comment 4? The first 2 are correct, the latter 2 don't show but aren't needed and will be removed. (In reply to Richard Marti (:Paenglab) from comment #14) > Comment on attachment 8362150 [details] [diff] [review] > folderpickers2.patch > > With this patch applied I get this in console > > Sun Jan 19 2014 14:22:55 > Error: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 > (NS_ERROR_FAILURE) [nsIStringBundle.formatStringFromName] > Source file: XStringBundle > Line: 37 > > When I go in Account Manager to "Copies & Folders". Account Manager is part3, so the update will put the string back, it was moved prematurely.

updated.

Comment on attachment 8362222 [details] [diff] [review] folderpickers2.patch Review of attachment 8362222 [details] [diff] [review]: ----------------------------------------------------------------- Looks good.

Comment on attachment 8362222 [details] [diff] [review] folderpickers2.patch Review of attachment 8362222 [details] [diff] [review]: ----------------------------------------------------------------- Ok, I've gone through the code and made some comments. I will try to actually run this later today. ::: mail/base/content/FilterListDialog.js @@ +115,5 @@ > > // Get the folder where filters should be defined, if that server > // can accept filters. > + let args = window.arguments; > + gSelectedFolder = args && args[0] && args[0].folder ? args[0].folder : null; Nit: I'd probably just do something like this instead: try { gSelectedFolder = window.arguments[0].folder; } catch(e) { gSelectedFolder = null; } I think that's more readable. @@ +825,5 @@ > return msgFolder; > > try { > + if (msgFolder.server.type == "rss") > + // Choose Folder for feeds. Nit: could you put this comment above the if() block or put braces around the body? It's a bit weird seeing a no-brace if statement have two indented lines below it. ::: mailnews/base/content/folderWidgets.xml @@ +385,5 @@ > + (!mode || > + (this.getAttribute("showFileHereLabel") == "true" && > + (this._parentFolder.noSelect || > + this._parentFolder.canFileMessages || mode == "newFolder" || > + this.getAttribute("showAccountsFileHere") == "true")))) { This if statement is pretty complex. I think we could simplify it and make it read the same way as the comments above describe it like so: let parent = this._parentFolder; if (parent && (this.getAttribute("showFileHereLabel") == "true" || !mode)) { let showAccountsFileHere = this.getAttribute("showAccountsFileHere"); if ((!parent.isServer || showAccountsFileHere != "false") && (!mode || mode == "newFolder" || parent.noSelect || parent.canFileMessages || showAccountsFileHere == "true")) { /* create the menuitem */ } } @@ +714,5 @@ > <method name="selectFolder"> > <parameter name="aFolder"/> > <body><![CDATA[ > // Set the label of the aParent element as if aFolder had been selected. > + function setupParent(aFolder, aParent, aNoFolders) { You use aParent.menupopup a bunch in this function. I think it would help readability if you said "let menupopup = aParent.menupopup;" and then just used "menupopup" below. @@ +719,4 @@ > aParent.setAttribute("label", > + aFolder ? aParent.menupopup.getDisplayName(aFolder) : > + aNoFolders ? aParent.menupopup._stringBundle.getString("noFolders") : > + aParent.menupopup._stringBundle.getString("chooseFolder")); I think our style guides frown upon multiple ternary operators in one statement. How about doing something like this: if (aFolder) { aParent.setAttribute("label", menupopup.getDisplayName(aFolder)); } else { aParent.setAttribute("label", menupopup._stringBundle.getString( aNoFolders ? "noFolders" : "chooseFolder" )); } @@ +733,3 @@ > aParent.setAttribute("IsFeedFolder", > + aFolder ? (aParent.menupopup.FeedUtils.getFeedUrlsInFolder(aFolder) ? true : false) : > + false); You could replace this with: aParent.setAttribute("IsFeedFolder", Boolean( aFolder && menupopup.FeedUtils.getFeedUrlsInFolder(aFolder) ));

updated.

1. convert remaining Account Manager pickers (server settings-imap, copies/folders, junk settings). 2. remove msgFolderPickerOverlay.* references from Tb. 3. make saved (virtual) folder name look pretty in Properties.

suite part. msgFolderPickerOverlay.xul/.js can really be removed, used really only in gettng a folder in the filters run target using the rdf method.

Comment on attachment 8362108 [details] [diff] [review] folderpickers.patch Review of attachment 8362108 [details] [diff] [review]: ----------------------------------------------------------------- For some reason, the submenus in the new folder dialog don't open when you mouse over them like the folder picker does in other places. This seems to be the case in v24 too, so it's not an issue with the patch itself, but we should fix it here or in a followup. Other than that, r=me pending a successful try build (which I'll kick off shortly).

Comment on attachment 8362511 [details] [diff] [review] folderpickers2.patch Review of attachment 8362511 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. r=me once try passes.

Comment on attachment 8362697 [details] [diff] [review] folderpickers3.patch Review of attachment 8362697 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. r=me with successful try build.

Here's the try run: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=5465964e5e05

Also, thanks for looking into this! I was going to try to fix up the folder pickers a while ago, but the code was pretty hairy and I decided I wanted to go do something else instead.

(In reply to Jim Porter (:squib) from comment #26) > Also, thanks for looking into this! I was going to try to fix up the folder > pickers a while ago, but the code was pretty hairy and I decided I wanted to > go do something else instead. I'll refrain from commenting on the code, but it became too unbearable to use or look at them all anymore. @paenglab, as an aside, do the icons and text look off to you in the menulists (or other menu-iconic things, and even the folderpane icons)? Ie the icon is too low relative to the text, at least on linux. It's likely a core global layout issue though.

Comment on attachment 8362697 [details] [diff] [review] folderpickers3.patch Looks good. ui-r+ On Windows the icons are looking right aligned (although I opened bug 961506 for the different item tallness). I'll check this evening on Linux. But this wouldn't be a issue caused by this bug.

I find it sad that the oncommand functions didn't loose the setting of the label (now call to .selectFolder) altogether. You seem to implement the label setting despite Neil's bug 802609 comment 7. Maybe it is because you still call the .selectFolder externally (from the oncommand functions) and pass the folder in, instead of the widget itself knowing what was clicked on. But no problem, I'll try to clean it up in bug 802609 and bug 473009, where the patch will now need a rewrite and touch the same lines. With the removals in am-junk.js I hope you do not reintroduce bug 709581 and dependents. Can you try those scenarios with the patch? There should also be tests for it in mail/test/mozmill/account/test-account-settings-infrastructure.js.

I checked the icons and text on Linux Mint. Here they are looking good aligned (centered in menuitem). Only one or two icons could be better aligned in folder-pane.png like the drafts icon which should be 1 px taller. Now with 15px it is all the time off by 1px, now for me to high. But this could also change with different fonts. I'm using the default font system setting.

Comment on attachment 8362698 [details] [diff] [review] suitestrings.patch > +# LOCALIZATION NOTE(verboseFolderFormat): %1$S is folder name, %2$S is server name > +verboseFolderFormat=%1$S on %2$S > +chooseFolder=Choose Folder\u2026 Suite consistently uses the literal UTF-8 ellipsis character instead of an escaped string \u2026. r=me with this fixed

udpated.

https://hg.mozilla.org/comm-central/rev/62ad06c7ba12 https://hg.mozilla.org/comm-central/rev/8779877620ee https://hg.mozilla.org/comm-central/rev/2db24bbb4a11 https://hg.mozilla.org/comm-central/rev/e5faabbee912

This broke the SeaMonkey FilterListDialog. What changes do we need to make on our side?

(In reply to alta88) > 5. oncommand should be either on menulist or menupopup, but consistent. As it turns out from bug 1106225, it should not be on the menupopup.

So the event bubble model is broken for menus then?

(In reply to alta88 from comment #36) > So the event bubble model is broken for menus then? No, it's just that the menupopup needs to be cloned in order to work around an OSX issue, but then the clone gets its own event listener, and therefore both menupopups receive the event.

That's broken then in a way specific to cloning. It didn't used to be that listeners were retained on dom element moves/copies, for example, and had to be re-added manually. Plus, events in menus do bubble in unexpected/incorrect ways; why should the default behavior not be for the event target or closest ancestor only to process the event? Ref bug 867393.