813218 - Folder Picker in Toolbar is _waaaayyyy_ too condensed.

Status: RESOLVED FIXED

(Thunderbird :: Toolbars and Tabs, defect)

Description

[Blake Winton (:bwinton) (:☕️)]

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.

Comment 1

[:aceman]

Which one? Screenshot?

Comment 2

[Blake Winton (:bwinton) (:☕️)]

https://dl.dropbox.com/u/2301433/Screenshots/FolderTiny.png

Comment 3

[:aceman]

Attachment: patch

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 4

[Blake Winton (:bwinton) (:☕️)]

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.

Comment 5

[:aceman]

Attachment: patch v2

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 6

[[e-mail]]

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 :-(

Comment 7

[:aceman]

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 8

[Blake Winton (:bwinton) (:☕️)]

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.

Comment 9

[:aceman]

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.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

(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.

Comment 11

[Richard Marti (:Paenglab)]

Blake,

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

Comment 12

[Richard Marti (:Paenglab)]

Blake, http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/[e-mail]-0ed3fdc6437e has this patch and the one from bug 815220 applied. With this it should be easier for the ui-r.

Comment 13

[Blake Winton (:bwinton) (:☕️)]

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)!

Comment 14

[:aceman]

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

Comment 15

[Blake Winton (:bwinton) (:☕️)]

Yes.

Comment 16

[:aceman]

Attachment: patch v3

Ok, thanks.

Comment 17

[Magnus Melin [:mkmelin]]

Comment on attachment 687864 [details] [diff] [review]
patch v3

Review of attachment 687864 [details] [diff] [review]:
-----------------------------------------------------------------

Looks ok to me. r=mkmelin

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/48afd0b23600

Comment 19

[Magnus Melin [: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

Comment 20

[:aceman]

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?

Comment 21

[Blake Winton (:bwinton) (:☕️)]

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

Thanks,
Blake.

Comment 22

[:aceman]

Attachment: finish of the backout

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 23

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 698046 [details] [diff] [review]
finish of the backout

Review of attachment 698046 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! Great catch, aceman.

Comment 24

[Mike Conley (:mconley) (:⚙️)]

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

Comment 25

[:aceman]

Attachment: patch v4

This fixes the test.

Comment 26

[:aceman]

Comment on attachment 698105 [details] [diff] [review]
patch v4

Adding mconley if he is interested how the fetching of entities got solved ;)

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/764a9ff6e1ac

Comment 28

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 698105 [details] [diff] [review]
patch v4

Neat technique. :)