Optimize perceived speed of switching IMAP folders

RESOLVED FIXED in Thunderbird 5.0b1

Status

Thunderbird
Mail Window Front End
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: davida, Assigned: Bienvenu)

Tracking

({perf})

Trunk
Thunderbird 5.0b1
Dependency tree / graph
Bug Flags:
wanted-thunderbird +

Firefox Tracking Flags

(thunderbird5.0 wanted, blocking-thunderbird3.1 -)

Details

(Whiteboard: [no l10n impact][has awesome patch we should land immediately])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

8 years ago
There are a bunch of things going on when switching IMAP folders (in the same tab) which make the switch feel much slower than it could be.

0) When TB is offline, the switch is almost immediate.  There's no good UX reason why we can't have that speed when TB is online.  It'd be good to understand why we're blocking on network activity in there.

1) When we switch from folder A to folder B, we immediate show the thread pane contents for folder B (good), but we don't immediately scroll to the appropriate position (bad).  We should scroll to where we think we're going to be ASAP.

3) If the start page is displayed (default config), the start page flashes in between folders, more or less reliably depending on the speed of the various operations.  We should avoid that, as it's visually disrupting, and possibly taking significant CPU time (I'm hoping we're not also hitting the disk/network to get that page).

4) there's a lot of hiding and showing of the messsage header area which feels overly complex, but I don't expect we can untangle that mess soon.
Flags: blocking-thunderbird3?
(Assignee)

Comment 1

8 years ago
Created attachment 404338 [details] [diff] [review]
only display start page once per message pane - checked in.

this makes us only display the start page once per window/tab.
Assignee: nobody → bienvenu
Attachment #404338 - Flags: ui-review?(clarkbw)
Attachment #404338 - Flags: review?(david.ascher)
(Reporter)

Updated

8 years ago
Attachment #404338 - Flags: review?(david.ascher) → review+
(Assignee)

Comment 2

8 years ago
Created attachment 404347 [details] [diff] [review]
patch for david to try...

I'm not sure what this breaks, but it makes us do the scrolling, msg selecting, etc, immediately after updating the folder. I'm definitely not proposing this as a fix, but something for David to try to see if he likes it better...
(Reporter)

Comment 3

8 years ago
I do like it better, a lot better.  I haven't seen it break anything yet. but it hasn't been long.
I don't think the logic change should happen in FolderNotificationHelper.

I would suggest we structure it this way:

- Migrate most of the logic triggered by FolderDisplayWidget.onAllMessagesLoaded to a newly introduced "onSomeMessagesLoaded" event.  This allows us to generate onSomeMessagesLoaded when entering the IMAP folder (that has headers already) and onAllMessagesLoaded when the update completes.  In the case of local folders we generate both messages in succession passing a flag in the all case to indicate that no time has elapsed since the 'some case'.  In the case of virtual folders we generate some/all together when the search completes[1] for now.  In the future, virtual folders could generate 'some' as soon as the local search completes[1] and 'all' as soon as the updateFolder for each of the underlying folder completes (when we start generating updateFolder calls for each underlying folder, that is.)

 1: I am unclear on whether we can rely on the cached headers at this point given that I understand that quick-search and mail-views tend to clobber them.


- Add logic triggered by FolderDisplayWidget.onAllMessagesLoaded that re-scrolls the pane if we scrolled things via the "towards the newest message" case and the user did not change the scroll (and we did not just process the 'some' event).  The immediate way that springs to mind is to save the logical last visible row if we position things that way, and when we get the notification, see if we are still in that position.  If so, re-apply the logic.  Sort changes would invalidate the value (set it back to null).  This is the "snap-to" logic discussed with clarkbw where we want to act like a terminal window.  (Roughly speaking; we don't actually want to scroll all the way down, just make sure we are showing as much as the set of new messages as possible.  If there is more than one page of new messages, we want the oldest new ones.)


While we are at it, we should change the behavior of DBViewWrapper.shouldShowMessagesForFolderImmediately to no longer check the manyHeadersToDownload case.  That change was blocking on the db change listener providing notifications for the database getting blown away and re-created.  Since we have that notification, we can now do that.  I think there is already a separate bug on that (account central flicker).
Er, another possibility than to introduce onSomeMessagesLoaded is to change onAllMessagesLoaded to onMessagesLoaded(aAll) and have the contract that we are done calling that once we pass true.

Updated

8 years ago
Whiteboard: [no l10n impact]
Marking as blocking+ for point 3) in comment 0.  We'd also really really like to get the rest of the stuff.  That said, if this were the last bug and we found some other way to fix point 3, we might choose that.
Assignee: bienvenu → bugmail
Flags: blocking-thunderbird3? → blocking-thunderbird3+
Attachment #404338 - Flags: ui-review?(clarkbw) → ui-review+
Comment on attachment 404347 [details] [diff] [review]
patch for david to try...

This does feel a lot faster!  Switching between folders doesn't have an unknown wait time for things to sync.

Updated

8 years ago
Blocks: 518869
(Assignee)

Updated

8 years ago
Attachment #404338 - Attachment description: only display start page once per message pane → only display start page once per message pane - checked in.

Updated

8 years ago
Keywords: perf
(Reporter)

Comment 8

8 years ago
Comment #4 feels way too scary to take for 3.0.  I think we should do this for 3.1
(Reporter)

Comment 9

8 years ago
(oh, and point 3 in comment #0 is checked in, so I think we're done with the blocking bits specified in comment #6)
(Assignee)

Comment 10

8 years ago
I think only the first big paragraph of #c4 is actually required, which isn't quite as scary as the whole comment. But it's scary enough.
(Assignee)

Comment 11

8 years ago
clearing blocking, but we'd love to fix this safely.
Flags: blocking-thunderbird3+ → blocking-thunderbird3.1+
(Assignee)

Comment 12

8 years ago
I think this would be good to get early into 3.1, if only to encourage folks to use 3.1 nightly builds sooner rather than later :-) Unless you've started on this, I'm willing to take a whack at it, asuth. Is #c4 still the way you'd suggest structuring this?
Be my guest.  Past Andrew's suggestion still sounds good.
Assignee: bugmail → bienvenu
(Assignee)

Updated

8 years ago
Duplicate of this bug: 539826
I would love to get this for 3.1.  That said, given 3.1's main priority is a softer landing pad for Tb2 users, I can't see us holding for this.  Marking blocking- and wanted-thudnerbird+
blocking-thunderbird3.1: --- → -
Flags: blocking-thunderbird3.1+ → wanted-thunderbird+

Comment 16

7 years ago
wow, this would be really nice, even on a fast machine. the change in speed is impressive.

andrew,  in comment 4 are you suggesting this could be extended to virtual folders?  if so this would be doubly great there, given that a) unified folders is the default behavior, b) our account creation process favors imap vs pop.


> I think there is already a separate bug on that (account central flicker).

I was not successful in finding that bug.
(In reply to comment #16)
> andrew,  in comment 4 are you suggesting this could be extended to virtual
> folders?  if so this would be doubly great there, given that a) unified folders
> is the default behavior, b) our account creation process favors imap vs pop.

I think so, assuming at least one of the folders is capable of offline searches.
 
> > I think there is already a separate bug on that (account central flicker).
> 
> I was not successful in finding that bug.

I think we fixed it in bug 499069.

Updated

7 years ago
Blocks: 534520
(Assignee)

Comment 18

7 years ago
ok, gonna try this for 3.3
Status: NEW → ASSIGNED
(Assignee)

Updated

7 years ago
status-thunderbird5.0: --- → wanted
(Assignee)

Comment 19

7 years ago
Created attachment 529003 [details] [diff] [review]
first pass at asuth's suggested approach

This seems to work - I don't guarantee that you'll get both onMessagesLoaded notifications, but I'm not sure that we need to. I haven't done anything with the scrolling, but I did check that if you select a message, we won't scroll to new. This passes all mozmill tests, and I'll generate a try server build if anyone wants to try it.
Attachment #529003 - Flags: feedback?(bugmail)
(Assignee)

Updated

7 years ago
Whiteboard: [no l10n impact] → [no l10n impact][has patch for feedback]
(Assignee)

Comment 20

7 years ago
builds should be here:
http://stage.mozilla.org/pub/mozilla.org/thunderbird/try-builds/bienvenu@nventure.com-97a2c1bdf7d3/
Attachment #404347 - Attachment is obsolete: true
Comment on attachment 529003 [details] [diff] [review]
first pass at asuth's suggested approach

YES! Woo! Ship it!

This is a massive UX improvement, especially on underpowered IMAP servers.  My only concern is that underpowered IMAP servers may also have other serious deficiencies (I'm looking at you, dreamhost) which trick autosync so that the lack of scrolling may be a problem because we may not actually get the headers until we enter.  Of course, if the Thunderbird instance is the one moving messages into folders, that should be fine.  (In other words, on crappy IMAP servers using sieve or multiple Thunderbird instances, there may be issues.)  That is most definitely something for another bug to deal with, though.

Thanks for making the effort to fix-up the comments so extensively.
Attachment #529003 - Flags: feedback?(bugmail) → review+
Whiteboard: [no l10n impact][has patch for feedback] → [no l10n impact][has awesome patch we should land immediately]
(Assignee)

Comment 22

7 years ago
fixed on trunk - http://hg.mozilla.org/comm-central/rev/cb7eb50cc3c5

thx for the quick feedback/review, and the comments were very helpful in figuring out what was going on, so keeping them up to date was payback :-)
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.3a4
(Assignee)

Updated

7 years ago
Duplicate of this bug: 607486

Comment 24

5 years ago
(In reply to Andrew Sutherland (:asuth) from comment #17)
> (In reply to comment #16)
> > andrew,  in comment 4 are you suggesting this could be extended to virtual
> > folders?  if so this would be doubly great there, given that a) unified folders
> > is the default behavior, b) our account creation process favors imap vs pop.
> 
> I think so, assuming at least one of the folders is capable of offline
> searches.

Everyone,

Was this idea implemented in the patch?
(In reply to Wayne Mery (:wsmwk) from comment #24)
> Was this idea implemented in the patch?

I think the combination of this patch and some follow-on patches got everything.  I think your interest about virtual folders was done in a follow-up patch where 'shouldShowMessagesForFolderImmediately' ended up being always true for virtual folders (or maybe it was already the case).
You need to log in before you can comment on or make changes to this bug.