Created attachment 80359 [details] [review]
Patch that improves the behaviour

Please do review this patch, I haven't yet tested it with Evolution

This one needs these lines added to message_from_data:

	mi = (CamelImapMessageInfo *)camel_folder_summary_info_new_from_message (folder->summary, msg);

	camel_object_unref (CAMEL_OBJECT (msg))
+	size = GPOINTER_TO_INT (g_datalist_get_data (&data, "RFC822.SIZE"));
+	if (size)
+		mi->info.size = size;

if ((idate = g_datalist_get_data (&data, "INTERNALDATE")))
		mi->info.date_received = decode_internaldate ((const unsigned char *) idate);

And this change in imap_update_summary:

{
			uidset = imap_uid_array_to_set (folder->summary, needheaders, uid, UID_SET_LIMIT, &uid);

			if (!camel_imap_command_start (store, folder, ex,
-						       "UID FETCH %s (FLAGS INTERNALDATE BODY.PEEK[%s])",
+						       "UID FETCH %s (FLAGS RFC822.SIZE INTERNALDATE BODY.PEEK[%s])",

						       uidset, header_spec)) 
			{
				g_warning ("IMAP error getting headers (1)");

On other words, the RFC822.SIZE thingy must be re-added, else will camel_message_info_size return the size of the headers, not the entire message.

this is wrong:

+					  flags = GPOINTER_TO_INT (g_datalist_get_data (&data, "FLAGS"));
+					  if (flags) 
+					  {
+						mi->server_flags = flags;
+						mi->info.flags |= flags;
+						flags_to_label(folder, mi);
+					  }


you don't conserve the workarounds for Exchange, etc IMAP servers...

and I fail to see how this is an improvement over the current code in any way?

Yes, I didn't figure out which headers are exactly needed for Evolution to work correctly. The bandwidth improvement is mostly due limiting those.

I have already improved it a little bit more by using "UID FETCH %s:* (UID)" in stead of "UID FETCH %s:* (FLAGS)" for the first one (not asking for the flags when they aren't needed at that point). This is not in this patch I think.

The improvement (for Evolution) is that the summary can be written each 1000th header. So in case of a cancellation, the already retrieved information is stored already. With the mmap work this saving of the summary also moves memory from heap to the mmap. Hence reducing memory consumption (I know this depends on whether you consider a read-only mmap to be equally to heap, which I don't).

Another improvement is that the retrieved information is not kept in a GPtrArray  and/or GData instances, but rather freed while retrieving. The original valgrind report looked like a huge triagle. Consuming up to 60 MB for > 25,000 headers and then suddenly dropping to a full deallocation of that. Which is a totally unacceptable memory consumption profile for mobile devices (try it).

A future improvement will be that I'll ask for the ENVELOPE rather than using the BODY.SEEK trick.

As for the Exchange workarounds, I guess I'll have to test this. With the mmap work it no longer matters that there are two messages with the same uid. When looking the summary item up using the uid, it will simply always return the last one (this, I tested). So that's why I didn't see the need to duplicate items if the uid isn't unique. But I might have misinterpreted the workaround.

Anyway, duplicating the message in the summary doesn't sound like the right solution to me (I was rather confused when I saw the workaround, wondering how it's a workaround rather than an introduction of a defect that will confuse the user too).

Also note that if "mi" was NULL, in the patched version, that it's simply not added (so there's no need to fix it?).


I don't know ... I'm just not convinced that the current workaround is a fix.


Anyway, given that I might still do the ENVELOPE imlpementation, you might want to wait with bringing this patch to Evolution's Camel.

Closing as OBSOLETE since this was filed against the legacy IMAP backend which was removed in version 3.8.