Integrate label setting code from SetFolderPickerElement() in msgFolderPickerOverlay.js into the folderWidgets.xml picker. Currently the label after picking a folder must be set from oncommand= on the parent menulist with code like this: function onFolderSelect(event) { dialog.folder = event.target._folder; document.getElementById("msgNewFolderPicker") .setAttribute("label", dialog.folder.prettyName); (http://mxr.mozilla.org/comm-central/source/mailnews/base/content/newFolderDialog.js#49) This seems ugly and is also inconsistent as not all callers do this properly (e.g. some show "folder on server", some only "folder" even when any server can be chosen). There is also a MsgFolderPickerOnLoad function that seems unused so after this change maybe the whole file could be removed. Is it wished to kill msgFolderPickerOverlay.js or do we actually want to put more stuff into it? E.g. there is a 'function prettyFolderName' in amUtils.js that could be merged into it. But if this bug is solved as I propose this function will be removed too (as the picker will do the label setting itself).

There is already label setting code in the method selectFolder, but is usually overriden by label setting by the caller. My proposal suggests to teach it the "folder on server" format and add a <handler event="click"> that activates this label setting instead of the caller needing to do it.

Can anybody please comment if this is wanted?

That sounds to me like a reasonable thing to do. I wouldn't call it a top priority though.

I see seamonkey uses the msgFolderPickerOverlay.* a bit more so it can't be removed so easily. But maybe it can be moved under suite/: http://mxr.mozilla.org/comm-central/search?string=msgFolderPickerOverlay.&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

Once the picker knows about the proper formatting, it should also use verboseFolderFormat string to format the "folder on server" label in the Recent menu.

Neil?

(In reply to aceman) > Integrate label setting code from SetFolderPickerElement() in > msgFolderPickerOverlay.js into the folderWidgets.xml picker. This is fine assuming you'd track the folder as a property on the widget. (In reply to aceman from comment #5) > Once the picker knows about the proper formatting, it should also use > verboseFolderFormat string to format the "folder on server" label in the > Recent menu. Don't use the verbose format if you've got a root folder though. (In reply to aceman from comment #4) > I see seamonkey uses the msgFolderPickerOverlay.* a bit more so it can't be > removed so easily. SeaMonkey might have a load of unused references to that .js file, better to ident the functions in it to see if they're really used first. The one obvious one is that we're still using RDF in our filter list dialog, so someone would have to convert that to use the folder widget before msgFolderPickerOverlay could be removed.

InvisibleSmiley, would you be able to do for SeaMonkey what Neil describes in comment 7? (In reply to neil@parkwaycc.co.uk from comment #7) > (In reply to aceman) > > Integrate label setting code from SetFolderPickerElement() in > > msgFolderPickerOverlay.js into the folderWidgets.xml picker. > This is fine assuming you'd track the folder as a property on the widget. Do I understand it correctly that what you say is bug 473009?

(In reply to aceman from comment #8) > (In reply to comment #7) > > (In reply to aceman) > > > Integrate label setting code from SetFolderPickerElement() in > > > msgFolderPickerOverlay.js into the folderWidgets.xml picker. > > This is fine assuming you'd track the folder as a property on the widget. > Do I understand it correctly that what you say is bug 473009? Looks like that's exactly what I was thinking was needed.

OK, I'll probably need to work on both bugs at the same time.

(In reply to :aceman from comment #8) > InvisibleSmiley, would you be able to do for SeaMonkey what Neil describes > in comment 7? (Sorry for the late reply) Reading "still using RDF" scares me. Neil is probably much better suited to attack this properly.

InvisibleSmiley, is it not possible to just copy how TB does the picker in the filter list dialog?

(In reply to :aceman from comment #12) > InvisibleSmiley, is it not possible to just copy how TB does the picker in > the filter list dialog? Sorry, don't know. I'm not super familiar with that code and am not going to have enough time to dive into it any time soon, hence the pointer to Neil. Or Mnyromyr, or IanN maybe.

this bug is pretty much obsoleted by bug 878805. imo, it is not necessary or even desirable to enforce a selectFolder() in an onclick event in the binding. a folderpicker menulist user may not want that; there is nothing wrong with the consumer needing to call it, if the behavior is wanted. i think this can be closed.

Why would the consumer have to do work if the widget can do it automatically. I find most of the existing oncommand functions unneeded if we can implement it in the widget. Not to mention that there are consumers that set the label wrongly. If the widget can make it correctly and automatically, why not do it. Of course, if there is other work done in oncommand function, that can still be left working. As shown in bug 473009.

(In reply to :aceman from comment #15) > Why would the consumer have to do work if the widget can do it automatically. > I find most of the existing oncommand functions unneeded if we can implement > it in the widget. Not to mention that there are consumers that set the label > wrongly. If the widget can make it correctly and automatically, why not do > it. Of course, if there is other work done in oncommand function, that can > still be left working. As shown in bug 473009. it's always set correctly if selectFolder() is now called, which is what bug 878805 did across the codebase. it's completely unimportant to force it in the binding and if you want to do it post bug 878805 it's up to you; i don't have anything to add to what's already been said.

Note for myself: 1. remove now unused prettyFolderName from amUtils.js 2. try to remove now duplicate verboseFolderFomat from messenger.properties 3. add maxwidth on folder pickers where it is missing (as requested in bug 878805 comment 1 but unimplemented)

So this is my proposal for the change. The folder picker sets the label itself without needing the caller to do it. It can be seen in the patch that there were still some left-over callers that did set the label and even in conflict with what the picker wanted (like there was displayformat=verbose but the caller showed only folder name. So all this should be cleaned up now. There is also some cleanup of dead code. I changed the format of IMAP folder Trash folder because that does not need to be "verbose" assuming we can only pick folders inside the IMAP server, not on other servers. I tried to test all the pickers this is touching, thus I found that the filter editor pickers aren't working on trunk, due to bug 1489172. That got reverted temporarily, so the hunk in mailnews/base/search/content/searchWidgets.js here will not apply for now. Try run is at https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&selectedJob=213689330&revision=6b1fd7c6ed52b4224cbfbbccdaad941b28e17b5f . There is a failure in mozmill\account\test-account-deletion.js which I'm not sure is caused by this patch. Can you please test it locally?

Corresponding Seamonkey patch if the main patch gets accepted.

Comment on attachment 9027360 [details] [diff] [review] 802609.patch Review of attachment 9027360 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/FilterListDialog.js @@ +179,5 @@ > * > * @param msgFolder The nsIMsgFolder server containing filters > * (or a folder for NNTP server). > */ > +function setFilterFolder(msgFolder, aUpdatePicker = true) { let's not add any more a-named parameters... ::: mailnews/base/content/folderWidgets.xml @@ +573,5 @@ > + if (event.target.localName != "menu") { > + this.selectFolder(event.target._folder); > + event.stopPropagation(); > + }}, > + { once: false }); odd indention. I think the common one is (event) => { }, { once: false });

(In reply to Magnus Melin [:mkmelin] from comment #20) > > +function setFilterFolder(msgFolder, aUpdatePicker = true) { > > let's not add any more a-named parameters... Why? Since when did it change?

This function didn't use them in the first place (which is good). Sent an email just now about this to maildev. I think all new code has stopped using a's - it doesn't add any value.

Comment on attachment 9027360 [details] [diff] [review] 802609.patch Review of attachment 9027360 [details] [diff] [review]: ----------------------------------------------------------------- applying 802609.patch unable to find 'mailnews/base/search/content/searchWidgets.js' for patching

Comment on attachment 9027361 [details] [diff] [review] 802609-SM.patch Waiting with the review until the main patch gets r+. One thing: > suite/locales/en-US/chrome/mailnews/messenger.properties Please don't remove any l10n variables in suite unless the patch goes into 60 too. We need them for 2.57 based on ESR60 which isn't finished yet. The plan is to merge the current l10n-central variables in suite only with the esr60 l10n base.

(In reply to Frank-Rainer Grahl (:frg) from comment #24) > Please don't remove any l10n variables in suite unless the patch goes into > 60 too. We need them for 2.57 based on ESR60 which isn't finished yet. The > plan is to merge the current l10n-central variables in suite only with the > esr60 l10n base. This isn't intended for ESR60, it is a big change (and more will follow on top). So what should I do? Leave the string in? Will you then remember to remove it later?

Fixed the 'a' named arguments and updated after the action target patch has landed again. One hunk depends on bug 1508415 getting landed.

> So what should I do? Leave the string in? Yes please. > Will you then remember to remove it later? Sure if there is a later after 2.57. FRG

Sadly there is still the failure in test_account_data_deletion :(

Comment on attachment 9029145 [details] [diff] [review] 802609.patch v1.1 Review of attachment 9029145 [details] [diff] [review]: ----------------------------------------------------------------- Looks reasonable in general. Reflag for review once the tests pass ::: mailnews/base/content/folderWidgets.xml @@ +573,5 @@ > + if (event.target.localName != "menu") { > + this.selectFolder(event.target._folder); > + event.stopPropagation(); > + }}, > + { once: false }); strange indention this.addEventListener("click", (event) => { if (event.target.localName != "menu") { this.selectFolder(event.target._folder); event.stopPropagation(); }}, { once: false }); @@ +729,5 @@ > > <!-- > - Makes a given folder selected. > - > + - @param aFolder (optional) The folder to select (if none, then Choose Folder). @param {nsIMsgFolder} [aFolder] http://usejsdoc.org/tags-param.html#optional-parameters-and-default-values

Comment on attachment 9027361 [details] [diff] [review] 802609-SM.patch Clearing review for now. aceman please ask again when the base patch gets r+

Yes, the failure in test_account_data_deletion seems persistent. Possibly the patch causes some reference to a folder be held and then some of the files for the folder/account cannot be deleted on Windows (due to the file being open). Or it just exposes some other race condition of holding files. In the account manager we have an account displayed and still the backend tries to remove it from under our feet and then update the UI. It seems to work, but may have some catch. On Windows, at the end of the test, the account appears to be removed, but it leaves some files behind in the account directory, that should have been deleted. I need to look into it.