Closed Bug 545221 Opened 11 years ago Closed 11 years ago

change folder pane, smart folders default settings

Categories

(Thunderbird :: Folder and Message Lists, defect)

defect
Not set
normal

Tracking

(blocking-thunderbird3.1 beta2+, thunderbird3.1 beta2-fixed)

RESOLVED FIXED
Thunderbird 3.1b2
Tracking Status
blocking-thunderbird3.1 --- beta2+
thunderbird3.1 --- beta2-fixed

People

(Reporter: clarkbw, Assigned: clarkbw)

References

Details

(Whiteboard: [UXprio])

Attachments

(2 files, 5 obsolete files)

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.
Can we mark these blocking ? Would be nice to have them in b1 to gather feedback.
Whiteboard: [UXprio]
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.
Assignee: nobody → clarkbw
Status: NEW → ASSIGNED
blocking-thunderbird3.1: --- → needed
Whiteboard: [UXprio] → [UXprio][has patch, needs review]
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
Attachment #428468 - Attachment is obsolete: true
Attachment #428478 - Flags: review?(bienvenu)
Comment on attachment 428478 [details] [diff] [review]
updated patch that removes the unused locale name

david, can you review this for me?
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
Attachment #428478 - Flags: review?(bienvenu) → review-
was this patch supposed to be on top of an other patch?
Attached patch updated patch (obsolete) — Splinter Review
updated patch but I still haven't successfully built TB so I haven't tested this yet either
Attachment #428478 - Attachment is obsolete: true
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! :)
Attached patch updated patch (obsolete) — Splinter Review
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.
Attachment #429737 - Attachment is obsolete: true
Attachment #429837 - Flags: review?(bienvenu)
blocking-thunderbird3.1: needed → beta2+
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).
(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...
(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.
Duplicate of this bug: 550960
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.
(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.
Attachment #429837 - Flags: review?(bienvenu) → review+
Whiteboard: [UXprio][has patch, needs review] → [UXprio][needs landing]
here's the updated patch.  carrying forward r+ from bienvenu even though I wouldn't mind if he checked it out anyway :)
Attachment #429837 - Attachment is obsolete: true
Attachment #432975 - Flags: review+
take that blocker bug!
Keywords: checkin-needed
Whiteboard: [UXprio][needs landing] → [UXprio][needs checkin]
Checked in: http://hg.mozilla.org/comm-central/rev/024c73a5eefd
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [UXprio][needs checkin] → [UXprio]
Target Milestone: --- → Thunderbird 3.1b2
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [UXprio] → [UXprio][needs unit tests fixing]
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.
(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?
(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.
Attached patch updated test (obsolete) — Splinter Review
(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.
Attachment #435735 - Flags: review?(bienvenu)
Attachment #435735 - Flags: feedback?(sid.bugzilla)
Attachment #435735 - Flags: feedback?(sid.bugzilla) → feedback+
Comment on attachment 435735 [details] [diff] [review]
updated test

As I mentioned on IRC, the test is all wrong. Thanks for fixing it!
Attached patch updated test v2Splinter Review
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.
Attachment #435735 - Attachment is obsolete: true
Attachment #435747 - Flags: review?(bienvenu)
Attachment #435735 - Flags: review?(bienvenu)
Whiteboard: [UXprio][needs unit tests fixing] → [UXprio][needs review of unit tests and landing]
Comment on attachment 435747 [details] [diff] [review]
updated test v2

this test fails w/o the patch, and succeeds with it...
Attachment #435747 - Flags: review?(bienvenu) → review+
Whiteboard: [UXprio][needs review of unit tests and landing] → [UXprio][checkin-needed]
I'll re-land it.
(In reply to comment #27)
> I'll re-land it.

Thanks Ben!
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>
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Whiteboard: [UXprio][checkin-needed] → [UXprio]
You need to log in before you can comment on or make changes to this bug.