The new XBL folder picker for TB 3 doesn't expose the selected folder, and one must track selection events to know which folder it is. This doesn't make sense - there should be a folder attribute or something similar. See http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/messengercompose.xul#482 http://mxr.mozilla.org/comm-central/source/mail/base/content/FilterListDialog.xul#78 for what is currently done to track the selected folder. (there's a selectFolder() method...)

Does this help: http://mxr.mozilla.org/comm-central/source/mail/base/content/folderPane.js#289 ?

(In reply to comment #1) No, we're not talking about the folder pane...

Should this be tagged as a regression, since it worked in 2.0.0.x?

The XBL folder picker is something new, it didn't exist before - but you used to be able to do: MsgFolderPickerOnLoad('actionTargetFolder'); /* etc. etc. */ var uri = document.getElementById('actionTargetFolder').getAttribute('uri'); so let's say it's a regression.

I don't think it qualifies as a regression as I don't expect it was a documented API.

That's not the reason I asked for blocking. Please read the initial comment again. Bustage of an important but undocumented feature, should not be so easily overlooked.

I'm fine w/ this capability being added. I still don't think it blocks.

I'm sure _you're_ fine, you didn't open this bug. Other people are not fine. It's not fair to extension authors to check in an XBL widget which is clearly missing an important method, and busts existing capability, making us have to write ugly workarounds. Also, it would be nicer to first conclude a discussion, then make a decision (assuming it's entirely your decision to make without, say, jminta's input on the matter - an assumption I'm not making).

I support the decision not to block on this. There's no "bustage" and to me, the workaround, while definitely a workaround, isn't prohibitively ugly in my mind. All you have to do is attach an oncommand listener to the menulist, and store the folder attached to the event when it fires. (That's what all the in-tree consumers do.) Yes, it'd be better to be able to access this data at all times, which is why we have this bug. However, I don't know of any capabilities that are actually blocked by this bug. If you have examples that become significantly more difficult without this method, I'd be very interested in seeing them.

I think I can do this.

This is a WIP of the functionality. I ask for feedback if this is the right direction. Please ignore the debugging printouts. Some things to take note of: - this sets the "value" of the parent menulist (having type="folder") to the URI of the folder selected. It intentionally sets "value" instead of .value to not trigger the internal XBL handler of menulist. Is such overload safe? On the other hand having "value" attribute return some useful value seems nice and expected. - it adds a .selectedFolder property to the menupopup of the menulist, that returns the selected nsIMsgFolder object. Is it OK to have here? But also all the other properties are on the menupopup as that has the XBL binding attached. - there is a visual bonus in that now the menulist remembers with item was selected and it is highlighted properly when the menulist is reopened. - I converted some of the callers to the new functionality to see if it simplifies things. But it is possible these changes are backwards compatible even if callers are not converted. - some of the 'oncommand' functions are shortened here and may go away completely in bug 802609. Please tell me if there are any problems in this way. Thanks

(In reply to aceman from comment #11) > - this sets the "value" of the parent menulist (having type="folder") to the > URI of the folder selected. It intentionally sets "value" instead of .value > to not trigger the internal XBL handler of menulist. Is such overload safe? > On the other hand having "value" attribute return some useful value seems > nice and expected. Basically you don't want the selected item to be a grandchild of the menupopup. Even setting the value attribute on the menuitems is unsafe, unless they are direct children. I'm not sure what the code that sets _selectedFolder is trying to achieve. Some of the conversions don't appear to be simplified after all.

Comment on attachment 8356799 [details] [diff] [review] 473009.patch Review of attachment 8356799 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/SearchDialog.js @@ +568,5 @@ > // Get the msg folder we're moving messages into. > // If the id (uri) is not set, use file-uri which is set for > // "File Here". > +alert(destUri); > + if (!destUri || destUri.length == 0) destUri.length == 0 is not needed then

(In reply to neil@parkwaycc.co.uk from comment #12) > (In reply to aceman from comment #11) > > - this sets the "value" of the parent menulist (having type="folder") to the > > URI of the folder selected. It intentionally sets "value" instead of .value > > to not trigger the internal XBL handler of menulist. Is such overload safe? > > On the other hand having "value" attribute return some useful value seems > > nice and expected. > Basically you don't want the selected item to be a grandchild of the > menupopup. Even setting the value attribute on the menuitems is unsafe, > unless they are direct children. OK, I can use a different attribute. Any proposal for its name? > I'm not sure what the code that sets _selectedFolder is trying to achieve. I'll look into that whether we can select values that are children of the menupopup and then manually assigning value may go away. > Some of the conversions don't appear to be simplified after all. Yeah, it may not be shorter in number of chars, but it is much more easier to grasp to just get the chosen value from an attribute or property, than catching it in an oncommand handler. As I said, the oncommand function that just catched the new value and prettified the menulist label will go away completely after the other bug.

This is an updated version, which converts more users. You can now see a case where I could remove a global variable keeping track of the currently selected folder. That should now be unnecessary as we can query the folder from the picker whenever needed. This may duplicate some parts that are being done in bug 878805, to keep this patch working. After bug 878805 lands I will update this patch to remove those duplicated parts. Bug 878805 already implements much of bug 802609 so in bug 802609 I can just remove many of the oncommand functions. Or do you want me to do those changes here (merge the bugs) to see the real code simplifications?

as mentioned in bug 802609, it may not be desirable to force selectFolder() on all users in a binding. it was a non goal of bug 878805 for sure. this patch will have to be reworked, as you say, for bug 878805, perhaps better to just do that, as some of its cleanup is being duplicated. the key thing in this bug is to expose the folder as a property of menulist. i would add a second imortant part of that is to make sure selectFolder() tests for the actual disk/server(imap) existence of a folder. after menupopup is built, folders may go away, outside of Tb change listeners, via an os disk operation. while users shouldn't expect add/change/delete to be reflected in Tb without a restart, no longer valid folders should be handled cleanly if not. it would be incorrect to teardown/build the menulist each time. and even so, subFolders() property may *also* be not current with disk/server state. this edge case problem is best handled in selectFolder(), with something like nsIFile.exists() and a message 'Folder no longer found, choose folder' in the menulist.

Comment on attachment 8364751 [details] [diff] [review] patch v2 OK, so my gut feeling is that you're trying to shoehorn too much into the folder popup binding. I came up with the following scheme: menulist > menupopup[type="folder"] { -moz-binding: url("chrome://messenger/content/folderWidgets.xml#menulist-folder-menupopup"); } In folderWidgets.xml create the binding derived from the existing binding. This binding handles all the stuff related to menulist folders, such as setting the initial folder and updating the selected folder when a selection is made.

Also the non-menulist folders will continue to use event.target._folder, because it doesn't make sense for them to have a selected folder.

Why? They still can get .selectedFolder from the menupopup. So I can remove the setting of the "URI" on the menulist as it seems there will be user of it. And everybody will get the folder from the menupopup. I do not understand the suggestion.

perhaps iann will know? (In reply to neil@parkwaycc.co.uk from comment #18) > Also the non-menulist folders will continue to use event.target._folder, > because it doesn't make sense for them to have a selected folder. (In reply to :aceman from comment #19) > Why? They still can get .selectedFolder from the menupopup. > So I can remove the setting of the "URI" on the menulist as it seems there > will be user of it. And everybody will get the folder from the menupopup. > > I do not understand the suggestion

Has this been superseded by another bug now? Or at least bitrotted?

Yes, this is all bitrotted by bug bug 878805, but it is not ready yet. We just asked for your answer to comment 20.