Created attachment 62033 [details] [review]
e-d-s patch

Created attachment 62034 [details] [review]
evo patch

Created attachment 62035 [details] [review]
Older patch for e-d-s 1.2, for reference.

In e-d-s 1.2 it was different, and didn't need any changes in Evolution itself. The code for checking folders seems to have got messier since then.

this is not the way to do it. this patch sucks.

*sigh* If the patch sucks, help to come up with a new one, don't just close it as WONTFIX.  This is a valid problem not to be confused with what you may think is an invalid fix.

1. the patch would break current user setups as it changes the meaning of the use_lsub URI property.

2. the way this should be done is to have a folder property (accessable via File->Folder Properties or via right-click on folder) specifying whether or not the folder should be checked for new mail.

None of this hack-job stuff.

Hi Jeff, what do you think about this?
My patches below are partially based on your above comment, partially on a hint from mail-send-recv.c (see evo's patch), partially on patches above and with help from pvanhoof, which pointed me to right direction when I observed one issue with this (and after chat with him I realized that it is not an issue at all).

The patches below add new property for imap and exchange folders, "Always check for new mail in this folder" which is accessible via Folder->Properties, or in popup over such folder. When user doesn't have checked "Check for new messages in all folders", then it will determine if check or not check in a folder based on this new property.

The pity thing about this patch is that I did what was as a TODO in mail-send-recv.c, even not completely as was suggested there. I added new virtual function to the CamelStore, named camel_store_can_refresh_folder, which, by default, permit refresh of all Inbox folders. Each provider/store should determine if this is sufficient for him or not. For imap and exchange it is not sufficient.

Because I changed the size of CamelStore structure, then everything subclassing this structure (or any descendant) should be recompiled against new camel, otherwise one can see a warning like this on console:
(evolution:2596): camel-WARNING **: camel_type_register: 'CamelExchangeStore' has smaller class size than parent 'CamelOfflineStore'.

Created attachment 98764 [details] [review]
proposed eds patch

for evolution-data-server;

Created attachment 98765 [details] [review]
proposed evo patch

for evolution;

Created attachment 98766 [details] [review]
proposed eex patch

for evolution-exchange;

Fejj, you got some time to review this?

Sankar/Matt, I think this needs to be pushed asap to trunk. Can one of you review/approve this?

a couple of comments for the e-d-s patch (I'll review each patch in a new comment so I don't have to worry about ff crashing and losing all my comments):

+static gboolean
+can_refresh_folder (CamelStore *store, CamelFolderInfo *info, CamelException *ex)
+{
+	g_return_val_if_fail (store != NULL, FALSE);
+	g_return_val_if_fail (info != NULL, FALSE);
+

those g_return checks are not needed there as they are checked in the public API (camel_store_can_refresh_folder)

+static gboolean
+imap_can_refresh_folder (CamelStore *store, CamelFolderInfo *info, CamelException *ex)
+{
+	gboolean res;
+
+	g_return_val_if_fail (store != NULL, FALSE);
+	g_return_val_if_fail (info != NULL, FALSE);

these checks aren't needed either, for the same reason.

+	if (!res && CAMEL_EXCEPTION_NONE == camel_exception_get_id (ex)) {

evolution style would be:

if (!res && !camel_exception_is_set (ex))

fwiw, evolution style prefers that the variable is on the left and the value on the right, e.g: camel_exception_get_id (ex) == CAMEL_EXCEPTION_NONE

+			res = CAMEL_IMAP_FOLDER (folder)->check_folder != 0;

That could be simplified to:

res = CAMEL_IMAP_FOLDER (folder)->check_folder;

evolution patch review:

static void
get_folders (CamelStore *store, GPtrArray *folders, CamelFolderInfo *info)
{
	while (info) {
		CamelException ex;

		camel_exception_init (&ex);
		if (camel_store_can_refresh_folder (store, info, &ex))
			g_ptr_array_add (folders, g_strdup (info->uri));
		camel_exception_clear (&ex);

		get_folders (store, folders, info->child);
		info = info->next;
	}
}


I think this could be better such that you don't re-init CamelException more than you need to:

static void
get_folders (CamelStore *store, GPtrArray *folders, CamelFolderInfo *info)
{
	CamelException ex;
	
	camel_exception_init (&ex);
	
	while (info) {
		if (camel_store_can_refresh_folder (store, info, &ex))
			g_ptr_array_add (folders, g_strdup (info->uri));
		camel_exception_clear (&ex);

		get_folders (store, folders, info->child);
		info = info->next;
	}
}



camel_store_can_refresh_folder() has to instantiate a CamelFolder object for at least all IMAP folders (granted, it's shortcutted for local folders and nntp folders), so I wonder:

Would it be better to make this code work more like this?:

static void
get_folders (CamelStore *store, GPtrArray *folders, CamelFolderInfo *info)
{
	CamelException ex;
	
	camel_exception_init (&ex);
	
	while (info) {
		if ((folder = camel_store_get_folder (store, ..., &ex)) &&
		    camel_folder_should_refresh (folder))
			g_ptr_array_add (folders, folder);
		camel_exception_clear (&ex);

		get_folders (store, folders, info->child);
		info = info->next;
	}
}


The reason I wonder, is because camel_store_get_folder() could be an expensive operation, especially for IMAP. The problem with doing it my way, however, is that we definitely incur more overhead for local folders (as well as nntp, and nntp are probably no less expensive than IMAP and if there are a lot of them, this could be bad?).

Anyways... I don't really know the performance implications of either approach, just thoughts.

That said, I'm happy with Milan's current approach for now. I think that my approach would be better if/when Camel changes to support CamelFolder::open()/::close() functionality such that camel_store_get_folder() becomes really cheap, but until then - it might just be that Milan's approach would have less overall overhead.

evolution-exchange patch review:

same comments as for the e-d-s patch



That said, once Milan makes the cosmetic changes I suggested, I think it's ok to commit (barring any objections from the current mail maintainers, of course).

Thanks a lot for your review and thoughts Fejj.

David: have you tried these patches? Do they solve your problem? How about my performance concerns - are they actually a problem?

Created attachment 100705 [details] [review]
proposed eds patch ][

for evolution-data-server;

From my point of view, double check is better than no check. But as you wish.
The check_folder != 0, I thought about different type sizes, it was the only reason why used here in this way.

Created attachment 100706 [details] [review]
proposed evo patch ][

for evolution;

I do not see there any difference between your and mine solution (the folders and performance you mentioned), because the folder has been instantiated before and here we got one from cache, so no big performance overhead, I guess (I tested on IMAP, actually, and I didn't notice any difference).

Created attachment 100708 [details] [review]
proposed eex patch ][

for evolution-exchange;

> I do not see there any difference between your and mine solution (the folders
> and performance you mentioned), because the folder has been instantiated before
> and here we got one from cache, so no big performance overhead,

If that's the case, then it's not a problem which is good :)

Thanks Fejj. Milan please commit.

eds part committed to trunk. Committed revision 8288.
evo part committed to trunk. Committed revision 34688.
eex part committed to trunk. Committed revision 1517.

Sorry, I've only just got round to installing the latest Evolution and testing. 

How do I make it do the right thing, checking for mail only in the folders which are marked as 'active' on the IMAP server? The only thing I've worked out how to do is manually go through each folder with the mouse. On each computer from which I use Evolution. That's obviously not a viable solution.

I guess you've turned off "Check in all folders" in your account preferences, which is right. Now right click on the folder and choose in Properties "Always check for new mail in this folder". It works only for remote folders.

OK, thanks. This is useful progress but still not as useful as what I had before. Keeping the list in sync manually, with the mouse, on all instances of Evolution, isn't really what I had in mind. It's much easier to simply mark the folders as 'active' on the server and for that status to be interpreted by Evolution as 'should be checked for new mail'.

OK, reopening because of the discussion with David. As he reminded me, we have "Show only subscribed folders" option, thus in case this is unchecked, we can use the subscribed flag to distinguish whether update the folder or not.

There will be new option in "Receiving Options" tab "Check for new messages in subscribed folders only" and will be enabled only if not checked "in all folders" and not checked "Show only subscribed folders". The new option will be shown only when both options are shown (thus not in exchange, for example).

Created attachment 113793 [details] [review]
proposed eds patch

for evolution-data-server;

Hmm, this is available for IMAP only, and there is no way to have there such nice enabling of items as I describe in previous comment, so you can check them all or none or any as you wish. Can be confusing a bit, but not so much I guess.

Seems fine to me. Announce string/ui change.

Committed to trunk. Committed revision 9202.

As I was told by abharath, changes here make evo startup awfully slow for new accounts (no local cache) when the account has unchecked "check for new message in all folders". It's very visible with eex.

To not reopen this one, I'll try to fix it in bug #558883

*** Bug 481247 has been marked as a duplicate of this bug. ***