Closed Bug 978592 Opened 6 years ago Closed 6 years ago

"Favorite Folders" view shouldn't sort in a hierarchy?

Categories

(Thunderbird :: Folder and Message Lists, enhancement, P1)

enhancement

Tracking

(thunderbird31+ fixed)

RESOLVED FIXED
Thunderbird 32.0
Tracking Status
thunderbird31 + fixed

People

(Reporter: jik, Assigned: aceman)

References

Details

Attachments

(4 files, 8 obsolete files)

In today's trunk, the "Favorite Folders" view is suddenly hierarchical. I don't know whether this is intentional or unintentional. I hope the latter. It uses the screen real estate in credibly poorly. For Example, if I have three different accounts with favorite folders in them, then I now end up with three different unnecessary lines taken up in the favorite folder list, one for each of the account names; these were previously either not visible at all or visible as " - AccountName" after the folder name, which was far more efficient of screen real estate.

Looking at my favorite folder list right now in this new configuration, there are six lines in it that weren't there befoer tha add no useful information and just force me to scroll when otherwise I wouldn't have to.

This is in my opinion an awful UX change and I hope it gets put back the way it was before.
It's intentional and probably won't be changed back; seeing the hierarchy is actually pretty useful because it groups things in a sane way. I for one never used the Favorite folders view because it was a horrible mess and just made it *harder* for me to find the folder I wanted, rather than easier.

That said, there's nothing stopping you from making your own version of the favorite folders view. Here's an example add-on that does it: https://addons.mozilla.org/en-US/thunderbird/addon/advanced-unread-folders/

See also bug 533775.
Was any attempt made to survey a large number of users of the application to find out what the preferences of the people who actually use the Favorite Folders view would be?

Was any A/B UX testing done of the old and proposed new format to find out which is actually more usable for people?

Or is this yet another instance of a few people deciding that a new way would be better and going ahead and changing it without making any effort to ascertain whether their views are shared by the majority of users?

If the old format for the view was already implemented, then why in heaven's name would you throw it away, as opposed to, say, adding a preference to control whether to display the view the old way or the new way?

I tire of this "it's OK that we stripped this functionality that people like out of TB, because somebody can just write an add-on to put it back" trope. It's annoying and hostile to users. I don't have time to write and maintain a separate TB add-on for every piece of functionality that y'all decide on a whim to rip out of the application. If "it's OK, we can make it an add-on" is a valid argument, then why didn't you make the NEW format an add-on and leave the old format in place for those of us who have been using it, and like it, for many years?

I have a lot of Favorite folders. I fight on a daily basis to make the list small enough that it actually fits vertically without scrolling, so I can get to all the favorites easily. That is, after all, the point of having favorites. With the new format, it will be simply impossible for me to fit all of my favorites in the window. This is a big step backward, not forward, for me. I am confident that I won't be the only one who feels this way.

Would you at least accept a patch to make the old and new behavior switchable with a preference?
If you read the bug I referenced, you'll see why this was changed. The sorting for "favorite folders" was basically total nonsense (it mixed folders together with no regard for the account they were originally from). This was fixed by way of providing a hierarchical view for favorite folders to make it clear which folder belonged to which account. Doing it that way is more ux-consistent with the other folder views, and helps ux-discovery by not requiring users to remember which account holds a given folder; the "- Account Name" suffix would only show if you had duplicate folder names.

I could see an argument to elide non-favorited folders that have favorited subfolders (anyone who wanted to see the parent folder could just favorite it, after all), but I think we should always show the root account.

In fact, with a little adjustment to your routine, this change could make it *easier* to fit your favorite folders in one screen, not harder. I tend to group my folders fairly hierarchically so that related folders all have a common parent. With the new Favorites view, I can collapse the parent folder and just watch the parent's unread count to know if I need to drill down into the subfolders.
I understand the rationale for changing it. I disagree with it. Don't assume that if someone disagrees with you the only possible explanation is because they don't know the same facts you do. People can start with the same facts and arrive at different conclusions.

I had no trouble remembering which folders belonged to which accounts. The "- Account Name" suffix was more than sufficient for me when there was confusion.

The single biggest priority for me was screen space, and you've shot that to hell. I'm sure I'm not the only one.

I repeat my previous question -- was any effort made to actually survey users or do A/B testing to find out what the majority of users want, or was this just a few people deciding their way is "obviously" better so there's no need to make any effort to find out how people actually use the functionality before changing it?

I don't use Favorite Folders for unread messages; I use them for filing messages once I'm done with them. So your "little adjustment to [my] routine" is irrelevant to me and won't help me at all. That just goes to show how little understanding there was of how people actually use this view before changing it.

I repeat my earlier questions: (1) why wasn't the old view preserved for people who prefer it; (2) why wasn't the new view put into an add-on if that's an acceptable answer to people who want different functionality; (3) would you accept a patch to make the old behavior toggleable with a preference, or not?

(And that's leaving aside the fact that I don't think you should have changed it at all because I'm not at all convinced you're correct that the majority of users will prefer the new UX to the old one, but I honestly have no hope of convincing you of that, despite the fact that you don't appear to have any data supporting your position, so I'm not going to try.)
I'm sorry I'm copping such an attitude about this. I know the folks who work on Thunderbird put in a lot of hours and don't get paid for it, and I'm grateful for it. But it's just so darn frustrating to see stuff like this happen over and over again, especially when, frankly, there are hundreds if not thousands of open Thunderbird bugs waiting for someone to fix. I cannot help but think that it would be a better use of people's time to work on those than to make minor, incremental, questionable UX changes which are going to infuriate many users.
While I can understand the desire to keep things as they were in order to support people's existing use cases, I find that pretty often, the use cases exist because Thunderbird is broken. There really should be better ways of filing messages when you're done with them, but the Favorite folders view is at best just a workaround for Thunderbird's deficiencies in that area.

I'd be interested in seeing if we could come up with a new way to handle filing of messages so that the Favorites view wasn't necessary for that purpose. (I also wouldn't be *entirely* opposed to backing out bug 533775 until after TB31 to give us more time to provide alternatives.)

It would be nice if we could do more user studies, but to be perfectly honest, I don't think anyone has the resources to do so right now.
One possible improvement to filing messages would be to enhance the "File" toolbar button so that you could customize what was shown in it, or even just to list the "Favorite" folders in there somehow. That would make it easy to access your list of folders, and reduce the need to drag-and-drop, which can be a bit fiddly at times.
(In reply to Jim Porter (:squib) from comment #6)
> (I also wouldn't be *entirely* opposed to backing out bug 533775
> until after TB31 to give us more time to provide alternatives.)
> 
> It would be nice if we could do more user studies, but to be perfectly
> honest, I don't think anyone has the resources to do so right now.

These two points may seem disparate, but they are actually related.

Implement the new display format but leave the old format code in place, toggled by a preference. The first time the folder list is displayed to a user with the new logic, launch a balloon tip over the list pointing out that it was changed and offering a "click here to change it back, or if you want to try it out for a while, we'll ask you again in a week," or something like that. If the user doesn't change it back, then pop up the balloon tip again a week later offering one last chance to change it back. Then use the infrastructure that is already built into TB for getting user experience feedback to find out how many people leave the new view in place and how many change it back.

If the vast majority of people keep the new format, then you can justify eventually excising the old format from the code base and telling the few people who preferred it to just live with it. But if a good chunk of people revert to the old format, then that's justification for making it configurable with a visible preference rather than junking it.

This approach is not specific to this particular UX element; something very much like this could be used for every UX element where changes are proposed. It's an easy way to collect real-world feedback from a ton of people about which UX is preferred.
Yeah, sorry for the sudden change. Several TB reviewers liked the change so bug 533775 was implemented.
I would be fine with a pref (or even offering both versions directly in the View -> Folders menu).

(In reply to Jim Porter (:squib) from comment #7)
> One possible improvement to filing messages would be to enhance the "File"
> toolbar button so that you could customize what was shown in it, or even
> just to list the "Favorite" folders in there somehow. That would make it
> easy to access your list of folders, and reduce the need to drag-and-drop,
> which can be a bit fiddly at times.

Good news, I have this on my plate in bug 546722 :)
I'm not opposed to creating a (non-default) about:config pref that restores the sorting back to the previous behavior (as long as writing the code is simple), but I definitely do not think we should revert back entirely or add it to the preferences window (I won't completely write out the idea of adding it to the preference window yet, if there is evidence to suggest it's necessary). Most users would want the more organized structure, especially with several accounts. Example below.

One of the main reasons this was added is for people with multiple accounts. For example, say you marked Inbox as a favorite in all your accounts, and two of the accounts you marked Sent as favorite. With the old behavior, the Favorites view would display something like:

Inbox
Inbox
Sent
Inbox
Sent

That's not helpful at all, and quite confusing. The new behavior fixes this, as you now get:

Account Name
  Inbox
Other Account Name
  Inbox
  Sent
Third Account Name
  Inbox
  Sent

Which is substantially better, as you can follow where everything is coming from. You do lose some vertical space, but I think it's for the better.

If implementing the pref is difficult or complicated, I do not think it is worth adding for solely saving space (That seems to be the only reason this changed is not preferred). If it's difficult, I think this would be better suited for a "compact folders" add-on. Otherwise, go ahead with the pref.
Summary: "Favorite Folders" view is broken → "Favorite Folders" view shouldn't sort in a hierarchy?
Squib: The bug you linked was about the order, not about the display…
I don't think Jonathan particularly wants the previous order, just a display that doesn't show accounts, and has the folders on the left.

Josiah: How do you feel about a pref which keeps the new sorting order, but instead displaying your example as:

Inbox - Account Name
Inbox - Other Account Name
Sent - Other Account Name
Inbox - Third Account Name
Sent - Third Account Name

?

[Note 1: We would only display the account name if there were duplicated folder names!  So if you only had one "Sent" folder, we would only display "Sent".]

[Note 2: What does everyone think about displaying it as:

Account Name
Inbox
Other Account Name
Inbox
Sent
Third Account Name
Inbox
Sent

instead ?]

Thanks,
Blake.
We only have 5 modes in the View -> Folders menu. We could just add this as a new mode "Favorite - compact" in there. Hidding that mode based on a pref would be fine.

Modifying the existing mode to have different behaviour according to some pref may become a mess. Also code expecting some behaviour (layout of the folders, e.g. tests) would also need to check the pref.
Adding the new mode is not hard, just copy the code from patch in bug 533775.
I can even do it if it is agreed on.

(In reply to Blake Winton (:bwinton) from comment #11)
> Squib: The bug you linked was about the order, not about the display…
That bug also changed the display (now it is in a hierarchy).

> Josiah: How do you feel about a pref which keeps the new sorting order, but
> instead displaying your example as:
> 
> Inbox - Account Name
> Inbox - Other Account Name
> Sent - Other Account Name
> Inbox - Third Account Name
> Sent - Third Account Name

This is basicaly the old mode, just keeping the new order.

> [Note 1: We would only display the account name if there were duplicated
> folder names!  So if you only had one "Sent" folder, we would only display
> "Sent".]
> 
> [Note 2: What does everyone think about displaying it as:
> 
> Account Name
> Inbox
> Other Account Name
> Inbox
> Sent
> Third Account Name
> Inbox
> Sent

What is different here from the current state (and what Josiah describes)? Just indentation?
(In reply to Blake Winton (:bwinton) from comment #11)
> 
> Josiah: How do you feel about a pref which keeps the new sorting order, but
> instead displaying your example as:
> 
> Inbox - Account Name
> Inbox - Other Account Name
> Sent - Other Account Name
> Inbox - Third Account Name
> Sent - Third Account Name
> 
> ?

Great idea. Sounds good to me, but I like aceman's idea of making this a new view mode instead of changing the existing Favorites. We may not even need to make it a pref, just add it to the list.

> 
> [Note 1: We would only display the account name if there were duplicated
> folder names!  So if you only had one "Sent" folder, we would only display
> "Sent".]

This might be okay with a few accounts, but I still think it might get confusing if you have several folders from several accounts marked as favorites. It still might not be clear which Sent folder it is.

> 
> [Note 2: What does everyone think about displaying it as:
> 
> Account Name
> Inbox
> Other Account Name
> Inbox
> Sent
> Third Account Name
> Inbox
> Sent
> 
> instead ?]

This does help with horizontal space, but I don't think it's as good a solution as the compact one you suggested above, as vertical space is still reduced with this solution.
Two points for the record:

1) As others have noted, Comment 10's description of the old view format is incorrect, since the old format would append the account name to the folder name if the (case-insensitive) folder name was not unique in the favorites view. This was critical functionality.

2) I STRONGLY PREFER the "compact" view we're talking about to be sorted alphabetically first, not by account first. It sounds most of you commenting on what the compact view should look like don't actually prefer it to the new view, so please listen to someone who DOES prefer the old view -- alphabetical is much more useful. I do not want to have to think about the arbitrary order in which Thunderbird sorts my accounts every time I need to find a folder in the view. I just want to scan down the list in alphabetical order.
Blocks: 533775
Duplicate of this bug: 985435
I should look at this soon, before TB31 or many users will be surprised.
I just copy back the code before the changes under some new "* - compact" named modes, like this:
All
Unified
Favorite
Favorite - compact
Unread
Unread - compact
Recent

Recent is not hierarchical yet. So this would add 2 modes to the current 5, which seems fine to me.
If you'd like 'flat' instead of 'compact' for shorter menuitems, just speak up :)
Assignee: nobody → acelists
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Priority: -- → P1
Hardware: x86_64 → All
I don't think it's a good idea to add those modes, as it's really cluttering.
There are (it seems)  some edge case users that it would be useful for, but as an application we need to choose what's best for the majority. Folder modes can easily be added by an extension, and in this case I suppose the extension could almost just copy paste the code we had before.
(In reply to Magnus Melin from comment #17)
> ... as an application we need to choose what's best for the majority.

I want to reiterate my earlier statement that I see no evidence that's what was done here.

Guessing what users want is not actually the same as actually collecting real data from users about what they want.
I don't think there's any other reasonable way to "collect real data" than to use an assumed improvement in alpha/beta testing. So far we only have this bug and bug 985435 (which is unclear to me if it's meant as an actual complaint or just an observation)
(In reply to Magnus Melin from comment #19)
> I don't think there's any other reasonable way to "collect real data" than
> to use an assumed improvement in alpha/beta testing.

comment #8
As I said we only have 5 modes so the menu is quite under-utilized. Adding 2 more doesn't seem bad to me. And also it does not cost much code wise (2 short functions).
I agree we do not have much user input so we do not know which version they want. We just have opinions from ui people and reviewers that they like the new version better. But if it does cost us almost nothing in this case, I vote for having more options in TB than less but randomly chosen ones.
Attached patch patch (obsolete) β€” β€” Splinter Review
So this is what I have in mind.
If somebody really wants I can hide the flat modes from the menu in onpopupshowing=InitViewFolderViewsMenu() based on some pref. But I would be for exposing all modes so that users have all the options by default.
Attachment #8402931 - Flags: ui-review?(josiah)
Status: NEW → ASSIGNED
Comment on attachment 8402931 [details] [diff] [review]
patch

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

This looks good to me. The only change I would make is to swap the word "flat" for "compact", since flat doesn't describe well enough what the mode does.

And yes, let's not hide the new modes.
Attachment #8402931 - Flags: ui-review?(josiah) → ui-review+
Attached patch patch v2 (obsolete) β€” β€” Splinter Review
Thanks.
Attachment #8402931 - Attachment is obsolete: true
Attachment #8405632 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8405632 [details] [diff] [review]
patch v2

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

Sorry, I really think this is material for an extension. You can try to get someone else to review it if you want.

It's not about not having the space, but to the user it looks like we just couldn't make up our minds about what it should look like, and that is our responsibility.
I also find it strange that now that Recent is still compact, you don't get to choose Compact there even if it is. If we do take this we should make sure all the special modes are there for consistency. If we take it I'd also be happier with just a checkbox similar to View > Threads > Ignore threads than to have them as separate UI modes.

::: mail/base/content/folderPane.js
@@ +106,5 @@
> +   * @param aProperty  The changed property string.
> +   * @param aOld       The old value of the property.
> +   * @param aNew       The new value of the property.
> +   */
> +  handleChangedIntProperty: function IFolderTreeMode_handleChangedIntProperty(aItem, aProperty, aOld, aNew) {

I hear function naming is not not useful anymore (IFolderTreeMode_handleChangedIntProperty)
Attachment #8405632 - Flags: review?(mkmelin+mozilla)
(In reply to Magnus Melin from comment #25)
> Sorry, I really think this is material for an extension. You can try to get
> someone else to review it if you want.
> 
> It's not about not having the space, but to the user it looks like we just
> couldn't make up our minds about what it should look like, and that is our
> responsibility.
Or that we just offer more options. I worry that user outrage about removed functionality may be a bigger than their thinking "we couldn't make up our minds". But we will see. The patch is here in case we need to push it quickly to a point release :)

> I also find it strange that now that Recent is still compact, you don't get
> to choose Compact there even if it is. If we do take this we should make
> sure all the special modes are there for consistency.
We can rename it to Recent-compact easily. If somebody implements Recent (non-compact) then of course it would be good to have both. But we are not there yet.

> If we take it I'd also
> be happier with just a checkbox similar to View > Threads > Ignore threads
> than to have them as separate UI modes.
I can try this checkbox if Josiah/bwinton likes that. So it would be:
All
Unified
Unread
Favorite
Recent
--------
[] Compact

> > +  handleChangedIntProperty: function IFolderTreeMode_handleChangedIntProperty(aItem, aProperty, aOld, aNew) {
> I hear function naming is not not useful anymore
> (IFolderTreeMode_handleChangedIntProperty)

So what should it be now? 
handleChangedIntProperty: function (aItem, aProperty, aOld, aNew) {} ?
Flags: needinfo?(josiah)
(In reply to :aceman from comment #26)
> I can try this checkbox if Josiah/bwinton likes that. So it would be:
> All
> Unified
> Unread
> Favorite
> Recent
> --------
> [] Compact
> 

Yes, that would be great. Much better. Of course, if we do that then users will expect ALL modes to feature that ability. Can that be done?
Flags: needinfo?(josiah)
I can't do both versions for all modes. But we can disable or hide the option for modes having only one version.
Attached patch patch v3 (obsolete) β€” β€” Splinter Review
OK, this reworks it with the "Compact" toggle in the Folder menu.
Attachment #8405632 - Attachment is obsolete: true
Attachment #8409489 - Flags: ui-review?(josiah)
Attachment #8409489 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8409489 [details] [diff] [review]
patch v3

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

All in all, this is much better if you do think we should have it in core. With this at least should usually end up with reasonable defaults.

At least it would let us progress with migrating Recent mode to hierarcies too.
(Unless we plan to add the hierarchy shortly, Reecent should be disabled + compact *checked* atm)

::: mail/base/content/folderPane.js
@@ +363,5 @@
>    /**
> +   * Name of the mode without the _compact suffix, used e.g. in the menulists.
> +   */
> +  publicMode: function() {
> +    return this.mode.replace("_compact", "");

probably safer to do this.mode.replace(/_compact$/, "")

@@ +1384,5 @@
> +     */
> +    unread_compact: {
> +      __proto__: IFolderTreeMode,
> +
> +      generateMap: function (ftv) {

here and other places, function(foo), without the space please

@@ +1451,5 @@
>  
>          return favRootFolders;
> +      },
> +
> +      handleChangedIntProperty: function ftv_favorite_handleChangedIntProperty(aItem, aProperty, aOld, aNew) {

ftv_favorite_handleChangedIntProperty not needed

@@ +1482,5 @@
> +        // duplicated folder names.
> +        let uniqueNames = new Set(); // set of folder names seen at least once
> +        let dupeNames = new Set(); // set of folders seen at least twice
> +        for (let item of faves) {
> +          let name = item._folder.abbreviatedName.toLocaleLowerCase();

shouldn't this use the real case?

::: mail/base/content/mailWindowOverlay.xul
@@ +1210,5 @@
>                        label="&recentFolders.label;"
>                        type="radio"
>                        name="viewmessages"
>                        oncommand="gFolderTreeView.mode = this.value;"/>
> +            <menuseparator/>

add an id

@@ +2475,5 @@
>                        accesskey="&recentFolders.accesskey;"
>                        type="radio" name="viewmessages"
>                        oncommand="gFolderTreeView.mode = this.value;"/>
> +            <menuseparator/>
> +            <menuitem id="menu_compactFolderMode"

add an id
Attachment #8409489 - Flags: review?(mkmelin+mozilla) → review+
(In reply to Magnus Melin from comment #30)
> All in all, this is much better if you do think we should have it in core.
> With this at least should usually end up with reasonable defaults.
> 
> At least it would let us progress with migrating Recent mode to hierarcies
> too.
> (Unless we plan to add the hierarchy shortly, Reecent should be disabled +
> compact *checked* atm)
I split Recent into a new bug because I didn't see an easy way how to do it hierarchically right now (I think we'd need a global array holding the recent folders). So it will not be done shortly.

Forcing a *checked* "Compact" for Recent is a good idea, I just need to rework the patch. It could be useful for other modes too (e.g. added by extensions).

> @@ +1482,5 @@
> > +        // duplicated folder names.
> > +        let uniqueNames = new Set(); // set of folder names seen at least once
> > +        let dupeNames = new Set(); // set of folders seen at least twice
> > +        for (let item of faves) {
> > +          let name = item._folder.abbreviatedName.toLocaleLowerCase();
> 
> shouldn't this use the real case?
This just puts back code removed in bug 533775. With some tweaks, but this line seems unchanged. We just look for dopes in lowercase. So if we have Inbox and inbox, it comes out as duped and gets account name appended.
Attached patch patch v4 (obsolete) β€” β€” Splinter Review
OK, this is a cleaned up and commented patch. I'd rather get this into core. If it is considered confusing, we can trivially hide the "compact view" option later in the release cycle. That wouldn't be possible the other way round.
Attachment #8409489 - Attachment is obsolete: true
Attachment #8409489 - Flags: ui-review?(josiah)
Attachment #8412124 - Flags: ui-review?(bwinton)
Attachment #8412124 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8412124 [details] [diff] [review]
patch v4

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

::: mail/locales/en-US/chrome/messenger/messenger.dtd
@@ +159,5 @@
>  <!ENTITY favoriteFolders.label "Favorite">
>  <!ENTITY favoriteFolders.accesskey "F">
>  <!ENTITY recentFolders.label "Recent">
>  <!ENTITY recentFolders.accesskey "R">
> +<!ENTITY compactVersion.label "Compact view">

view with capital V

::: mail/locales/en-US/chrome/messenger/messenger.properties
@@ +503,5 @@
>  
>  # Folder Pane Header Title Strings
>  folderPaneModeHeader_all=All Folders
>  folderPaneModeHeader_unread=Unread Folders
> +folderPaneModeHeader_unread_compact=Unread Folders - compact

- Compact View 
I think. same below
Attachment #8412124 - Flags: review?(mkmelin+mozilla) → review+
Attached patch patch v5 (obsolete) β€” β€” Splinter Review
Thanks. So this seems to have all the needed reviews.

But I see mkmelin was not persuaded by this patch.
Does anybody want to veto this patch going into TB31 tomorrow?
Attachment #8412124 - Attachment is obsolete: true
Attachment #8412124 - Flags: ui-review?(bwinton)
Attachment #8413422 - Flags: ui-review?(josiah)
Attachment #8413422 - Flags: ui-review?(bwinton)
Comment on attachment 8413422 [details] [diff] [review]
patch v5

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

Umm, so this patch now doesn't allow me to disable the checkmark on the Compact view item. (OS X 10.9). We definitely can't land it in 31 like this.
Attachment #8413422 - Flags: ui-review?(josiah) → ui-review-
Nothing in the error console. So I'm not sure what's going on. But it was working in the patch I tried a few days ago.
What mode wasn't working?
I can't turn off compact for any view mode that allows it. It always stays checked.
Thanks, I can see the problem on the appmenu version of the toggle. I am working on it.
Please try this.
Attachment #8413422 - Attachment is obsolete: true
Attachment #8413422 - Flags: ui-review?(bwinton)
Attachment #8413903 - Flags: ui-review?(josiah)
Hmm... Nope, same issue.
Have you actually rebuilt after applying the patch? The xul file is preprocessed.
Ok, given this implements a get-out for a potentially impacting user change in 31, and we've had no feedback other than this bug so far, rather than back out bug 533775, I've taken the decision to land this patch with pending-ui-review.

If this hadn't affected strings, I would have been happier to wait.

If the changes to get the completed ui-review are too great, then we'll back it out from aurora, and risk going forward (or back out 533775 as well).

Note that I also re-added the removed string so that if we want to backout, we can still use the original strings.

https://hg.mozilla.org/comm-central/rev/c23b118f9daf
https://hg.mozilla.org/comm-central/rev/6d6550f3e91a
Thanks, I'll take care to get the needed fix (if any) into aurora too.
aceman: This broke mozmill tests I believe. Also I was seeing the same issue as Josiah.
I personally really think this stuff should be backed out and re-landed with tests to keep this from happening. This is clearly too fragile currently. But that's just my opinion.
Please give me time at least today. I'll add tests and fix this one reported.
Attached patch fix and add tests (obsolete) β€” β€” Splinter Review
Aryx, can I get a try run with this? All tests and platforms.
Attachment #8415604 - Flags: feedback?(archaeopteryx)
Attachment #8413903 - Attachment description: patch v5.1 → patch v5.1 [checked in in comment 43]
Attachment #8413903 - Flags: review+
Attached patch fix and add tests v2 (obsolete) β€” β€” Splinter Review
The run on the try server seems strange, looks like some problem with building (independent on this patch). Still it has shown some small problems, that should be fixed in this new version.
Attachment #8415604 - Attachment is obsolete: true
Attachment #8415604 - Flags: feedback?(archaeopteryx)
Aryx, another try run please :)
Flags: needinfo?(archaeopteryx)
Great, much better:)
A failure on OS X. Reportedly there the main menu is some strange beast and we can't click it from mozmill. It is in the DOM but doesn't respond.

Small failure on Linux, again something with ordering of the tests. Something still does not clean up after itself.
This is just the isolated fix for the current mozmill failure.

I'll play with the new tests in a separate patch.
Attachment #8415929 - Flags: review?(standard8)
Attachment #8415929 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8415929 [details] [diff] [review]
fix test failure in test-unread-folders.js::test_newly_added_folder [checked in]

Looks ok, r=Standard8
Attachment #8415929 - Flags: review?(standard8)
Attachment #8415929 - Flags: review?(mkmelin+mozilla)
Attachment #8415929 - Flags: review+
Attached patch add tests v3 (obsolete) β€” β€” Splinter Review
Josiah, please try if you still see the problem with this patch (on top of today's trunk).
Attachment #8415716 - Attachment is obsolete: true
Attachment #8416065 - Flags: ui-review?(josiah)
Aryx, a new try run would be welcome.
Flags: needinfo?(archaeopteryx)
This is a proof of concept demo.xul illustrating how current patch could be streamlined (polish), and probably become more robust wrt syncing the checked state of respective UI elements.
Attachment #8416114 - Attachment mime type: application/vnd.mozilla.xul+xml → text/plain
Yes, I know about that, but it isn't so easy. There is generic code that manages all the commands in the menu. As this Compact view command is not plugged into the generic infrastructure, the generic code forces the menuitem and command to be disabled when I open the menu. I didn't want to plug it in as it would need to be done in several places and it felt too invasive to me. The point of this bug is we can restore the old modes with not much work/changes. But if it proves too difficult, we'll need to scrape the idea.
Comment on attachment 8413903 [details] [diff] [review]
patch v5.1 [checked in in comment 43]

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

Given that there were some problems with syncing the checked status of menuitems, I think this aspect of the patch could be polished to be more streamlined, extendable, and easier to maintain if we use a central command with Command Attribute Forwarding[1], as demonstrated in XUL Demo 1 (attachment 8416114 [details]). However, if this patch works correctly (or can be tweaked with less effort to that end), I'm fine with landing this as-is and do the polishing later. I don't want to delay this from landing.

I have only reviewed this particular aspect of the patch and described my proposal for minimal fix; more streamlining might be possible.
Disclaimer: My proposal has been tested per proof of concept xul in attachment 8416114 [details], but the actual code changes I suggest for this patch are untested. Also, I had to rush this review a bit under the circumstances.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Tutorial/Broadcasters_and_Observers#Command_Attribute_Forwarding

::: mail/base/content/folderPane.js
@@ +320,5 @@
> +   */
> +  _updateCompactState: function(aMode) {
> +    let checked = aMode.endsWith("_compact");
> +    let menuitem = document.getElementById("menu_compactFolderView");
> +    let appmenuitem = document.getElementById("appmenu_compactFolderView");

This entire block starting with "let menuitem" to the end of this function can be radically simplified if we just update the checked attribute of a central command which will then broadcast and update all observing UI elements automatically without any code, including UI elements potentially added later by us or addons if they listen to the same commands.

Which would become something like this:

let checked = aMode.endsWith("_compact");
let compactFolderViewCmd = document.getElementById("cmd_compactFolderView");
// Command Attribute Forwarding: Will update checked state of all UI elements observing "cmd_compactFolderView"
compactFolderViewCmd.setAttribute("checked", checked);
let baseMode = this.baseMode(aMode);
let compactToggleable = (baseMode in this._modes) &&
                        (this.fullMode(baseMode, true) in this._modes);
// Command Attribute Forwarding: Will disable all observing UI elements
compactFolderViewCmd.disabled = !compactToggleable;

@@ +356,5 @@
> +    // "Compact" checkbox in the UI whether the user actually wanted the compact version.
> +    let userMode = aMode;
> +    if (!userMode.endsWith("_compact")) {
> +      userMode = this.fullMode(aMode,
> +        document.getElementById("menu_compactFolderView").hasAttribute("checked"));

This is a bit flimsy, getting the userMode from one out of many UI elements; hasAttribute might also cause trouble when checked="false", I think.

Instead, better get this from the central command, and use getAttribute:
let compactFolderViewCmd = document.getElementById("cmd_compactFolderView");
userMode = this.fullMode(userMode, compactFolderViewCmd.getAttribute("checked"));

I know there's a comment in documentation somewhere that seems to suggest that hasAttribute is preferred; however, that only applies to cases of <element checked> (having shorthand checked attribute without explicit value), but that's not our case here.

@@ +402,5 @@
> +   * Name of the mode including the _compact suffix if appropriate.
> +   *
> +   * @param aMode  If set, construct the base name from this mode name instead
> +   *               of the currently active one.
> +   * @param aCOmpact  Bool value whether to force adding the suffix or not.

spelling: aCompact

::: mail/base/content/mailWindowOverlay.xul
@@ +1220,5 @@
>                        name="viewmessages"
>                        oncommand="gFolderTreeView.mode = this.value;"/>
> +            <menuseparator id="appmenu_compactViewSeparator"/>
> +            <menuitem id="appmenu_compactFolderView"
> +                      label="&compactVersion.label;"

&compactVersion.label isn't very descriptive in the grand scheme of things; perhaps better:

label="&compactFolderView.label;"

What about access key?

@@ +1222,5 @@
> +            <menuseparator id="appmenu_compactViewSeparator"/>
> +            <menuitem id="appmenu_compactFolderView"
> +                      label="&compactVersion.label;"
> +                      type="checkbox"
> +                      oncommand="gFolderTreeView.toggleCompact(this.hasAttribute('checked'));"/>

This should be changed to use central command:
command="cmd_CompactFolderView"

Then, in an appropriate place, add the command:
<command id="cmd_CompactFolderView" oncommand="toggleCompactFolderView()"/>

Add function:
function toggleCompactFolderView() {
  let compactFolderViewCmd = document.getElementById("cmd_compactFolderView");
  gFolderTreeView.toggleCompact(!(compactFolderViewCmd.getAttribute("checked")=="true"));
}

@@ +2382,5 @@
>                        oncommand="gFolderTreeView.mode = this.value;"/>
> +            <menuseparator id="menu_compactViewSeparator"/>
> +            <menuitem id="menu_compactFolderView"
> +                      label="&compactVersion.label;"
> +                      accesskey="&compactVersion.accesskey;"

dito, change &compactVersion.label and &compactVersion.accesskey as above

@@ +2384,5 @@
> +            <menuitem id="menu_compactFolderView"
> +                      label="&compactVersion.label;"
> +                      accesskey="&compactVersion.accesskey;"
> +                      type="checkbox"
> +                      oncommand="gFolderTreeView.toggleCompact(this.hasAttribute('checked'));"/>

dito, use command attribute to link to central command.

::: mail/locales/en-US/chrome/messenger/messenger.dtd
@@ +160,5 @@
>  <!ENTITY favoriteFolders.accesskey "F">
>  <!ENTITY recentFolders.label "Recent">
>  <!ENTITY recentFolders.accesskey "R">
> +<!ENTITY compactVersion.label "Compact View">
> +<!ENTITY compactVersion.accesskey "C">

This should be compactFolderView.label and compactFolderView.accesskey as recommended above
(In reply to :aceman from comment #56)
> Created attachment 8416065 [details] [diff] [review]
> add tests v3
> 
> Josiah, please try if you still see the problem with this patch (on top of
> today's trunk).

Yep, still hit it.
That is strange. You can see on TB-try the test passed on OS X too now. But I used the appmenu as the main menu is somehow strange on OS X. But previous version used the main menu and on other platforms it passed too. We need to talk on IRC about the exact clicks you use to reproduce it.
On official nightly builds it works fine for me on Windows and I also have other user confirm it. It works fine for me on my dev build in Linux.

But we also have standard8's comment that it does not work for him. So there must be something to it.
Thomas, thanks for your suggestions. But the patch+test I am working on needs to go into Aurora (TB31) so it is not allowed to be such an invasive change. Please file your whole comment 61 as a new bug and I will look at it as further refinement for trunk (TB32), once this bug is closed.
Attachment #8415929 - Attachment description: fix test failure in test-unread-folders.js::test_newly_added_folder → fix test failure in test-unread-folders.js::test_newly_added_folder [checked in]
Comment on attachment 8415929 [details] [diff] [review]
fix test failure in test-unread-folders.js::test_newly_added_folder [checked in]

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

This is needed on Aurora too. Otherwise newly unread folders do not appear in the Unread view.
Attachment #8415929 - Flags: approval-comm-aurora?
Attachment #8415929 - Flags: approval-comm-aurora? → approval-comm-aurora+
Comment on attachment 8416065 [details] [diff] [review]
add tests v3

I'm clearing the ui-r flag until we can sort out the OS X issues here.
Attachment #8416065 - Flags: ui-review?(josiah)
Thanks for the debugging on OS X and please try this.
Attachment #8416065 - Attachment is obsolete: true
Attachment #8425020 - Flags: ui-review?(josiah)
Comment on attachment 8425020 [details] [diff] [review]
add tests v4 [checkin: comment 72]

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

There we go! :)

Works great, ui-r+ (finally)
Attachment #8425020 - Flags: ui-review?(josiah) → ui-review+
Attachment #8425020 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8425020 [details] [diff] [review]
add tests v4 [checkin: comment 72]

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

Nothing to complain about :)
Attachment #8425020 - Flags: review?(mkmelin+mozilla) → review+
Thanks.
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/78e61031f8d4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 32.0
Attachment #8425020 - Attachment description: add tests v4 → add tests v4 [checkin: comment 72]
Comment on attachment 8425020 [details] [diff] [review]
add tests v4 [checkin: comment 72]

[Approval Request Comment]
Regression caused by (bug #): bug 978592
User impact if declined: change of interface for tests and extensions (set mode()) compared to TB24.
Testing completed (on c-c, etc.): trunk, tests added
Risk to taking this patch (and alternatives if risky):

I propose this for TB31 aurora. It may look like a big change, but actually isn't.
Compare set mode() function - the code is actually reverted to the state before attachment 8413903 [details] [diff] [review].
So it should return back to the original behaviour tests and possible extensions were used to.
Only the UI menuitems use the new toggle functionality.
Attachment #8425020 - Flags: approval-comm-aurora?
Attachment #8425020 - Flags: approval-comm-aurora? → approval-comm-aurora+
Thanks.
Target Milestone: Thunderbird 32.0 → Thunderbird 31.0
Target milestone still for 32. For branch, status-thunderbird31:fixed
Target Milestone: Thunderbird 31.0 → Thunderbird 32.0
You need to log in before you can comment on or make changes to this bug.