Bug 151916 - [PATCH]dimap folder uploads status of all messages on sync
Summary: [PATCH]dimap folder uploads status of all messages on sync
Status: RESOLVED FIXED
Alias: None
Product: kmail
Classification: Unmaintained
Component: disconnected IMAP (show other bugs)
Version: 1.9.7
Platform: unspecified Linux
: NOR normal
Target Milestone: ---
Assignee: kdepim bugs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-11-06 06:29 UTC by bonne
Modified: 2009-03-04 15:21 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments
Patch to only upload status of changed messages (3.29 KB, patch)
2007-11-07 00:23 UTC, bonne
Details
Revised verson of Bonne's patch working on UIDs (6.52 KB, patch)
2008-03-13 00:30 UTC, Jim Hague
Details
Patch to only upload status of changed messages (for 4.2) (6.38 KB, patch)
2009-02-27 03:56 UTC, bonne
Details

Note You need to log in before you can comment on or make changes to this bug.
Description bonne 2007-11-06 06:29:25 UTC
Version:           1.9.7 (using KDE 3.5.7, Gentoo)
Compiler:          Target: i686-pc-linux-gnu
OS:                Linux (i686) release 2.6.22-gentoo-r5

This one is fairly simple. When synchronising a disconnected imap folder with the imap server ALL of the message flags are uploaded to the server (not just the ones which are changed). This leads to very slow synchronisation (and occupies the majority of time for my particular setup).

I'm currently using the gmail IMAP service. 

I think one simple solution would be to store somewhere, for each message, whether it's status has changed since the last sync, and only upload the statuses of these messages. 

A side effect of this bug is that during the synchronisation messages get marked as "unread" on the server (I think until the "read" status flag is sent).
Comment 1 bonne 2007-11-06 06:56:00 UTC
One way I can think to make it faster is this: Instead of storing mStatusChangedLocally for each folder (look in kmfoldercachedimap.cpp) as a boolean stating whether the folder has _ANY_ statuses changed, we could store an array of the uids that are changed so that when uploadFlags() is called we can only look at those particular uids. 

Comment 2 bonne 2007-11-07 00:23:44 UTC
Created attachment 22009 [details]
Patch to only upload status of changed messages

I briefly tested this and it appears to work as expected. The debugging
messages confirm that only the status of the changed messages is uploaded.

It should probably be changed to store the UID rather than just the index in
the message folder because that will be the same after new messages arrive.
Comment 3 Ryan Winter 2007-11-26 13:34:22 UTC
I tested the patch from Comment #2 on my system and it appears to work. Thanks Bonne!
Comment 4 Ryan Winter 2007-11-26 13:35:09 UTC
*** This bug has been confirmed by popular vote. ***
Comment 5 Tobias Niwi 2008-01-26 11:58:56 UTC
I would like to use the patch in comment #2. But I don't know how to install it. It seems to be be portage specific (see the first two lines). Could someone write a short note how to use the patch with other distributions? I am a user of openSUSE and Kubuntu Feisty.

Thanks a lot,
niwi
Comment 6 Tobias Niwi 2008-01-27 00:33:16 UTC
I have written earlier today about my problems to install the patch in comment#2. I think I finally succeeded in installing the patch but now I get an error message while compiling the patched source code. Since I don't know if I patched the source code the right way I can't say for sure if this is a problem caused by the patch or me. But what I can say for sure, is that the error is not caused by the source code or the way I try to compile it. I have no problems to compile the unpatched source code.

Here is what I did:
1. Because the header information for kmfoldercachedimap.cpp and kmfoldercachedimap.h in the patch seem to be hard coded for a portage system I removed them and split the patch in two files. Each splitted part included the information for one of two mentioned files to patch.

2. Then I installed the patches with:
# patch ~/kdepim-3.5.8/kmail/kmfoldercachedimap.cpp ~/patch1.diff'
# patch ~/kdepim-3.5.8/kmail/kmfoldercachedimap.h ~/patch2.diff

With patch 1 (for kmfoldercachedimap.cpp) I got the following error messages:

missing header for unified diff at line 1 of patch
patching file /home/niwi/Werkstatt/kdepim-3.5.8/kmail/kmfoldercachedimap.cpp
Hunk #1 succeeded at 189 (offset 9 lines).
Hunk #2 succeeded at 277 (offset 12 lines).
Hunk #3 FAILED at 857.
Hunk #4 succeeded at 1430 (offset 47 lines).
Hunk #5 succeeded at 1531 (offset 92 lines).
patch unexpectedly ends in middle of line
Hunk #6 succeeded at 1843 with fuzz 1 (offset 110 lines).
1 out of 6 hunks FAILED -- saving rejects to file /home/niwi/Werkstatt/kdepim-3.5.8/kmail/kmfoldercachedimap.cpp.rej

I patched hunk #3 manually. The reason for the problem is a missing ")" in line 29,80 of the patch. I checked hunk #5 and 6 and they got patched correctly.

After I installed the patches I did a successful './configure' and started 'make'. 

Here is the error message I finally got:

kmfoldercachedimap.cpp: In member function 'void KMFolderCachedImap::rememberDel                       etion(int)':
kmfoldercachedimap.cpp:495: warning: comparison of unsigned expression >= 0 is a                       lways true
kmfoldercachedimap.cpp: In member function 'void KMFolderCachedImap::serverSyncI                       nternal()':
kmfoldercachedimap.cpp:897: error: could not convert '((KMFolderCachedImap*)this                       )->KMFolderCachedImap::mStatusChangedLocally' to 'bool'
make[3]: *** [kmfoldercachedimap.lo] Error 1
make[3]: Leaving directory `/home/niwi/Werkstatt/kdepim-3.5.8/kmail'
make[2]: *** [all-recursive] Error 1
make[2]: Leaving directory `/home/niwi/Werkstatt/kdepim-3.5.8/kmail'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/home/niwi/Werkstatt/kdepim-3.5.8'
make: *** [all] Error 2


I guess there is a problem with the patch (or me...?).

Any idea what I could do?
Comment 7 bonne 2008-01-27 03:10:31 UTC
Tobias, it looks like you're trying to use the patch against version 3.5.8. The patch was written for version 3.5.7, so it may not work for 3.5.8. It might take a little bit of work to get it to work for the later version if there have been other changes to the code in that area. 
Comment 8 Tobias Niwi 2008-01-27 11:07:05 UTC
Bonne, thank you for the quick response. Yes, I use KDE 3.5.8. However, 3.5.8 includes KMail 1.9.7, which is the version reported for this bug report. Is it still likely that 3.5.8 reason why I can't compile? Or was the patch made for an earlier version of KMail?
Comment 9 Till Adam 2008-02-17 16:38:04 UTC
Bonne, your basic idea is sound, although the use of the indeces is dangerous, as you've noted yourself. UIDs should be fine, though, as long as the list is "forgotten" when uid validity changes. Do you think you can adjust your patch that way? If so, I think we can integrate it. Thanks for the contribution, in any case. If you need help, we hang out in #kontact on freenode, or you can post to the kdepim mailing list.
Comment 10 Jim Hague 2008-03-13 00:30:56 UTC
Created attachment 23883 [details]
Revised verson of Bonne's patch working on UIDs

I've revised Bonne's patch for KDE 3.5.9 (kmail 1.9.9) taken from Debian
Unstable. This patch stores UIDs rather than index numbers. It works as
expected (checked by recording protocol traffic) and makes a big difference in
my download time. Thanks Bonne.
Comment 11 Till Adam 2008-03-13 07:40:54 UTC
Excellent, Jim. I'll have our QA team test the patch on enterprise branch and provided it doesn't cause any regressions, we'll push it into 3.5 branch as well. Thanks a lot for the contribution, this bug is really Free Software at its best, I think. :)
Comment 12 bonne 2008-05-05 03:35:30 UTC
Thanks Jim, I've been overseas away from my dev computer for ages so couldn't get back to this patch. Good to see you picked up my slack, thanks.
Comment 13 Vincent MEURISSE 2008-11-11 12:00:40 UTC
I got the same bug Kmail 1.10.3 under KDE 4.1.3.
I use KDE from Debian experimental on a Lenny installation.
This is really annoying as I have more than 30000 mails and the process take something like 30 min. 
Comment 14 bonne 2009-02-27 01:17:45 UTC
KMail 1.11.0 in KDE 4.2 still has this bug.
Comment 15 Thomas McGuire 2009-02-27 01:30:42 UTC
> KMail 1.11.0 in KDE 4.2 still has this bug.

Looks like the patch was never applied.
Can you change that patch so that it applies against KMail 1.11.0 and send the result to reviewboard.kde.org (kdepim group)? That way, the patch will not get lost.
Comment 16 bonne 2009-02-27 03:54:40 UTC
I have created a review request here: http://reviewboard.kde.org/r/199/ with a new patch which applies against trunk.
Comment 17 bonne 2009-02-27 03:56:11 UTC
Created attachment 31662 [details]
 Patch to only upload status of changed messages (for 4.2)
Comment 18 Thomas McGuire 2009-03-02 14:35:30 UTC
SVN commit 934167 by tmcguire:

Only update the status of changed messages when syncing cached imap folders,
to speed up folder syncing by a large amount for people with many messages
in their folders.

Thanks to Bonne Eggleston and Jim Hague for the patches!

I slightly modified the patch for config compatibility.

Bonne, please close the review request as submitted.
 
http://reviewboard.kde.org/r/199/
CCMAIL: bonne@ixum.net
BUG: 151916


 M  +42 -11    kmfoldercachedimap.cpp  
 M  +11 -5     kmfoldercachedimap.h  


WebSVN link: http://websvn.kde.org/?view=rev&revision=934167
Comment 19 Thomas McGuire 2009-03-04 15:21:26 UTC
SVN commit 935085 by tmcguire:

Backport r934167 by tmcguire from trunk to the 4.2 branch:

Only update the status of changed messages when syncing cached imap folders,
to speed up folder syncing by a large amount for people with many messages
in their folders.

Thanks to Bonne Eggleston and Jim Hague for the patches!

I slightly modified the patch for config compatibility.

Bonne, please close the review request as submitted.
 
http://reviewboard.kde.org/r/199/
CCMAIL: bonne@ixum.net
CCBUG: 151916



 M  +42 -11    kmfoldercachedimap.cpp  
 M  +11 -5     kmfoldercachedimap.h  


WebSVN link: http://websvn.kde.org/?view=rev&revision=935085