465269 - Folder location toolbar item does not work

Status: VERIFIED FIXED

(Thunderbird :: Toolbars and Tabs, defect)

Description

[Kent James (:rkent)]

(probable regression from bug 414038)

STR:

1) In TB, add the "Folder location" toolbar item.

Expected results: list of folders is shown

Actual results: blank entry with a folder icon in the toolbar.

Comment 1

[Mark Banner (:standard8)]

This was an intentional regression from bug 414038, I think we're planning on fixing it at a later stage.

Comment 3

[Ronald Killmer]

(In reply to comment #2)
> *** Bug 466329 has been marked as a duplicate of this bug. ***

An addition from that bug is an error Console report I pasted in.

Comment 4

[Wayne Mery (:wsmwk)]

FWIW, implied by comment 1, this widget is not on the toolbar by default.

My typical use case - which maybe no one else does - is when I want the entire screen just for thread and preview panes, i.e. I remove the folder pane by dragging the splitter far left. So it does have value.

Comment 5

[Bryan Clark (DevTools PM) [:clarkbw]]

It'd be great to have a clearer understanding of the use case for this.  The use case I originally understood was a replacement for the folder pane on systems that had small screens.  For this use case I had worked out, what I thought was, a better method of changing this into a "mode".  Right now a person has to add this item to the toolbar and then collapse their folder pane; where a mode would do those changes automatically and then also revert things back.

It also might make sense to push this out to an extension, at least that had been my hope under the above use case.  We don't really have a great system for moving existing code out into extensions but it would be excellent to improve that.

Comment 6

[Ronald Killmer]

Historically Netscape used a similar Widget because They did not have a Folder Pane, a feature introduced in Mozilla Suite.  The Toolbar widget was added as an  RFE to Tbird to gain ability for hiding the folder pane and use the drop down list as an alternate folder access method. The widget can be parked either on the Menu bar or the Tool bar.

I perceive value in retaining the widget, it is consistent with the marketing strategy of User customizable.  Yet the "Mode" idea Bryan sketched in Comment #5 has merit, if closing the Folder Pane auto places the Widget on one of the two Bars. Then I would expect an assignment of a persistent Ordinal so each restore happens at the same placement. Also some confilct manager would be needed to detect space collision cases if a Bar becomes overloaded.

Bar overload is less a Fx problem since it has Custom Toolbar functions that permit adding more toolbars. For Tb this requires an extension unless We decide to turn on that aspect of the ? codebase. Before I wander off into another tool bar bug topic I will stop here.

Comment 7

[David Ascher (:davida)]

Joey, you probably know this code best -- would it be easy to make an add-on that implemented the folder picker, given that the strength of the use case seems weak.

Comment 8

[Bryan Clark (DevTools PM) [:clarkbw]]

Dropping this off the b2 radar until someone takes this on.  We will have to continue to relnote this for the b2 release.

Comment 9

[Chris Andrichak]

Seeing this problem in the b2 smoketests:
https://litmus.mozilla.org/single_result.cgi?id=169255

Adding the folder dropdown to the toolbar - it's blank at first, and then when I try to click on it, the chrome/window flashes white then redraws. Next click on it does nothing. Next click causes the flash again. Repeat the every-other-click-flash pattern ad infinitum.

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

Re-assigning to jminta in the hopes that he'll have the bandwidth to take a run at this.  Joey, any idea how much work this is likely to be?

Comment 11

[Joey Minta]

There's the basics of an js implementation to replace the rdf one that no longer works in bug 442802. Bienvenu and I found some problems with scrolling and clicking in that version, but it'd be a good start for someone who wanted to work on this. Without a better understanding of those problems though, I'd struggle to provide an estimate of workload.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Is getting a better understanding of those problems something you're likely to have bandwidth to do in the coming weeks (b3 is projected to be about 9 or 10 weeks out; I should have a straw-man schedule soon).

Comment 13

[Bryan Clark (DevTools PM) [:clarkbw]]

just a quick note of warning to watch out for bug 431573 while looking at fixing this

Comment 14

[Michael Peters]

In the german web-forum www.thunderbird-mail.de we have a long discussion about this bug. Please think about business-users like me, who are using sub-notebooks with small displays. Since the early days of thunderbird I never used the folderpane because I want to have maximum space to read the maillist on my 12-inch-Display. The folder location toolbar is a "must-have" for all netbook-users as well. I will not change to TB3, if the folder location toolbar is dropped away for the new and big "killfeature-search-toolbar". Please fix this bug...

Comment 15

[Ananth S]

It is a regression from the previous release. Please treat this as show-stopper issue.

Comment 16

[Mark Banner (:standard8)]

(In reply to comment #14)
> Please fix this bug...

(In reply to comment #15)
> It is a regression from the previous release. Please treat this as show-stopper
> issue.

Please note, this bug is marked as blocking Thunderbird 3 as can be seen by the flags at the top of the bug. Therefore it is already marked as a show-stopper issue. Please only comment on bugs when it adds value to the bug.

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

We've shipped betas without this before, we wouldn't hold b3 for it either.  Sure would be nice to see a fix sooner rather than later, however.

Comment 18

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

I've got some free time while I'm waiting for a patch from Bryan, so if no-one minds, I'll take a look at it and see what it involves.

Comment 19

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

Attachment: A work-in-progress patch, to show the direction I'm going.

There are a number of problems with this patch, but I think it's getting closer, and might just make it in for tomorrow's deadline, if I can get some quick reviews.

The problems I know about are:
1) The icon in the menulist is always a folder, not whatever icon goes with the selected element.
2) There is no way to select a folder that contains sub-folders.
3) The sub-popup menus use a larger font than the first popup menu.

I hope to post another patch later tonight with some of those things fixed, and at that point, I'll mark it r?.  In the meantime, if anyone reading this wants to point out other things for me to fix, I'll be happy to throw them in as well.

Thanks,
Blake.

Comment 20

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

Attachment: The previous patch, with better icons in the menulist, and sub-popups with a smaller font.

I still can't select folders with sub-folders yet, but I thought I should run this by Bryan to make sure he's good with the sub-popup UI, instead of the flat list that TB2 used.

Thanks,
Blake.

Comment 21

[Pardal Freudenthal (:ShareBird)]

diff --git a/mail/base/content/messenger.css b/mail/base/content/messenger.css
--- a/mail/base/content/messenger.css
+++ b/mail/base/content/messenger.css
@@ -189,10 +189,14 @@
   -moz-binding: url("chrome://messenger/content/mailWidgets.xml#searchpopup");
 }
 
-.folderLocationPopup {
-  display: -moz-popup;
-  -moz-binding: url("chrome://messenger/content/mailWidgets.xml#locationpopup");
-  visibility: visible;
+/* Make any sub-menu item of the folderLocationPopup act like a menuitem. */
+.folderLocationPopup menuitem,
+.folderLocationPopup menu{
+  -moz-padding-end: 30px;
+  padding-top: 0px;
+  padding-bottom: 0px;
+  max-width: none;
+  font: message-box;
 }
 
 .folderSummaryPopup


Why are these style rules going to content? Shouldn't they leave on skin?

Comment 22

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

Yes, they should.  I had them in content because that was where I found the .folderLocationPopup selector.  I'll definitely move them for the next patch.

Thanks,
Blake.

Comment 23

[Pardal Freudenthal (:ShareBird)]

(In reply to comment #22)
> Yes, they should.  I had them in content because that was where I found the
> .folderLocationPopup selector.  I'll definitely move them for the next patch.
> 
> Thanks,
> Blake.

Thank you!

Comment 24

[Joey Minta]

Comment on attachment 387124 [details] [diff] [review]
The previous patch, with better icons in the menulist, and sub-popups with a smaller font.

diff --git a/mail/base/content/commandglue.js b/mail/base/content/commandglue.js

I'm a bit confused because the mxr copy of commandglue.js is pretty different from the copy diff'd here...

+        var locationItem = document.getElementById('locationFolders');
+        locationItem.setAttribute("label", msgFolder.prettyName);
You're probably going to need to if-check that this exists, to handle the case where someone has removed it from the toolbar.

-                   height="400"
+                   type="folder"
Consider the fileHereLabel attribute as a cheap way to allow selecting folders with subfolders.

+  var aValue = selectedItem.id;
+  SelectFolder(aValue);
Several comments here:
(1) The a- prefix is reserved for function arguments.
(2) Using item.target._folder will avoid the need to add "id" attributes to everything (see below).
(3) Calling gFolderTreeView.selectFolder() directly will avoid the conversion to/from a URI. (That's all SelectFolder does anyway).
(4) Consider using let instead of var, though I'm not sure what the style-guide rules are on that.

+            // We need to call this, or hasSubFolders will always return false.
+            folder.subFolders;
Really? That's very sad and a scary bug. Can you include a bug # in the comment?

+              node.setAttribute("id", folder.URI);
You can't do this. XBL bindings need to be re-usable to the point where multiple copies can exist in the same document. This addition makes that impossible, since there would end up with multiple nodes having the same id. The above comments should make this easily avoidable though.

Comment 25

[Ronald Killmer]

(In reply to comment #20)
> I still can't select folders with sub-folders yet, but I thought I should run
> this by Bryan to make sure he's good with the sub-popup UI, instead of the flat
> list that TB2 used.
> 
Do You have a screen shot for comparison to the Tb2 list?

Comment 26

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

Attachment: TB2's dropdown, for reference.

Comment 27

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

Attachment: TB3's proposed dropdown, for comparison.

Comment 28

[Ronald Killmer]

OK, that is enlightning. The Tb2 method was OK for short lists of accounts and basic mbox use.  Having several news accounts with lots of group subscriptions made for a very long list needing the scrollbar functions.

I like the Fly-out idea where selecting an item in primary pane exposes the items options. From a users point of view, fly-outs can be annoying if they are too sensitive to a mouse move that slips out of the focus zone. 

Years ago I set a pref in another system that adjusted a time delay that persisted a menu if there was a crack between the menu and a fly-out. For the life of Me I can't find it today on this Vista install. That old issue might be gone, since I see most menu fly-outs slightly overlap the calling menu in Tb2 and Fx2. Yet its a design point to keep in mind to prevent recurrence.

Comment 29

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

Attachment: The previous patch, with jminta's suggestions.

(In reply to comment #28)
> OK, that is enlightning. The Tb2 method was OK for short lists of accounts and
> basic mbox use.  Having several news accounts with lots of group subscriptions
> made for a very long list needing the scrollbar functions.

The thing that really blows up the list for me is the new archive stuff.
Maybe I'm uncommon because I have email going back to 2006 (actually 2000,
but I haven't archived the really old stuff yet), but even with only some
of my old email archived, I've already got 32 items in the folder.

(In reply to comment #21)
> Why are these style rules going to content? Shouldn't they leave on skin?

Fixed.

(In reply to comment #24)
> I'm a bit confused because the mxr copy of commandglue.js is pretty different
> from the copy diff'd here...

Yeah, it was.  I pulled, updated, and fixed the patch.

> +        var locationItem = document.getElementById('locationFolders');
> +        locationItem.setAttribute("label", msgFolder.prettyName);
> You're probably going to need to if-check that this exists, to handle the case
> where someone has removed it from the toolbar.

Done.

> -                   height="400"
> +                   type="folder"
> Consider the fileHereLabel attribute as a cheap way to allow selecting folders
> with subfolders.

Done.  You're right, though, it does look kind of cheap.

> Several comments here:
> (1) The a- prefix is reserved for function arguments.
> (2) Using item.target._folder will avoid the need to add "id" attributes to
> everything (see below).
> (3) Calling gFolderTreeView.selectFolder() directly will avoid the conversion
> to/from a URI. (That's all SelectFolder does anyway).
> (4) Consider using let instead of var, though I'm not sure what the style-guide
> rules are on that.

I've changed the code to:
  gFolderTreeView.selectFolder(item.target._folder);
  FolderPaneSelectionChange();
and thus sidestepped the a-prefix and let/var issues.  :)

> +            // We need to call this, or hasSubFolders will always return
> false.
> +            folder.subFolders;
> Really? That's very sad and a scary bug. Can you include a bug # in the
> comment?

Yeah, it was.  I added a comment that referenced Bug 502900.  (But I don't think this is blocking, because I've got a workaround.  Thank goodness for debugging statements, or I might never have found the workaround.  ;)

> +              node.setAttribute("id", folder.URI);
> You can't do this.
> The above comments should make this easily avoidable though.

Yup.  Fixed.

Thanks,
Blake.

Comment 30

[Joey Minta]

Comment on attachment 387250 [details] [diff] [review]
The previous patch, with jminta's suggestions.

+function LocationSelect(item)
+{
+  gFolderTreeView.selectFolder(item.target._folder);
+  FolderPaneSelectionChange();
I'm not sure I understand why the call to FolderPaneSelectionChange is needed. Should the folderTree's select event be fired anyway? Or is something weird happening there? If you remove that call, then you might as well inline the .selectFolder call. If not, then the function argument should at least be named "event" rather than "item".

diff --git a/suite/themes/modern/global/menu.css b/suite/themes/modern/global/menu.css
I don't think you want this change in the patch.

r=jminta with those nits picked/questions answered.

Comment 31

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 387250 [details] [diff] [review]
The previous patch, with jminta's suggestions.

i like it, nice work

Comment 32

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

Attachment: The previous patch, with jminta's nits fixed.

(In reply to comment #30)
> +function LocationSelect(item)
> If you remove that call, then you might as well inline the
> .selectFolder call.

Inlined.

> diff --git a/suite/themes/modern/global/menu.css
> b/suite/themes/modern/global/menu.css
> I don't think you want this change in the patch.

Nope, fixed.

> r=jminta with those nits picked/questions answered.

Thanks!

jminta mentioned that I should probably get someone else to look at the code, as well, which is why I'm asking for a second review.  I'm also not entirely sure whether the code also needs a super-review, since there is a change to mailnews/base/resources/content/folderWidgets.xml, but it's a workaround for a different bug, and should be removed when that bug is fixed.

Comment 33

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 387294 [details] [diff] [review]
The previous patch, with jminta's nits fixed.

This gets unhappy when it is customized onto / off-of the toolbar during a session.  I suspect this has to do with caching/reuse of menu items, perhaps with some DOM/XBL event involvement.  That's not in your changes, but I don't think we should make the combo-box happier with that problem.

More significantly, you are also running afoul of my changes to the tab logic completely ignoring the existence of the widget.  When you switch tabs, the widget does not update.  You should probably alter FolderPaneSelectionChange to run your logic in the synthetic case, although you could also alter gFolderTreeView.selectFolder.  I would suggest the former; re-arrange the right-click bail check (using isSelected) to be first, then run your code, then bail if it's synthetic, then the call to gFolderDisplay.show if we're still around.

In order to ensure that we avoid breaking the widget again in the future, please add a mozmill unit test.  mail/test/mozmill/folder-display/tests-tabs-simple.js is probably a good test to use as a basis.  I would keep the code that opens a message in a tab and you would want to assert that when the message is displayed that the folder widget gets disabled or hidden or something.

Er, it looks like it does not get hidden/something, so you would probably want to modify mailWindowOverlay.js' mailTabType._setPaneStates to collapsed the widget (if present) when viewing a message.  I would just duplicate the code currently used for "search-container" and "mailviews-container".

Also, you should use double-quotes in your JS code unless you would get into escaping issues.  It's part of the JS code guide (it's a secret to everybody), but save for preferring single quotes myself, I generally agree with it:
https://developer.mozilla.org/en/Javascript_Style_Guide

Comment 34

[Andrew Sutherland [:asuth] (he/him)]

(In reply to comment #33)
> but save for preferring single quotes myself, I generally agree with it:

Er, but I do obey the double quotes rule.

Comment 37

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

Attachment: A WIP patch, to ask for help with tests.

(In reply to comment #33)
> (From update of attachment 387294 [details] [diff] [review])
> This gets unhappy when it is customized onto / off-of the toolbar during a
> session.  I suspect this has to do with caching/reuse of menu items, perhaps
> with some DOM/XBL event involvement.

Could be.  It seems that we were creating three copies of the toolbar item when we dragged it and dropped it.  I'm now calling "this._teardown();" before I re-build the folder list, which doesn't seem like the best idea, but does work.

I also don't entirely understand how we can get a new node with all the children copied over, but without the _initialized attribute copied over.

> That's not in your changes, but I don't
> think we should make the combo-box happier with that problem.

I'm not quite sure what you mean here.  Do you mean that I should try to get the element to not be constructed three times, or to get the construction to also copy over the value of the _initialized?

> More significantly, you are also running afoul of my changes to the tab logic
> completely ignoring the existence of the widget. […] re-arrange the
> right-click bail check (using isSelected) to be first, then run your code,
> then bail if it's synthetic, then the call to gFolderDisplay.show if we're
> still around.

Done.

> In order to ensure that we avoid breaking the widget again in the future,
> please add a mozmill unit test. 
> mail/test/mozmill/folder-display/tests-tabs-simple.js is probably a good test
> to use as a basis.  I would keep the code that opens a message in a tab and
> you would want to assert that when the message is displayed that the folder
> widget gets disabled or hidden or something.

Done-ish.  I need some help with the add/remove test, which is why I'm posting this patch.  (The tab-switching test works just fine, though.  :)

> Er, it looks like it does not get hidden/something, so you would probably want
> to modify mailWindowOverlay.js' mailTabType._setPaneStates to collapsed the
> widget (if present) when viewing a message.  I would just duplicate the code
> currently used for "search-container" and "mailviews-container".

Done.

> Also, you should use double-quotes in your JS code unless you would get into
> escaping issues.  It's part of the JS code guide (it's a secret to everybody),
> but save for preferring single quotes myself, I generally agree with it:
> https://developer.mozilla.org/en/Javascript_Style_Guide

Yeah, I'm not quite sure how those got in there.  Anyways, they're removed.

So, as you can see if you run "make SOLO_TEST=folder-display/test-folder-toolbar.js mozmill-one", I'm getting a failure on "test_add_folder_toolbar".  I can't quite figure out why that's wrong, so if you had some time to give me a hand, I'ld really appreciate it.

Thanks,
Blake.

Comment 38

[Andrew Sutherland [:asuth] (he/him)]

Attachment: modifications to attachment 392836 to make the test work

I finally broke down and read the toolbar customization code after also hitting a series of brick walls about synthesizing drag and drop notifications.  It turns out we can just use currentSet on the toolbar to accomplish our goals and avoid the whole ugly business of the customization dialog.

The attached patch changes things to do that.  It also tries to work around a particularly nasty problem...

But first, something I didn't deal with.  The widget should be hidden if the folder pane is deemed illegal in _setPaneStates too.  Currently the widget still gets shown (and can change things!) when a gloda search tab is visible.

Now the horrible things:

- bug 230086 and bug 296474 suggest that removeChild does not trigger XBL destructors to fire.  Which is why I made the change in _tearDown so that we force our children to be torn down before we remove them.  That way they have a chance to remove their listeners.

- In the same vein, if you customize the toolbar with the widget and drag it back and forth, you will eventually see horrible errors (in a debug build) about XPConnect being called on a scope without Components present.  I believe what is happening is that we are leaking a listener.  Although the listener survives, most of his world gets destroyed, leaving him in a screwed up scope.  I'm not sure we actually need to fix this, as it seems hard/annoying and unlikely to crop up all that often.  Although we should probably log a bug.

- The greater confusion and concern is that the code in the event notifications that checks if "this._menu._teardown" exists pretty much fails all the time and it seems like a great mystery to me as to why it is happening.  dumps (removed in the patch) suggest that this._menu._teardown is valid when it is registered as a listener, and a dump in the notification suggests that we are dealing with the same XUL object and wrapper (as this._menu) when the notification actually fires.

- I think we may have some other kind of listener leak on our hands, which is not terribly surprising given the mysterious disappearance of _teardown.  Unfortunately, I may have caused this as a result of debugging code as I had accidentally removed one of the calls to removelistener.  I guess, just keep an eye out for this.  The only real good way to keep an eye on this is to break on "nsMsgMailSession::AddFolderListener" and take a look "*mListeners.mArray.mHdr" while you are in there.  You can compel the widget to be instantiated by right-click and choosing to add a new folder.  The new folder dialog uses the widget.  It is safer than using the folder-picker combobox, at least on linux, because GrabKeys happens for popups and then your X session becomes useless.

I don't think the last 2 probably are immediately addressable, and they certainly are not new, but if you have any ideas, I'd be glad to hear them.  Otherwise they too should probably get a bug.

Comment 39

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

Status Note: I do have this on my to-do list, and haven't forgotten about it, but I put it behind the stuff that has an l10n-impact, because in theory, I could slide it in after the string freeze, if necessary.

Thanks,
Blake.

Comment 40

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

Attachment: The previous two patches combined, with the requested fix.

(In reply to comment #38)
> But first, something I didn't deal with.  The widget should be hidden if the
> folder pane is deemed illegal in _setPaneStates too.  Currently the widget
> still gets shown (and can change things!) when a gloda search tab is visible.

Fixed.

> Now the horrible things:
> - In the same vein, if you customize the toolbar with the widget and drag it
> back and forth, you will eventually see horrible errors (in a debug build)
> about XPConnect being called on a scope without Components present.
> I'm not sure we actually need to fix this, as it seems hard/annoying and
> unlikely to crop up all that often.  Although we should probably log a bug.

Filed as Bug 514448.

> - The check if "this._menu._teardown" exists pretty much fails all the time
> - I think we may have some other kind of listener leak on our hands.
> I don't think the last 2 probably are immediately addressable, and they
> certainly are not new, but if you have any ideas, I'd be glad to hear them. 
> Otherwise they too should probably get a bug.

No, nothing springs to mind.

Filed as Bug 514445.

Thanks,
Blake.

Comment 41

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 398410 [details] [diff] [review]
The previous two patches combined, with the requested fix.

I'm getting reliable mozmill test failures:

TEST-UNEXPECTED-FAIL |  test_delete_last_message_closes_message_displays
  EXCEPTION: There should only be one tab left!
    at: test-deletion-with-multiple-displays.js line 253
       Error("There should only be one tab left!")  0
       test_delete_last_message_closes_message_displays() test-deletion-with-multiple-displays.js 253
            frame.js 452
            frame.js 504
            frame.js 546
            frame.js 400
            frame.js 551
            server.js 164
            server.js 168

TEST-UNEXPECTED-FAIL |  test_message_pane_persistence_generally_works
  EXCEPTION: The message display thinks it is visible, but it should not!
    at: test-message-pane-visibility.js line 103
       Error("The message display thinks it is visible, but it should not!")  0
       assert_message_pane_hidden() test-message-pane-visibility.js 103
       verifyTabs([object Array]) test-message-pane-visibility.js 301
       test_message_pane_persistence_generally_works() test-message-pane-visibility.js 313
            frame.js 452
            frame.js 504
            frame.js 546
            frame.js 400
            frame.js 551
            server.js 164
            server.js 168

No smoking gun jumps out at me.

Comment 42

[Magnus Melin [:mkmelin]]

Blake, also see http://hg.mozilla.org/comm-central/rev/c4efea11dfa2#l1.7, get rid of that comment if it's unneeded when this is done, and toolbar customization is working with this widget again.

Comment 43

[David :Bienvenu]

Comment on attachment 398410 [details] [diff] [review]
The previous two patches combined, with the requested fix.

clearing review request based on asuth's -

Comment 44

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

Attachment: The previous patch, with fixed tests and comment.

(In reply to comment #41)
> I'm getting reliable mozmill test failures:
> TEST-UNEXPECTED-FAIL |  test_delete_last_message_closes_message_displays
> TEST-UNEXPECTED-FAIL |  test_message_pane_persistence_generally_works

I wasn't cleaning up after my tests.  Fixed, and thanks for catching it!

(In reply to comment #42)
> Blake, also see http://hg.mozilla.org/comm-central/rev/c4efea11dfa2#l1.7, get
> rid of that comment if it's unneeded when this is done, and toolbar
> customization is working with this widget again.

Gotten rid of.  (Actually, replaced with code that does the right thing.)

Thanks,
Blake.

Comment 45

[David :Bienvenu]

Comment on attachment 399152 [details] [diff] [review]
The previous patch, with fixed tests and comment.

it seems this patch has bit-rotted - do you have a current version?

Comment 46

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

Attachment: The previous patch, un-rotted.

(In reply to comment #45)
> (From update of attachment 399152 [details] [diff] [review])
> it seems this patch has bit-rotted - do you have a current version?

Sure.  How's this one?  (pbranch FTW, if it works.)

Thanks,
Blake.

Comment 47

[David :Bienvenu]

that applies, thx. This patch works from what I can tell, but the folder pane doesn't scroll to show the selected folder, which makes it a bit confusing...

Comment 48

[David :Bienvenu]

the test failed for me with these errors, maybe something wrong with my mozmill setup, though I have updated it to 1.2

TEST-UNEXPECTED-FAIL |  test_add_folder_toolbar
  EXCEPTION: aToolbarElement is null
    at: test-folder-display-helpers.js line 1369
       add_to_toolbar(null,"folder-location-container") test-folder-display-hel
ers.js 1369
       test_add_folder_toolbar() test-folder-toolbar.js 77
            frame.js 452
            frame.js 504
            frame.js 546
            frame.js 409
            frame.js 557
            server.js 164
            server.js 168
TEST-UNEXPECTED-FAIL |  test_folder_toolbar_shows_correct_item
  EXCEPTION: aToolbarElement is null
    at: test-folder-display-helpers.js line 1369
       add_to_toolbar(null,"folder-location-container") test-folder-display-hel
ers.js 1369
       test_folder_toolbar_shows_correct_item() test-folder-toolbar.js 88
            frame.js 452
            frame.js 504
            frame.js 546
            frame.js 409
            frame.js 557
            server.js 164
            server.js 168
TEST-UNEXPECTED-FAIL |  test_folder_toolbar_disappears_on_message_tab
  EXCEPTION: aToolbarElement is null
    at: test-folder-display-helpers.js line 1369
       add_to_toolbar(null,"folder-location-container") test-folder-display-hel
ers.js 1369
       test_folder_toolbar_disappears_on_message_tab() test-folder-toolbar.js 1
3
            frame.js 452
            frame.js 504
            frame.js 546
            frame.js 409
            frame.js 557
            server.js 164
            server.js 168
TEST-UNEXPECTED-FAIL |  test_remove_folder_toolbar
  EXCEPTION: aToolbarElement is null
    at: test-folder-display-helpers.js line 1386
       remove_from_toolbar(null,"folder-location-container") test-folder-displa
-helpers.js 1386
       test_remove_folder_toolbar() test-folder-toolbar.js 145
            frame.js 452
            frame.js 504
            frame.js 546
            frame.js 409
            frame.js 557
            server.js 164
            server.js 168
make: *** [mozmill-one] Error 1

Comment 49

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

Attachment: The previous patch, with the tests working, and the folder pane scrolling.

It also scrolls on "n" if the next unread message is in a folder that you can't see.

Thanks,
Blake.

Comment 50

[David :Bienvenu]

yay, mozmill tests much happier.

Comment 51

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 399942 [details] [diff] [review]
The previous patch, with the tests working, and the folder pane scrolling.

Thanks for the folder pane scrolling fix too.  Very nice to have that :)

Comment 52

[David :Bienvenu]

Comment on attachment 399942 [details] [diff] [review]
The previous patch, with the tests working, and the folder pane scrolling.

yes, thx for fixing that!

Comment 53

[David :Bienvenu]

I think the relnote keyword should be removed when this patch is pushed.

Comment 54

[Rain [:rain1] (inactive)]

http://hg.mozilla.org/comm-central/rev/a06095d0da3d

Comment 55

[Wayne Mery (:wsmwk)]

beautiful.  "n" also works
v.fixed 20090912 nightly