GNOME Bugzilla – Bug 333707
Evolution mail list scrolls to beginning for no reason
Last modified: 2013-09-13 00:51:35 UTC
Whenever I get new mail that gets attached to a thread and click the expander to open that thread (I keep all expanders folded most of the time), the mail list immediately jumps back to the top of the list. Strictly speaking it doesn't happen EVERY time, but most of the time it does. Notably, it doesn't happen twice in a new mail batch. This bug is really annoying for mailing lists with large archives.
Yes, I am seeing a similar scolling problem in Evolution 2.6.0 and Gnome 2.14 (as distributed with FC5). For me, this is how to reproduce (I have threading turned off): 1. Highlight the bottom-most email in the mailbox so it gets viewed in the preview pane. 2. Delete that piece of mail. 3. Expunge. 4. Move focus to another application on the screen. 5. Move focus back to evolution. At the moment focus goes back to evolution, the mailbox will automatically scroll to the top of the mailbox and highlights the first message. I believe the correct action would be to highlight the message directly adjacent to the message just deleted. On my mailbox with 3000 messages, automatically scrolling to the top can be very annoying. This behavior did not happen with Evolution 2.0. It's especially annoying because I usually move focus back to evolution when new mail appears. As soon as my cursor moves over that new piece of mail, it scrolls out from under me to the top of the list.
I have isolated the cause of this bug to be the fix for bug 322776. That fix causes the message list to automatically select the topmost message (and thus scroll to the top) if there is nothing highlighted in the current message list. Nothing will be highlighted in the message list in at least two scenarios: 1. The previously highlighted message got expunged. 2. The previously highlighted message was a child of a threaded message, and the thread got folded shut. I see several possible solutions, of which perhaps all should be implemented: 1. Since bug 322776 was concerned with keyboard focus entering the message list, perhaps that fix should only be applied to the case when keyboard focus enters the message list, and NOT applied to the case where only mouse focus enters. 2. Perhaps the fix to bug 322776 should select the topmost message in the currently visible view of the message list (according to the scrolling position) rather than the absolute topmost message. 3. When the mailbox gets expunged, the nearest undeleted message should be highlighted, instead of highlighting nothing. 4. When a thread tree gets folded such that the highlighted message becomes invisible, highlight the (still visible) root of the tree, instead of highlighting nothing. Thoughts anyone?
Created attachment 63430 [details] [review] Patch that implements "solution 3" from comment #2 This patch modifies the function find_next_undeleted() in order to check for expunged messages as well as junk and deleted messages. Currently, find_next_undeleted() will happily choose an expunged message if the user has chosen to not "hide deleted messages". This is a bug. The only way that I could figure out if a message was expunged or not was to check if the refcount was 1 or less. If anyone has a better idea, let me know. I also modified find_next_undeleted() to start searching backwards in the list in the case where searching forwards fails to find a message. This fixes the problem when the last message in the mailbox goes away and the list gets rebuilt. The patch is against 2.6.0, but should apply cleanly to CVS also. I'll try to implement a patch for "solution 4" soon. Once that's done, this bug can probably be closed.
Created attachment 63505 [details] [review] Patch that implements "solution 4" from comment #2 This patch checks the cursor position when a thread gets collapsed. If the cursor lies inside the collapsed thread, the cursor gets set to the still-visible root of that thread. Together with my last patch, this patch should fix this bug completely. Could somebody please review these patches?
as far as the etree patch, I'm not sure if the logic is really correct in the callback function (not many people understand all the idiocyncricies of etable/etree, those who do have long since left the company) + + et->priv->table_cell_change_id = g_signal_connect (et->priv->etta, "model_rows_deleted", + G_CALLBACK (et_table_rows_deleted), et); this is wrong, you need to use a different variable to hold the id of the signal listener. you'll also need to disconnect it at the appropriate time, assuming this is even the rigth way to go about your fix. as far as the patch in comment #3, you shouldn't be looking at refcounts. I don't even see what you're trying to gauge based on refcounts. knowing the # of refs doesn't tell you who owns them. totally broken imho.
Created attachment 63516 [details] [review] Updated "solution 4" patch Sorry, I really botched the first version of this patch. Here's the fixed version.
Created attachment 63518 [details] [review] Updated "solution 3" patch Okay, here's a much improved version of the patch to find_next_undeleted() in message-list.c. I created a new function is_msg_disappeared() that checks if a message has disappeared by verifying that the folder summary still contains the message (instead of using refcounts). Could you please review these patches again?
ok, the etree patch looks much better now, at least in so far as I can't see any coding bugs in it. I don't feel comfortable approving it tho because I'm a noob when it comes to the ETable/ETree widget code :( The mailer patch looks much better too, but I've made some adjustments.
Created attachment 63713 [details] [review] fejjified "solution 3" patch This new message-list patch mostly just fixes code style stuff (s/rows --/rows--/ and indenting) and also changes is_msg_disappeared() which just felt awkward to me and made it message_exists() since that seems to be a more accurate description of what we wanted to know)
Thanks for reviewing the patches again. Just for the record, I thought my version of message_exists() was easier to read because it wasn't shrunk down into mostly one line of code. It should be the same in the eyes of an optimizing compiler. You also took out my comment -- and I can tell you it would have been a lot easier to write these patches had there been more comments in the surrounding code. Anyways, it's not that big a deal. Any etree gurus out there?
Confirming this bug. Fejj/other mail hackers : Patch status update ? Do they fix this bug now ?
Is there any progress here? This bug can drive elephants mad...
Can someone please commit these patches. I have heard no more complaints and they fix this bug, which is very annoying.
Changing status to NEW, patches need review.
harish: *poke* - patches attached.
I'm making another plea here for someone to please commit these patches.
varadhan, can the patch please be reviewed?
Any news regarding this one? Has anyone reviewed the patches yet?
Im getting this in for 2.11.3 Im obsoleting the mailer portion of the patch, as a similar patch has been committed some time back. I'm fine with the GAL patch and I have committed this for 2.11.3.