545221 - change folder pane, smart folders default settings

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

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

Smart Folders are awesome but for users upgrading from existing All Folders it can be confusing where the folders have gone.

Here are what I believe are our two options:

Default to smart folders only for new users, ignore upgrading users
 - or -
Not default to smart folders for any user

In either case we'll need to update the migration assistant to help sell the smart folders system or help people find their old all folders setting better.

Comment 1

[Ludovic Hirlimann [:Usul]]

Can we mark these blocking ? Would be nice to have them in b1 to gather feedback.

Comment 2

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

Attachment: revert default mode back to all from smart

Here's a patch that changes the default mode from smart folders back to all folders and removes the upgrade code which brings old users from all to smart.

Comment 3

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

Attachment: updated patch that removes the unused locale name

meant to remove the smartAccountName entity as well, here's a patch that does that
http://mxr.mozilla.org/comm-central/search?find=%2Fmail%2F&string=smartAccountName

Comment 4

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

Comment on attachment 428478 [details] [diff] [review]
updated patch that removes the unused locale name

david, can you review this for me?

Comment 5

[David :Bienvenu]

Comment on attachment 428478 [details] [diff] [review]
updated patch that removes the unused locale name

this gets rid of smart folders mode, which I don't think was the intent :-) I believe the reason is that smartAccountName is actually used:

        smartServer.prettyName = document.getElementById("bundle_messenger")
                                         .getString("smartAccountName");

I don't know if it gets reflected in the ui, but I'll check

Comment 6

[David :Bienvenu]

was this patch supposed to be on top of an other patch?

Comment 7

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

Attachment: updated patch

updated patch but I still haven't successfully built TB so I haven't tested this yet either

Comment 8

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

Ok, I couldn't find where the smartServer.prettyName was used even after setting it to a strange value but I'm going to leave that string in there until we actually know what's going on.  Plus it makes the patch work! :)

Comment 9

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

Attachment: updated patch

Ok, here's a patch that makes this happen.  I tested with an upgrade from a 2.0 profile to a 3.0 profile and my old mode pref was saved.

I'm leaving the SmartAccountName string in there for now as I can't seem to figure out how it's being used here:

        smartServer.prettyName = document.getElementById("bundle_messenger")
                                         .getString("smartAccountName");

Lame, yes but I tried a number of ways to make that prettyName show and couldn't find the string.

Comment 10

[David :Bienvenu]

localStore.rdf stores the folder pane mode as a string, and I don't think we want to change the mode name, just the localization (though I think we can't change the string without changing the string name, which implies changing the mode name).

Comment 11

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

(In reply to comment #10)
> localStore.rdf stores the folder pane mode as a string, and I don't think we
> want to change the mode name, just the localization (though I think we can't
> change the string without changing the string name, which implies changing the
> mode name).

Ok, I feel dizzy and spun around.  I'll buy whatever you're selling, a cornballer you say?  Sounds like it won't burn me at all...

Comment 12

[David :Bienvenu]

(In reply to comment #11)
> 
> Ok, I feel dizzy and spun around.  I'll buy whatever you're selling, a
> cornballer you say?  Sounds like it won't burn me at all...

I believe they're perfectly legal outside the U.S. :-)

I'll try to figure out how to change the string when I get a bit of time.

Comment 14

[Fenton]

Discrete mailboxes, without "All Folders", should optional feature, rather than a requirement. Conversely, for downward compatibility, "All Folders" without discrete folders, should be an optional feature.

Comment 15

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

(In reply to comment #14)
This bug is about changing the default from smart folders back to all folders, making smart folders an optional feature.

-

Just to note that bug 545217 has a patch which, when it lands, changes all the entity names for changing the smart folders string.

Comment 16

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

Attachment: updated patch now that bug 545217 landed

here's the updated patch.  carrying forward r+ from bienvenu even though I wouldn't mind if he checked it out anyway :)

Comment 17

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

take that blocker bug!

Comment 18

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/024c73a5eefd

Comment 19

[Mark Banner (:standard8)]

It looks like someone forgot to run MozMill tests - failing due to:

TEST-PASS |  setupModule
TEST-UNEXPECTED-FAIL |  test_toggle_folder_open_state
  EXCEPTION: The folder tree view's row count should be 4, but is actually 7
    at: test-folder-display-helpers.js line 2127
       Error("The folder tree view's row count should be 4, but is actually 7")  0
       assert_folder_tree_view_row_count(4) test-folder-display-helpers.js 2127
       test_toggle_folder_open_state() test-folder-pane.js 70
            frame.js 468
            frame.js 520
            frame.js 562
            frame.js 411
            frame.js 568
            server.js 164
            server.js 168
TEST-PASS |  teardownModule
TEST-UNEXPECTED-FAIL | (runtestlist.py) | Exited with code 1 during directory run
INFO | (runtestlist.py) | folder-pane: 2 passed, 1 failed

So I backed this out:

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

Comment 20

[Ben Bucksch (:BenB)]

If a test fails due to a good change, please back out the test, not the change. A test which fails due to a good change is a broken test, by definition.

Comment 21

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

(In reply to comment #20)
> If a test fails due to a good change, please back out the test, not the change.
> A test which fails due to a good change is a broken test, by definition.

How is Standard8 or anyone else supposed to know that the test is broken and not your code?  Also, who else is actually going to fix the test?

Comment 22

[Mark Banner (:standard8)]

(In reply to comment #20)
> If a test fails due to a good change, please back out the test, not the change.
> A test which fails due to a good change is a broken test, by definition.

No way. This code broke the *existing* tests. I'm not saying no-one ran the tests, but it certainly looked that way to me. If we were to apply your rule generally, then it'd mean that if I caused a regression which the tests showed up, I would have to back out the test and then fix the test for the regression.

The only time your suggestion is feasible IMO is when a patch lands with tests and its obvious the tests aren't quite right for a particular platform, or its obvious the tests are wrong and need further tweaks.

Comment 23

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

Attachment: updated test

(In reply to comment #19)
> It looks like someone forgot to run MozMill tests - failing due to:

Sorry I never ran the tests with that other patch.

Here's an updated test which does pass.  The problem was that Smart Folder would give an initialized view such that you'd get a higher row count from the beginning and adding a two folders would actually only increase that count by 2.

With All folders you start with an initial count of 2 (one for each account) and then once you create (and go into) a folder you get a much higher count with the Inbox, Trash, and Outbox appearing.

I've essentially expanded the test to check that we are always showing the correct number of items.  It might make sense that this test be renamed to an "all_folders" specific variety.  I'd appreciate feedback on that.

This is my lowest priority item right now so I'll be slow getting back to reviews.  Also I have no idea who to ask for this so I'm putting bienvenu back on and adding sid0 for feedback since he wrote the original test.

This only contains the test, the old patch still applies even though it requires some fuzz to get in there.

Comment 24

[Rain [:rain1] (inactive)]

Comment on attachment 435735 [details] [diff] [review]
updated test

As I mentioned on IRC, the test is all wrong. Thanks for fixing it!

Comment 25

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

Attachment: updated test v2

Ok, here's a finalized version of the test after chatting with sid0 on IRC.  I've added on extra assert at the beginning checking that we are in All Folders mode.  I've also added a comment to the function and renamed it to be a bit more clear about the scope of it's tests.

Comment 26

[David :Bienvenu]

Comment on attachment 435747 [details] [diff] [review]
updated test v2

this test fails w/o the patch, and succeeds with it...

Comment 27

[Ben Bucksch (:BenB)]

I'll re-land it.

Comment 28

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

(In reply to comment #27)
> I'll re-land it.

Thanks Ben!

Comment 29

[Ben Bucksch (:BenB)]

After running tests for an hour, seeing errors, and noticing that the errors also happen on trunk (esp. in mozmill), I'm going to ignore the test suite.

Tested manually and it works fine, fixes the bug.

Commited as <http://hg.mozilla.org/comm-central/rev/b42a5d2e6c5b>