Merge pull request #3059 from k9mail/clean_up_sync_code

"Simplify" sync code
This commit is contained in:
Vincent Breitmoser 2018-01-08 16:22:16 +01:00 committed by GitHub
commit 111f7fc194
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
4 changed files with 98 additions and 234 deletions

View file

@ -741,12 +741,11 @@ public class MessagingController {
if (remoteMessageStore != null) {
remoteMessageStore.sync(account, folder, listener, providedRemoteFolder);
} else {
synchronizeMailboxSynchronousLegacy(account, folder, listener, providedRemoteFolder);
synchronizeMailboxSynchronousLegacy(account, folder, listener);
}
}
void synchronizeMailboxSynchronousLegacy(final Account account, final String folder, final MessagingListener listener,
Folder providedRemoteFolder) {
void synchronizeMailboxSynchronousLegacy(Account account, String folder, MessagingListener listener) {
Folder remoteFolder = null;
LocalFolder tLocalFolder = null;
@ -788,13 +787,8 @@ public class MessagingController {
tLocalFolder = localStore.getFolder(folder);
final LocalFolder localFolder = tLocalFolder;
localFolder.open(Folder.OPEN_MODE_RW);
localFolder.updateLastUid();
Map<String, Long> localUidMap = localFolder.getAllMessagesAndEffectiveDates();
if (providedRemoteFolder != null) {
Timber.v("SYNC: using providedRemoteFolder %s", folder);
remoteFolder = providedRemoteFolder;
} else {
Store remoteStore = account.getRemoteStore();
Timber.v("SYNC: About to get remote folder %s", folder);
@ -828,14 +822,8 @@ public class MessagingController {
*/
Timber.v("SYNC: About to open remote folder %s", folder);
if (Expunge.EXPUNGE_ON_POLL == account.getExpungePolicy()) {
Timber.d("SYNC: Expunging folder %s:%s", account.getDescription(), folder);
remoteFolder.expunge();
}
remoteFolder.open(Folder.OPEN_MODE_RO);
}
notificationController.clearAuthenticationErrorNotification(account, true);
/*
@ -1002,10 +990,7 @@ public class MessagingController {
System.currentTimeMillis());
} finally {
if (providedRemoteFolder == null) {
closeFolder(remoteFolder);
}
closeFolder(tLocalFolder);
}
@ -1125,12 +1110,6 @@ public class MessagingController {
final List<Message> largeMessages = new ArrayList<>();
final List<Message> smallMessages = new ArrayList<>();
if (!unsyncedMessages.isEmpty()) {
/*
* Reverse the order of the messages. Depending on the server this may get us
* fetch results for newest to oldest. If not, no harm done.
*/
Collections.sort(unsyncedMessages, new UidReverseComparator());
int visibleLimit = localFolder.getVisibleLimit();
int listSize = unsyncedMessages.size();
@ -1149,15 +1128,6 @@ public class MessagingController {
fetchUnsyncedMessages(account, remoteFolder, unsyncedMessages, smallMessages, largeMessages, progress, todo,
fp);
String updatedPushState = localFolder.getPushState();
for (Message message : unsyncedMessages) {
String newPushState = remoteFolder.getNewPushState(updatedPushState, message);
if (newPushState != null) {
updatedPushState = newPushState;
}
}
localFolder.setPushState(updatedPushState);
Timber.d("SYNC: Synced unsynced messages for folder %s", folder);
}
@ -1343,16 +1313,6 @@ public class MessagingController {
});
}
private boolean shouldImportMessage(final Account account, final Message message,
final Date earliestDate) {
if (account.isSearchByDateCapable() && message.olderThan(earliestDate)) {
Timber.d("Message %s is older than %s, hence not saving", message.getUid(), earliestDate);
return false;
}
return true;
}
private <T extends Message> void downloadSmallMessages(final Account account, final Folder<T> remoteFolder,
final LocalFolder localFolder,
List<T> smallMessages,
@ -1363,8 +1323,6 @@ public class MessagingController {
FetchProfile fp) throws MessagingException {
final String folder = remoteFolder.getName();
final Date earliestDate = account.getEarliestPollDate();
Timber.d("SYNC: Fetching %d small messages for folder %s", smallMessages.size(), folder);
remoteFolder.fetch(smallMessages,
@ -1373,12 +1331,6 @@ public class MessagingController {
public void messageFinished(final T message, int number, int ofTotal) {
try {
if (!shouldImportMessage(account, message, earliestDate)) {
progress.incrementAndGet();
return;
}
// Store the updated message locally
final LocalMessage localMessage = localFolder.storeSmallMessage(message, new Runnable() {
@Override
@ -1436,18 +1388,12 @@ public class MessagingController {
final int todo,
FetchProfile fp) throws MessagingException {
final String folder = remoteFolder.getName();
final Date earliestDate = account.getEarliestPollDate();
Timber.d("SYNC: Fetching large messages for folder %s", folder);
remoteFolder.fetch(largeMessages, fp, null);
for (T message : largeMessages) {
if (!shouldImportMessage(account, message, earliestDate)) {
progress.incrementAndGet();
continue;
}
if (message.getBody() == null) {
downloadSaneBody(account, remoteFolder, localFolder, message);
} else {

View file

@ -18,7 +18,6 @@ import com.fsck.k9.Account;
import com.fsck.k9.Account.Expunge;
import com.fsck.k9.AccountStats;
import com.fsck.k9.K9;
import com.fsck.k9.Preferences;
import com.fsck.k9.activity.MessageReference;
import com.fsck.k9.controller.MessagingController;
import com.fsck.k9.controller.MessagingListener;
@ -332,9 +331,7 @@ class ImapSync {
/*
* If the folder is a "special" folder we need to see if it exists
* on the remote server. It if does not exist we'll try to create it. If we
* can't create we'll abort. This will happen on every single Pop3 folder as
* designed and on Imap folders during error conditions. This allows us
* to treat Pop3 and Imap the same in this code.
* can't create we'll abort.
*/
private boolean verifyOrCreateRemoteSpecialFolder(Account account, String folder, Folder remoteFolder,
MessagingListener listener) throws MessagingException {
@ -420,11 +417,6 @@ class ImapSync {
final List<Message> largeMessages = new ArrayList<>();
final List<Message> smallMessages = new ArrayList<>();
if (!unsyncedMessages.isEmpty()) {
/*
* Reverse the order of the messages. Depending on the server this may get us
* fetch results for newest to oldest. If not, no harm done.
*/
Collections.sort(unsyncedMessages, new UidReverseComparator());
int visibleLimit = localFolder.getVisibleLimit();
int listSize = unsyncedMessages.size();
@ -434,9 +426,7 @@ class ImapSync {
}
FetchProfile fp = new FetchProfile();
if (remoteFolder.supportsFetchingFlags()) {
fp.add(FetchProfile.Item.FLAGS);
}
fp.add(FetchProfile.Item.ENVELOPE);
Timber.d("SYNC: About to fetch %d unsynced messages for folder %s", unsyncedMessages.size(), folder);
@ -503,23 +493,6 @@ class ImapSync {
});
}
// If the oldest message seen on this sync is newer than
// the oldest message seen on the previous sync, then
// we want to move our high-water mark forward
// this is all here just for pop which only syncs inbox
// this would be a little wrong for IMAP (we'd want a folder-level pref, not an account level pref.)
// fortunately, we just don't care.
Long oldestMessageTime = localFolder.getOldestMessageDate();
if (oldestMessageTime != null) {
Date oldestExtantMessage = new Date(oldestMessageTime);
if (oldestExtantMessage.before(downloadStarted) &&
oldestExtantMessage.after(new Date(account.getLatestOldMessageSeenTime()))) {
account.setLatestOldMessageSeenTime(oldestExtantMessage.getTime());
account.save(Preferences.getPreferences(context));
}
}
return newMessages.get();
}
@ -658,7 +631,7 @@ class ImapSync {
public void messageFinished(final T message, int number, int ofTotal) {
try {
if (!shouldImportMessage(account, message, earliestDate)) {
if (!shouldImportMessage(message, earliestDate)) {
progress.incrementAndGet();
return;
@ -728,7 +701,7 @@ class ImapSync {
remoteFolder.fetch(largeMessages, fp, null);
for (T message : largeMessages) {
if (!shouldImportMessage(account, message, earliestDate)) {
if (!shouldImportMessage(message, earliestDate)) {
progress.incrementAndGet();
continue;
}
@ -775,7 +748,6 @@ class ImapSync {
) throws MessagingException {
final String folder = remoteFolder.getName();
if (remoteFolder.supportsFetchingFlags()) {
Timber.d("SYNC: About to sync flags for %d remote messages for folder %s", syncFlagMessages.size(), folder);
FetchProfile fp = new FetchProfile();
@ -816,7 +788,6 @@ class ImapSync {
}
}
}
}
private void downloadSaneBody(Account account, Folder remoteFolder, LocalFolder localFolder, Message message)
throws MessagingException {
@ -840,28 +811,6 @@ class ImapSync {
localFolder.appendMessages(Collections.singletonList(message));
Message localMessage = localFolder.getMessage(message.getUid());
// Certain (POP3) servers give you the whole message even when you ask for only the first x Kb
if (!message.isSet(Flag.X_DOWNLOADED_FULL)) {
/*
* Mark the message as fully downloaded if the message size is smaller than
* the account's autodownload size limit, otherwise mark as only a partial
* download. This will prevent the system from downloading the same message
* twice.
*
* If there is no limit on autodownload size, that's the same as the message
* being smaller than the max size
*/
if (account.getMaximumAutoDownloadMessageSize() == 0
|| message.getSize() < account.getMaximumAutoDownloadMessageSize()) {
localMessage.setFlag(Flag.X_DOWNLOADED_FULL, true);
} else {
// Set a flag indicating that the message has been partially downloaded and
// is ready for view.
localMessage.setFlag(Flag.X_DOWNLOADED_PARTIAL, true);
}
}
}
private void downloadPartial(Folder remoteFolder, LocalFolder localFolder, Message message)
@ -913,10 +862,8 @@ class ImapSync {
return messageChanged;
}
private boolean shouldImportMessage(final Account account, final Message message,
final Date earliestDate) {
if (account.isSearchByDateCapable() && message.olderThan(earliestDate)) {
private boolean shouldImportMessage(Message message, Date earliestDate) {
if (message.olderThan(earliestDate)) {
Timber.d("Message %s is older than %s, hence not saving", message.getUid(), earliestDate);
return false;
}

View file

@ -616,9 +616,10 @@ public class MessagingControllerTest {
@Test
public void synchronizeMailboxSynchronousLegacy_withOneMessageInRemoteFolder_shouldFinishWithoutError()
throws Exception {
configureRemoteStoreWithFolder();
messageCountInRemoteFolder(1);
controller.synchronizeMailboxSynchronousLegacy(account, FOLDER_NAME, listener, remoteFolder);
controller.synchronizeMailboxSynchronousLegacy(account, FOLDER_NAME, listener);
verify(listener).synchronizeMailboxFinished(account, FOLDER_NAME, 1, 0);
}
@ -626,9 +627,10 @@ public class MessagingControllerTest {
@Test
public void synchronizeMailboxSynchronousLegacy_withEmptyRemoteFolder_shouldFinishWithoutError()
throws Exception {
configureRemoteStoreWithFolder();
messageCountInRemoteFolder(0);
controller.synchronizeMailboxSynchronousLegacy(account, FOLDER_NAME, listener, remoteFolder);
controller.synchronizeMailboxSynchronousLegacy(account, FOLDER_NAME, listener);
verify(listener).synchronizeMailboxFinished(account, FOLDER_NAME, 0, 0);
}
@ -636,9 +638,10 @@ public class MessagingControllerTest {
@Test
public void synchronizeMailboxSynchronousLegacy_withNegativeMessageCountInRemoteFolder_shouldFinishWithError()
throws Exception {
configureRemoteStoreWithFolder();
messageCountInRemoteFolder(-1);
controller.synchronizeMailboxSynchronousLegacy(account, FOLDER_NAME, listener, remoteFolder);
controller.synchronizeMailboxSynchronousLegacy(account, FOLDER_NAME, listener);
verify(listener).synchronizeMailboxFailed(account, FOLDER_NAME,
"Exception: Message count -1 for folder Folder");
@ -649,7 +652,7 @@ public class MessagingControllerTest {
throws Exception {
messageCountInRemoteFolder(1);
controller.synchronizeMailboxSynchronousLegacy(account, FOLDER_NAME, listener, remoteFolder);
controller.synchronizeMailboxSynchronousLegacy(account, FOLDER_NAME, listener);
verify(remoteFolder, never()).open(Folder.OPEN_MODE_RW);
}
@ -660,7 +663,7 @@ public class MessagingControllerTest {
messageCountInRemoteFolder(1);
configureRemoteStoreWithFolder();
controller.synchronizeMailboxSynchronousLegacy(account, FOLDER_NAME, listener, null);
controller.synchronizeMailboxSynchronousLegacy(account, FOLDER_NAME, listener);
verify(remoteFolder).open(Folder.OPEN_MODE_RO);
}
@ -669,7 +672,7 @@ public class MessagingControllerTest {
public void synchronizeMailboxSynchronousLegacy_withRemoteFolderProvided_shouldNotCloseRemoteFolder() throws Exception {
messageCountInRemoteFolder(1);
controller.synchronizeMailboxSynchronousLegacy(account, FOLDER_NAME, listener, remoteFolder);
controller.synchronizeMailboxSynchronousLegacy(account, FOLDER_NAME, listener);
verify(remoteFolder, never()).close();
}
@ -680,37 +683,15 @@ public class MessagingControllerTest {
messageCountInRemoteFolder(1);
configureRemoteStoreWithFolder();
controller.synchronizeMailboxSynchronousLegacy(account, FOLDER_NAME, listener, null);
controller.synchronizeMailboxSynchronousLegacy(account, FOLDER_NAME, listener);
verify(remoteFolder).close();
}
@Test
public void synchronizeMailboxSynchronousLegacy_withAccountPolicySetToExpungeOnPoll_shouldExpungeRemoteFolder()
throws Exception {
messageCountInRemoteFolder(1);
when(account.getExpungePolicy()).thenReturn(Account.Expunge.EXPUNGE_ON_POLL);
configureRemoteStoreWithFolder();
controller.synchronizeMailboxSynchronousLegacy(account, FOLDER_NAME, listener, null);
verify(remoteFolder).expunge();
}
@Test
public void synchronizeMailboxSynchronousLegacy_withAccountPolicySetToExpungeManually_shouldNotExpungeRemoteFolder()
throws Exception {
messageCountInRemoteFolder(1);
when(account.getExpungePolicy()).thenReturn(Account.Expunge.EXPUNGE_MANUALLY);
controller.synchronizeMailboxSynchronousLegacy(account, FOLDER_NAME, listener, null);
verify(remoteFolder, never()).expunge();
}
@Test
public void synchronizeMailboxSynchronousLegacy_withAccountSetToSyncRemoteDeletions_shouldDeleteLocalCopiesOfDeletedMessages()
throws Exception {
configureRemoteStoreWithFolder();
messageCountInRemoteFolder(0);
LocalMessage localCopyOfRemoteDeletedMessage = mock(LocalMessage.class);
when(account.syncRemoteDeletions()).thenReturn(true);
@ -718,7 +699,7 @@ public class MessagingControllerTest {
when(localFolder.getMessagesByUids(any(List.class)))
.thenReturn(Collections.singletonList(localCopyOfRemoteDeletedMessage));
controller.synchronizeMailboxSynchronousLegacy(account, FOLDER_NAME, listener, remoteFolder);
controller.synchronizeMailboxSynchronousLegacy(account, FOLDER_NAME, listener);
verify(localFolder).destroyMessages(messageListCaptor.capture());
assertEquals(localCopyOfRemoteDeletedMessage, messageListCaptor.getValue().get(0));
@ -735,7 +716,7 @@ public class MessagingControllerTest {
when(localMessage.olderThan(dateOfEarliestPoll)).thenReturn(false);
when(localFolder.getMessages(null)).thenReturn(Collections.singletonList(localMessage));
controller.synchronizeMailboxSynchronousLegacy(account, FOLDER_NAME, listener, remoteFolder);
controller.synchronizeMailboxSynchronousLegacy(account, FOLDER_NAME, listener);
verify(localFolder, never()).destroyMessages(messageListCaptor.capture());
}
@ -743,6 +724,7 @@ public class MessagingControllerTest {
@Test
public void synchronizeMailboxSynchronousLegacy_withAccountSetToSyncRemoteDeletions_shouldDeleteLocalCopiesOfExistingMessagesBeforeEarliestPollDate()
throws Exception {
configureRemoteStoreWithFolder();
messageCountInRemoteFolder(1);
LocalMessage localMessage = localMessageWithCopyOnServer();
Date dateOfEarliestPoll = new Date();
@ -752,7 +734,7 @@ public class MessagingControllerTest {
when(localFolder.getAllMessagesAndEffectiveDates()).thenReturn(Collections.singletonMap(MESSAGE_UID1, 0L));
when(localFolder.getMessagesByUids(any(List.class))).thenReturn(Collections.singletonList(localMessage));
controller.synchronizeMailboxSynchronousLegacy(account, FOLDER_NAME, listener, remoteFolder);
controller.synchronizeMailboxSynchronousLegacy(account, FOLDER_NAME, listener);
verify(localFolder).destroyMessages(messageListCaptor.capture());
assertEquals(localMessage, messageListCaptor.getValue().get(0));
@ -766,7 +748,7 @@ public class MessagingControllerTest {
when(account.syncRemoteDeletions()).thenReturn(false);
when(localFolder.getMessages(null)).thenReturn(Collections.singletonList(remoteDeletedMessage));
controller.synchronizeMailboxSynchronousLegacy(account, FOLDER_NAME, listener, remoteFolder);
controller.synchronizeMailboxSynchronousLegacy(account, FOLDER_NAME, listener);
verify(localFolder, never()).destroyMessages(messageListCaptor.capture());
}
@ -774,11 +756,12 @@ public class MessagingControllerTest {
@Test
public void synchronizeMailboxSynchronousLegacy_withAccountSupportingFetchingFlags_shouldFetchUnsychronizedMessagesListAndFlags()
throws Exception {
configureRemoteStoreWithFolder();
messageCountInRemoteFolder(1);
hasUnsyncedRemoteMessage();
when(remoteFolder.supportsFetchingFlags()).thenReturn(true);
controller.synchronizeMailboxSynchronousLegacy(account, FOLDER_NAME, listener, remoteFolder);
controller.synchronizeMailboxSynchronousLegacy(account, FOLDER_NAME, listener);
verify(remoteFolder, atLeastOnce()).fetch(any(List.class), fetchProfileCaptor.capture(),
any(MessageRetrievalListener.class));
@ -790,11 +773,12 @@ public class MessagingControllerTest {
@Test
public void synchronizeMailboxSynchronousLegacy_withAccountNotSupportingFetchingFlags_shouldFetchUnsychronizedMessages()
throws Exception {
configureRemoteStoreWithFolder();
messageCountInRemoteFolder(1);
hasUnsyncedRemoteMessage();
when(remoteFolder.supportsFetchingFlags()).thenReturn(false);
controller.synchronizeMailboxSynchronousLegacy(account, FOLDER_NAME, listener, remoteFolder);
controller.synchronizeMailboxSynchronousLegacy(account, FOLDER_NAME, listener);
verify(remoteFolder, atLeastOnce()).fetch(any(List.class), fetchProfileCaptor.capture(),
any(MessageRetrievalListener.class));
@ -806,12 +790,13 @@ public class MessagingControllerTest {
public void synchronizeMailboxSynchronousLegacy_withUnsyncedNewSmallMessage_shouldFetchBodyOfSmallMessage()
throws Exception {
Message smallMessage = buildSmallNewMessage();
configureRemoteStoreWithFolder();
messageCountInRemoteFolder(1);
hasUnsyncedRemoteMessage();
when(remoteFolder.supportsFetchingFlags()).thenReturn(false);
respondToFetchEnvelopesWithMessage(smallMessage);
controller.synchronizeMailboxSynchronousLegacy(account, FOLDER_NAME, listener, remoteFolder);
controller.synchronizeMailboxSynchronousLegacy(account, FOLDER_NAME, listener);
verify(remoteFolder, atLeast(2)).fetch(any(List.class), fetchProfileCaptor.capture(),
any(MessageRetrievalListener.class));
@ -823,12 +808,13 @@ public class MessagingControllerTest {
public void synchronizeMailboxSynchronousLegacy_withUnsyncedNewSmallMessage_shouldFetchStructureAndLimitedBodyOfLargeMessage()
throws Exception {
Message largeMessage = buildLargeNewMessage();
configureRemoteStoreWithFolder();
messageCountInRemoteFolder(1);
hasUnsyncedRemoteMessage();
when(remoteFolder.supportsFetchingFlags()).thenReturn(false);
respondToFetchEnvelopesWithMessage(largeMessage);
controller.synchronizeMailboxSynchronousLegacy(account, FOLDER_NAME, listener, remoteFolder);
controller.synchronizeMailboxSynchronousLegacy(account, FOLDER_NAME, listener);
//TODO: Don't bother fetching messages of a size we don't have
verify(remoteFolder, atLeast(4)).fetch(any(List.class), fetchProfileCaptor.capture(),

View file

@ -247,8 +247,7 @@ public class ImapSyncTest {
}
@Test
public void sync_withAccountSupportingFetchingFlags_shouldFetchUnsychronizedMessagesListAndFlags()
throws Exception {
public void sync_shouldFetchUnsynchronizedMessagesListAndFlags() throws Exception {
messageCountInRemoteFolder(1);
hasUnsyncedRemoteMessage();
when(remoteFolder.supportsFetchingFlags()).thenReturn(true);
@ -262,20 +261,6 @@ public class ImapSyncTest {
assertEquals(2, fetchProfileCaptor.getAllValues().get(0).size());
}
@Test
public void sync_withAccountNotSupportingFetchingFlags_shouldFetchUnsychronizedMessages() throws Exception {
messageCountInRemoteFolder(1);
hasUnsyncedRemoteMessage();
when(remoteFolder.supportsFetchingFlags()).thenReturn(false);
imapSync.sync(account, FOLDER_NAME, listener, remoteFolder);
verify(remoteFolder, atLeastOnce()).fetch(any(List.class), fetchProfileCaptor.capture(),
any(MessageRetrievalListener.class));
assertEquals(1, fetchProfileCaptor.getAllValues().get(0).size());
assertTrue(fetchProfileCaptor.getAllValues().get(0).contains(FetchProfile.Item.ENVELOPE));
}
@Test
public void sync_withUnsyncedNewSmallMessage_shouldFetchBodyOfSmallMessage() throws Exception {
Message smallMessage = buildSmallNewMessage();