Ah. So, the folder doesn't refresh when the results related to search changes. I will fix it. Thanks for the bug.

Just back from my vacation. On my list for this week to fix.

Created attachment 122921 [details] [review]
Proposed patch

This would correctly refresh the vfolder and the counts when the query is reset.

I tested the latest HEAD with the patch, and the count in step 9 is fixed but the absence of the first message from the vfolder in steps 6 and 7 is not.

I'm guessing the problem is that after a new message matching the vfolder query appears, a full refresh of the vfolder is needed to bring in the rest of the thread.  The behavior was the same in Evolution 2.22, but there I could force a refresh by switching to another folder and back, and I trained myself to do that quickly.  Now, switching folders no longer forces a refresh, so I have no good way to bring in the rest of the thread.  (I suppose I could change the query and change it back, but that's too cumbersome to do many times a day and I would be afraid of messing up the query.)

Not refreshing a vfolder every time it is entered does make Evolution feel more "solid", but that's no good if the vfolder contents are incorrect.  It would be nice if a new matching message pulled in the rest of the thread without a refresh.  However, whether or not that improvement is made, I think there should be a way to force a refresh so that users can work around any future bugs in the incremental vfolder updating.  Making the "Refresh" menu item and keybinding work would be ideal.

Committed this to trunk/stable.

I think I understand your issue now. Parent isn't coming well, in All related, if child is marked important. Right? I will see it sometime this week. 

(In reply to comment #7)
> I think I understand your issue now. Parent isn't coming well, in All related,
> if child is marked important. Right?

Right.  To be more precise, I think the query is correct, because if I force a full refresh by changing the query (exercising the code path in your patch in comment #3), the correct vfolder contents appear.  I'm guessing (from my experience using Evolution, without having read the source) that the problem is that *incremental* updating of a vfolder when a new message matching the criteria appears does not consider the "Include threads" setting, so it adds only the new message, not the rest of the thread.

If that is indeed the case, it would not be a regression: Evolution 2.22 behaved the same way.  The difference is that with Evolution 2.22, I had a workaround of forcing a full refresh by switching to another folder and back, but this workaround no longer works in HEAD.

> I will see it sometime this week. 

Great!  This bug will block my upgrade to Fedora 10 (to be released tomorrow), so I would be very grateful for a prompt fix.

Making the summary more specific.

Created attachment 123407 [details] [review]
Fix 2nd issue by redoing search when source folder changes

This patch modifies folder_changed_change in camel-vee-folder.c to purge the old results from the DB in the case that the vfolder uses "Include threads" (aka "match-threads") instead of trying to incrementally update them.  Purging the results makes the next call to vee_refresh_info re-execute the query on the changed source folder from scratch, and I can call vee_refresh_info by leaving and re-entering the vfolder, just like in Evolution 2.22.

The patch is hack quality, but it does seem to solve my immediate problem.  A better solution would be to enhance the incremental updating code in folder_changed_change to consider match-threads, but the threader would have to gain the ability to find messages related to a given message without threading an entire folder, and anyway it's a larger change than I dare attempt on a unfamiliar codebase as complex as Evolution.

I would still like an option in the UI to force a complete vfolder re-query for all source folders in case something goes awry.

Matt, but this would be very slow. Lemme try out non-caching vfolders for threaded ones.

Created attachment 123522 [details] [review]
Don't cache results of threaded vfolders

Try this patch. This breaks counts, which I need to work. But when you do this, you can double click on the vfolder to refresh the contents.

And thanks Matt, your patch helped me understand the issue of the code much better. Test the patch and lemme know. But I must work out the count issue due to this. which is why I have set the patch to needs-work.

I tested the patch from comment 12, and it gives the same results as mine.  I don't understand why my patch would be any slower than yours: the only difference appears to be that your code ignores the old matches while mine actually deletes them from the database.

Both patches have the same issue of not removing old messages from the count.  I'm guessing this could be fixed with some COUNT_DEL_EXPRs like in vee_folder_remove_folder.  There may be other stuff that vee_folder_remove_folder is doing that our code should do too in order to avoid less obvious bugs.  In fact, I wonder if it would work to just call vee_folder_remove_folder, since vee_add_folder suggests that the next vee_rebuild_folder call is enough to reverse whatever vee_folder_remove_folder does.

Created attachment 123724 [details] [review]
Redo search *and force counts* when source folder changes

Calling vee_folder_remove_folder still resulted in incorrect counts.  I'm thinking this is because the COUNT_DEL_EXPRs were subtracting based on the new source folder contents, while the counts included the old source folder contents.  In fact, since the old source folder contents are no longer available, there is no way to determine the proper amount by which to decrease the counts.

Thus, I instead made folder_changed_change set the force_counts flag on the summary and made vee_refresh_info check that flag.  (The same idea could be implemented just as easily on top of your patch.)  I tested a vfolder pretty thoroughly with this patch and the counts were always correct.  I'm sure there's a performance penalty to this, but I don't see what else to do.

Note: It looks like force_counts is supposed to recalculate the counts only once, so perhaps redo_counts should set it to false.  But the existing code didn't do this, so I didn't do it in the patch.

Created attachment 123745 [details] [review]
Don't cache results + force counts and update display

With any of our patches so far, folder_changed_change on a threaded vfolder returns without completing the ordinary incremental updating.  As a result, the display of the vfolder does not update when I click the envelope icon of a message in the list to make it unread.

The attached patch is based on the one from comment 12 and has both the counts and the incremental updating fixed.  I tested it and everything seems to work properly, except that once in a while the vfolder's unread count in the folder tree gets out of sync, but this is quite hard to reproduce.

I have migrated my data from Fedora's evolution-2.22.3.1-1.fc9.i386 to an Evolution I compiled from the gnome-2-24 branch + the patch from comment 16.  My threaded vfolders are working great (and I can sort the task list without a crash, my original motivation for upgrading to Evolution 2.24), but my non-threaded "All mail" vfolder isn't showing any messages.  I will investigate.

(In reply to comment #14)
> I tested the patch from comment 12, and it gives the same results as mine.  I
> don't understand why my patch would be any slower than yours: the only
> difference appears to be that your code ignores the old matches while mine
> actually deletes them from the database.
> 
The difference is that, for every read/unread you might endup a DB call for cache/delete-cache, where as I avoided it. Disk operations are costly.

Im yet to read your other patches, would read and reply. thanks 

I must say that I liked your patch. For vfolders, it might make sense to force counts on load. But just commit to stable/trunk.

And thanks a lot for your patch and patience.

I have just encountered some severe bugginess in the patched Evolution: there's a thread that won't leave my vfolder no matter what I do, even though none of the messages in the thread matches the criteria.  Please hold off on committing the patch.

The root of the misbehavior appears to have been the dreaded "Summary and folder mismatch, even after a sync" condition, which I fixed by moving all the messages in the affected folder to another folder.  My computer suffered a system-wide crash shortly before the misbehavior, and I am thinking (hoping) the corruption was the fault of the crash and not the patch.  Still, I would like to test the patched Evolution for a few more days before you commit the patch to trunk, let alone to stable.  And we might want to look into the problem with non-threaded vfolders (comment 17).

Created attachment 123929 [details] [review]
Revised patch

Some improvements:

- redo_counts: Clear force_counts so that the counts are redone only once.  (Noticeable performance improvement.  Next, if possible, I want to make redo_counts query the vfolder instead of all the source folders.)
- vee_folder_sync was clearing the folders_changed list without actually rebuilding the vfolder.  I removed that, thinking it might contribute to failure to update properly.
- vee_rebuild_folder: Don't bother adding to the counts when we are going to force them anyway.
- vee_rebuild_folder: We never read the results for threaded vfolders from the DB, so don't bother to store them there.

This still needs more testing.

Matt, 

I would say push this. It looks good. You can work on top of it :-) for more issues. Otherwise, it drags a lot.

I still think pushing the patch, especially to so-called "stable", is a bad idea.  There are several things I'm unhappy with about the patch; most notably, in my testing, the count redoing introduces a ~15 second delay at shutdown during which Evolution is unresponsive and the "Force Quit" dialog comes up.

People who want somewhat-better-functioning threaded vfolders right now (including me) should be watching this bug and can use the patch, but I think the general public would be better off without it until it is more solid.  The premature (IMO) pushing of the disk-summary changes has caused me a lot of pain with threaded vfolders, and I would hate to do the same to others here.

Matt, Ok, I get it. but, delaying much might have other issues. Lets delay a bit more.

(In reply to comment #26)
> but, delaying much might have other issues.

Is there an upcoming release you want this fix to make or something?

The current vfolder-updating code is quite convoluted, and I need to take some time to really understand what it is doing so I can fix threaded vfolders without breaking anything else.  I'm thinking that the current slow approach of re-querying all the source folders to update vfolder counts is totally unnecessary.  We just need to make vee_rebuild_folder update the counts incrementally using update_summary, just like folder_changed_change does.  In fact, I don't see why vee_rebuild_folder shouldn't use the same folder_changed_{add,change,remove}_uid helpers; that would reduce code duplication.

The last week was good for me, and I seem to have quite a lot of things for 2.24.2.1 or so. But I would wait a week or two before that. So we have time till that.

Matt, So I'm working on a bit redesign for counts for vfolder, that solves everything. I sort of have a flag-cache now. But I don't use it fully. I'm working to see, if I can make that flag-cache, then I don't search the DB for counts. Which should be pretty faster. So you ignore hte count part in lot, and worry about the feature :-). Thanks a lot for your help

Are you talking about the same thing I am, namely letting update_summary update the counts based on its cached flags?  I'm working on this now.

Created attachment 124139 [details] [review]
Unfinished patch with better counts

Here's what I have right now.  I removed the count-forcing, COUNT_ADD_EXPR, and COUNT_DEL_EXPR and am preparing to change vee_rebuild_folder and vee_folder_remove_folder to use the folder_changed_{add,change,remove}_uid helper functions, which update the counts using update_summary.  Compiles but completely untested.

Created attachment 124146 [details] [review]
Patch with better counts (milestone 3)

In this patch, I've changed everything over to use the folder_changed_*_uid functions.  So far the counts seem to be working, but the folder doesn't update properly if the user edits the list of source folders after vfolder creation.  I will fix that next.

Created attachment 124148 [details] [review]
Patch with better counts (milestone 4), working

I couldn't reproduce the problem I mentioned in the last comment, but I found another one with adding of matching messages to the DB, which I fixed.

At this point, the patch works in my simple tests, but it needs much more thorough testing (including with non-threaded vfolders, Unmatched, and Evolution restarts) before it is committed.  I feel pretty confident that I have the right approach, but I may have overlooked some small things and the patch could use some tidying up.  I took the liberty to change some names to make them more meaningful.

Matt, its bit different. CamelVeeMessageInfo has old_flags that corresponds to the flags on CamelMessageInfo that it marks.

Every CamelVeeMessageInfo points to a folder/uid. At times they are in memory, as CamelMessageInfo and at times, in the db, which we query. Thatz one reason, we don't count, instead search in db for flags. This flags tell, what we have. 

During start up we don't have these old_flags, unless the vfolder is clicked on. But after that this old_flags is in sync with the real messsage. I'm now on working so that I can get this old_flags loaded, so that,

flag & CAMEL_MESSAGE_SEEN will tell me the urnead status and I can count like this for all message. This is how it it used to be for everything. But looks like I will put htis back for vfolder alone. For real folders, it works well.

I would say, hold back to your previous patch, [I understand that better than this :-) ] and gimme a day, I should get over this counts, and mine should fix counts for *ALL* vfolders.

In my patch, I ignored the issue of whether the flags were loaded, and it seems to work except that the counts increase incorrectly at startup; I don't know whether this is exactly the issue you are talking about or something else.  If you know the proper way to deal with the flags, by all means do it, but please look at my patch and consider some of the other improvements, such as routing all vfolder changes through the same vee_*_match functions (renamed from folder_changed_*_uid) to significantly reduce code duplication.  In the hope that Evolution would start faster by not having to redo as many queries, my latest patch also goes back to storing threaded vfolder results in the DB and deleting them when something changes, but I'm not sure that is a good idea.

Matt, Sure, I like quite a few bits in your patch. I would discuss and pick from you, when we commit it :-)

Created attachment 124151 [details] [review]
Proposed patch

With this, vfolder counts should be right always :-). Evo startup should also be faster with vfolder counts not playing much.

All that count_result/folder can be blindlly removed. Try removing all of them and it should be a lot more faster.

Matt, did you try with this? 

I am ready to test now.  It looks like your patch doesn't have any special behavior for threaded vfolders; should I be testing it in combination with mine from comment #35 or what?

Your patch (comment 37) and mine (comment 33) appear to be completely independent in what they change, so I'm using an Evolution containing both of them.  It works great in a few simple tests on my real mail collection, and it feels fast and solid; I'm really impressed!  I will let you know if I discover any problems over the next few days.

There's one opportunity for a further performance improvement that might be convenient to address here even though it doesn't relate specifically to threaded vfolders.  Currently, when vee_rebuild_folder loads matches from the database, it has no way to distinguish the lack of cached matches (meaning it should redo the query) from a cached match set that is genuinely empty.  If we could represent the difference in the DB somehow, that would make Evolution start considerably faster for me, because I have several vfolders that have source folders containing many messages but no matches.  A clever technique that is slightly kludgy but should have minimal compatibility problems (as compared to changing the schema) is to represent a cached empty set by storing a row with just the source hash and no source uid concatenated onto it.  This case would be easy to recognize in vee_rebuild_folder.  I'll code it up and post a patch so we can see what we think.

Created attachment 124242 [details] [review]
Cache empty match sets properly

Here's the patch that caches empty sets of matching messages (to be used in combination with the other patches).  With this patch, Evolution still works correctly, but I have been unable to demonstrate the performance improvement I expected with threaded vfolders.  Perhaps there would be one with non-threaded vfolders.

In any event, I'm very happy with my current Evolution with patches from comment 33 and comment 37.  The "Refresh" command even works (to pull in the rest of a thread one message of which just started to match); it didn't in Evolution 2.22.  The patch combo could use more testing, especially in the areas that I don't use much (most notably non-threaded vfolders), but then I would consider it ready to commit.

Yep, my patch is pretty independent. Now you don't have to worry on count/etc. Just rework your patch, ignore count portion and it should rock. It should change some dynamics in your old patch, where you do to get the count right.

(In reply to comment #41)
> Created an attachment (id=124242) [edit]
> Cache empty match sets properly
> 
> Here's the patch that caches empty sets of matching messages (to be used in
> combination with the other patches).  With this patch, Evolution still works
> correctly, but I have been unable to demonstrate the performance improvement I
> expected with threaded vfolders.  Perhaps there would be one with non-threaded
> vfolders.
> 
Matt, I didn't understand the dummy stuff. How can the uid be empty? You have some data/reasons?


Also, I would commit my patch by avoid abi break. Meanwhile can you rework your patch (with my patch on your machine) on comment #37? Thanks a lot. I see that vfolders are lot faster now.

(In reply to comment #42)
> Yep, my patch is pretty independent. Now you don't have to worry on count/etc.
> Just rework your patch, ignore count portion and it should rock. It should
> change some dynamics in your old patch, where you do to get the count right.

Let me make sure I understand correctly.  There are three functions that modify vfolder contents:

- vee_rebuild_folder
- vee_folder_remove_folder
- folder_changed_change.

Before this bug, the counts were updated as follows:

- vee_rebuild_folder used COUNT_ADD_EXPR
- vee_folder_remove_folder used COUNT_DEL_EXPR
- folder_changed_change used update_summary via the folder_changed_*_uid helpers

My patch renames the folder_changed_*_uid helpers to vee_*_match, gets rid of COUNT_*_EXPR, and changes all three functions to use vee_*_match, which call update_summary.

But now, if I understand your patch from comment 37 correctly, every time someone requests one of the counts, you recompute it by scanning the summary.  (I thought this might be slow, but in my testing it didn't seem to be.)  Does this mean I can get rid of update_summary altogether?  If so, that would remove the most pressing reason to route all three functions through a common set of helpers, but we might wish to do so anyway to reduce code duplication.

(In reply to comment #44)
> (In reply to comment #42)
> > Yep, my patch is pretty independent. Now you don't have to worry on count/etc.
> > Just rework your patch, ignore count portion and it should rock. It should
> > change some dynamics in your old patch, where you do to get the count right.
> 
> Let me make sure I understand correctly.  There are three functions that modify
> vfolder contents:
> 
> - vee_rebuild_folder
> - vee_folder_remove_folder
> - folder_changed_change.
> 
> Before this bug, the counts were updated as follows:
> 
> - vee_rebuild_folder used COUNT_ADD_EXPR

No need to fix/correct count anymore.

> - vee_folder_remove_folder used COUNT_DEL_EXPR
> - folder_changed_change used update_summary via the folder_changed_*_uid
> helpers
> 
> My patch renames the folder_changed_*_uid helpers to vee_*_match, gets rid of
> COUNT_*_EXPR, and changes all three functions to use vee_*_match, which call
> update_summary.

We don't need COUNT_* anymore. neither update_summary
> 
> But now, if I understand your patch from comment 37 correctly, every time
> someone requests one of the counts, you recompute it by scanning the summary. 
> (I thought this might be slow, but in my testing it didn't seem to be.)  Does
> this mean I can get rid of update_summary altogether?  If so, that would remove
> the most pressing reason to route all three functions through a common set of
> helpers, but we might wish to do so anyway to reduce code duplication.
> 

Yep. I don't scan all. We cache flags, and we count from the cached flags to begin.

Great!  So, do you like the simplifications and reduced code duplication resulting from changing vee_rebuild_folder and vee_folder_remove_folder to use the same helpers as folder_changed_change, or would you prefer that I drop that part of the patch now that fixing the bug no longer requires it?

Matt, Im always scared of simplifications that introduce new bugs. Since we are speaking of stable branch, I would say just focus on the bug. May be for trunk, we can look at some simplification. Hope you got my point. Thanks.

I had commited my patch to stable/trunk. Can you put up a new thing? I'm planning for a 2.24.2.1 later this week, and I would love to take yours :-) Thanks

I'll send the revised patch tonight.  (Sorry, I've been busy for the past few days.)

Sure. Just make sure, we get it in time for 2.24.2.1. Thanks.

I have written the revised patch and am testing it.  I will post it as soon as I am satisfied.

Note: Two misbehaviors I noticed in the patched Evolution (the problem with the non-threaded "All mail" I mentioned in comment 17 and another problem with vfolder creation) were actually there before the patch.  I entered them as bug 564953 and bug 564954, respectively.

Err, irrespectively.

Created attachment 124907 [details] [review]
Non-invasive patch that doesn't deal with counts

This patch fixes the folder updating without making unnecessary major changes.  It doesn't deal with counts at all; in fact, it removes update_summary and COUNT_*_EXPR.

The folder updating seems to work, but I am seeing some problems with counts under the following procedure.  I start with no unread messages anywhere, and I go to a source folder containing a thread of two non-matching messages.  I set a label on the first message so it matches, mark the second unread, and /then/ switch to the vfolder.  Both messages appear in the vfolder, and the unread count in the gray bar to the left of "Show: All Messages" updates to 1, but the unread count in the folder tree fails to update.  This is an irritation, but I don't think it's grounds for delaying the committing of the patch.  My patch from comment 33 (which does its own count updating in addition to yours) handled this case correctly.

Been busy for sometime, I would look at it this week. Sorry Matt.

A second problem with your counts patch: When I drag-and-drop a message from my vfolder to a real folder, an unread count of 1 for the destination folder appears in the folder tree, even though no message in any of my folders is unread.

Separately, I noticed an infelicitous behavior with my patch.  My primary workflow is to read a message in the vfolder, unlabel it so it no longer matches the criteria, and "refresh" the vfolder so it disappears.  In Evolution 2.22 I "refreshed" by switching to another folder and back, but now that the real Refresh action (F5 key) works, I use that.  The problem is that a refresh won't remove the selected message.  I assume this behavior results from the patch for bug 467892, but I'm not sure whether to consider it a bug in that patch (which I would file) or in mine.  What do you think?

Srinivasa, please do fix the count problems when you get a chance.  I would try except that I really don't understand what's going on in your patch.  I do have a hunch, namely that vf_getv produces the correct answer each time it is called but is getting called at the wrong time.

There's a third problem case: when I move any single message from one physical folder to another, the unread counts of the Trash and some of my vfolders increase to 1, even though there are no unread messages anywhere.  (The counts do fix themselves if I empty the trash.)  This is particularly problematic for me because I like sent messages to go to my Inbox (so they can be threaded with received messages), so I have an outgoing filter that moves all messages to the Inbox, so the counts mess up every time I send a message.  I tried to work around this by disabling the filter and instead configuring all my accounts to deposit sent messages in the Inbox, but it made no difference.

Created attachment 125883 [details] [review]
Proposed patch

This should solve your count issues. Just try and lemme know.

The patch is no good.  I tested it on the latest code from the 2.24 branch with no other patches, and Evolution segfaults every time I try to send a message or save a draft.  Unfortunately, I'm having trouble getting a useful stack trace.  I'll post again after investigating further.

Created attachment 125903 [details] [review]
Patch for fixing issue #1

My bad, this fixes it. This fixes your first issue.

I was all the time, on local provider and forgot about others. This would take care of the count issue on the first case. I'm working on your second issue, but third I couldn't reproduce with this patch.. can you try with this patch?

I tested the patch from comment 59: there was no segfault and the first issue was fixed, but the second and third still occurred.  The third only affects vfolders that have the destination folder of the move as a source; be sure to try that case.

Please give your patches meaningful titles!  I find it hard to remember which patch does what when you have four different "Proposed patches".  Also, you could mark the second "Proposed patch" as committed and the third as obsolete rather than rejected.

Sorry on the title, updated it. I would keep it better..

Created attachment 126014 [details] [review]
Fixes issue #2 

I still can't reproduce your third issue. Any special sequence steps?

Your patch for issue #2 makes no difference for me.  I'm thinking issue #2 is really a special case of issue #3.  I can reproduce issue #3 via the following procedure starting from a new homedir:

1. Create a new vfolder called Important whose sole source is the Inbox.  Criterion "Label is Important", "Include threads" set to "All related".
2. Restart Evolution so the vfolder appears (bug 564953).
3. Create a new physical folder called "foo".
4. Post a message to "foo".
5. Label the message Important.
6. Drag the message to the Inbox.

The Important vfolder then shows an unread count of 1 in the folder tree, but the single message it contains is not unread.  I'm using the latest code from the 2.24 branch with my patch from comment #53 and your patches for issues #1 and #2.

Let me work on #3. But I think my #2 fix fixes part of your issue. Lemme commit all for todays 2.24.3 release.

Do you imply that you can reproduce #3?  That would be a relief to me.

Good luck finishing this in time for the release (although it doesn't make a big difference to me since I'll probably continue using the stable branch from SVN now that I have it set up).  The fixes we have so far don't seem to introduce any major problems, so I suppose there's no harm in committing them even if additional fixes are needed.

Committed all three patches to stable/trunk.

I haven't tried your steps yet. I would do it today or so. Just been busy on Novell stuff.

I reverted my #2 fix, Im not sure, the old code had that XEV headers forcefully stripped, and I thought it might add some confusions later.

Please remember to fix count issue #3.  The Evolution taskbar button is the main way I find out about new mail, so it distracts me when the button says "Inbox (1 unread, N total)" but there isn't really any new mail.

Matt, I will reproduce and fix #3. I have rolled out plan for 2.24.[3/4] http://mail.gnome.org/archives/release-team/2009-January/msg00116.html

I would fix it in time.

Matt, Im not able to reproduce #3. Could you create a sample vfolder and do the action, in steps for me? I think I miss your kind of vfolder or so.

Did you follow the steps in comment #64?  I just did a clean checkout and build of the latest Evolution 2.24 stable code from SVN, started from a new home directory, and followed those steps, and my Important vfolder shows an unread count of 1 after the last step.  If you want, I can make a screencast of the procedure with istanbul.

Here's the screencast (I didn't attach it because it's 2 MB):

http://mattmccutchen.net/private/evolution-557348-counts3.ogg

Thx, I can reproduce it now. I was onto trash earlier.

Created attachment 127439 [details] [review]
Fixes #3

Just try this out. It fixes for me.

Created attachment 127465 [details] [review]
Better fix for #3

Try this. This should fix it well.

I would push once you confirm to me.

Yes, the "Better fix for #3" works for me.  Since I started using Evolution with that patch, I have not seen any count issues.

The problem with removal of the selected message (comment 55) is now filed in bug 569765.  Once you commit the "Better fix for #3", I think we're done with this bug.

Fixed to stable/trunk