520272 - Optimize perceived speed of switching IMAP folders

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[David Ascher (:davida)]

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.

Comment 1

[David :Bienvenu]

Attachment: only display start page once per message pane - checked in.

this makes us only display the start page once per window/tab.

Comment 2

[David :Bienvenu]

Attachment: 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...

Comment 3

[David Ascher (:davida)]

I do like it better, a lot better.  I haven't seen it break anything yet. but it hasn't been long.

Comment 4

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

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).

Comment 5

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

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.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 7

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

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.

Comment 8

[David Ascher (:davida)]

Comment #4 feels way too scary to take for 3.0.  I think we should do this for 3.1

Comment 9

[David Ascher (:davida)]

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

Comment 10

[David :Bienvenu]

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.

Comment 11

[David :Bienvenu]

clearing blocking, but we'd love to fix this safely.

Comment 12

[David :Bienvenu]

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?

Comment 13

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

Be my guest.  Past Andrew's suggestion still sounds good.

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

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+

Comment 16

[Wayne Mery (:wsmwk)]

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.

Comment 17

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

(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.

Comment 18

[David :Bienvenu]

ok, gonna try this for 3.3

Comment 19

[David :Bienvenu]

Attachment: 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.

Comment 20

[David :Bienvenu]

builds should be here:
http://stage.mozilla.org/pub/mozilla.org/thunderbird/try-builds/[e-mail]-97a2c1bdf7d3/

Comment 21

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

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.

Comment 22

[David :Bienvenu]

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 :-)

Comment 24

[Wayne Mery (:wsmwk)]

(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?

Comment 25

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

(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).