Aceman, since you're dabbling in this code already, could you take a look at this one, and make it look like all the other folder pickers? :) Thanks, Blake.

Which one? Screenshot?

This removes the special folderLocationPopup class that removes padding and uses the menulist-menupopup class that is used on picker e.g. in the filter list dialog. The folderLocationPopup class (NOT id) is used here: http://mxr.mozilla.org/comm-central/search?string=folderLocationPopup&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central It is not used in Seamonkey but I also fix the one usage in RSS, so please try it out too. Open the subscription dialog and select a folder of an already subscribed feed. The picker will appear in the last line of the dialog. The patch does not seem to have any visual effect on Linux. Please try on Mac and Win.

Comment on attachment 683273 [details] [diff] [review] patch Way better! The only remaining problem is that in customize mode, it gets really small, like so: https://dl.dropbox.com/u/2301433/Screenshots/SmallFolderCustomize.png Perhaps we can set a min-width to avoid that? Thanks, Blake.

So we intentionally shrink the menulist to nothing (remove all children) when it is in a wrapper (i.e. in the customize mode). There is a comment in folderWidgets.xml about it. So this patch sets it to a sane width=100 when in customize mode and then restore to the previous width when out of customize mode. Do we want to do this?

Comment on attachment 683333 [details] [diff] [review] patch v2 Not sure what feedback you want here; SeaMonkey has a different folder picker in its toolbar. The feed-subscriptions.xul change looks fine though, although I've only just noticed that bug 355789 regressed the menulist-menupopup class :-(

Neil, if you see any other place where we would use this picker AND it could get into a wrapper AND we would want it to shrink to almost nothing as it was before the patch.

Comment on attachment 683333 [details] [diff] [review] patch v2 Seems better, but we've lost the icons somehow… https://dl.dropbox.com/u/2301433/Screenshots/MissingIcons.png (We've also lost the icons in the other folder pickers on Mac. It's fine on Windows, though.) Thanks, Blake.

I don't see why this would happen on Mac only (it is also fine on Linux). The change is the same to all themes. I suspect that was caused by bug 795989 where they removed the icon files used by folderMenus.css. Notice they only updated folderPane.css.

(In reply to :aceman from comment #9) > I suspect that was caused by bug 795989 where they removed the icon files > used by folderMenus.css. Notice they only updated folderPane.css. Ah, yikes. :/ Slipped by my radar. I've filed bug 815220.

Blake, Could you please check this patch together with the patch from bug 815220? Maybe you could then revise your ui-r-

Blake, http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/richard.marti@gmail.com-0ed3fdc6437e has this patch and the one from bug 815220 applied. With this it should be easier for the ui-r.

Comment on attachment 683333 [details] [diff] [review] patch v2 The try-build is good, so ui-r=me, once bug 815220 has landed. :) Thanks, Richard (and aceman)!

bwinton, have you also tried moving the widget between the toolbar and the customization palette?

Yes.

Ok, thanks.

Comment on attachment 687864 [details] [diff] [review] patch v3 Review of attachment 687864 [details] [diff] [review]: ----------------------------------------------------------------- Looks ok to me. r=mkmelin

Backed out due to orange http://hg.mozilla.org/comm-central/rev/720ac7edeb08 SUMMARY-UNEXPECTED-FAIL | C:\talos-slave\test\build\mozmill\folder-display\test-folder-toolbar.js | test-folder-toolbar.js::test_add_folder_toolbar EXCEPTION: Uninitialized Folder doesn't have a blank label.: 'Folder Location' != ' '. at: test-folder-display-helpers.js line 2831 assert_true test-folder-display-helpers.js 2831 assert_equals test-folder-display-helpers.js 2818 test_add_folder_toolbar test-folder-toolbar.js 42 Runner.prototype.wrapper frame.js 582 Runner.prototype._runTestModule frame.js 652 Runner.prototype.runTestModule frame.js 698 Runner.prototype.runTestDirectory frame.js 522 runTestDirectory frame.js 704 Bridge.prototype._execFunction server.js 179 https://tbpl.mozilla.org/php/getParsedLog.php?id=17778794&tree=Thunderbird-Trunk&full=1

Yeah, I made this change intentionally: - label=" " + label="&folderLocationToolbarItem.title;" I thought it looks better than blank. Should I revert it or do we update the test?

Update the test, definitely, since this was an intentional change. Thanks, Blake.

The backout missed the file mail/themes/pinstripe/mail/folderMenus.css (probably because there were recent changes and the backout failed on that file, unnoticed).

Comment on attachment 698046 [details] [diff] [review] finish of the backout Review of attachment 698046 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! Great catch, aceman.

Landed the rest of the backout: https://hg.mozilla.org/comm-central/rev/fabf83f68caa

This fixes the test.

Comment on attachment 698105 [details] [diff] [review] patch v4 Adding mconley if he is interested how the fetching of entities got solved ;)

Comment on attachment 698105 [details] [diff] [review] patch v4 Neat technique. :)