From d5bb4629177fe7e6ed22fdf6515424b1f7ad1640 Mon Sep 17 00:00:00 2001 From: cketti Date: Thu, 6 Dec 2012 05:25:41 +0100 Subject: [PATCH] Don't create Message objects when changing flags of selected messages This changes the interface to MessagingController and the way flags are updated in the database. Now messages aren't changed one by one but in batches of 500. This should give better performance, but breaks the unread and flagged count. I'm not very sad about this, because now we can move towards only displaying the number of unread/flagged messages in the local database. --- src/com/fsck/k9/activity/MessageList.java | 2 +- .../k9/controller/MessagingController.java | 80 +++-- .../fsck/k9/fragment/MessageListFragment.java | 127 +++++--- src/com/fsck/k9/mail/store/LocalStore.java | 276 ++++++++++++++++++ 4 files changed, 425 insertions(+), 60 deletions(-) diff --git a/src/com/fsck/k9/activity/MessageList.java b/src/com/fsck/k9/activity/MessageList.java index 00060f40c..0153d6eb7 100644 --- a/src/com/fsck/k9/activity/MessageList.java +++ b/src/com/fsck/k9/activity/MessageList.java @@ -297,7 +297,7 @@ public class MessageList extends K9FragmentActivity implements MessageListFragme return true; } case KeyEvent.KEYCODE_G: { - mMessageListFragment.onToggleFlag(); + mMessageListFragment.onToggleFlagged(); return true; } case KeyEvent.KEYCODE_M: { diff --git a/src/com/fsck/k9/controller/MessagingController.java b/src/com/fsck/k9/controller/MessagingController.java index d23d5799c..4d59531d5 100644 --- a/src/com/fsck/k9/controller/MessagingController.java +++ b/src/com/fsck/k9/controller/MessagingController.java @@ -14,6 +14,7 @@ import java.util.Iterator; import java.util.LinkedList; import java.util.List; import java.util.Map; +import java.util.Map.Entry; import java.util.Set; import java.util.concurrent.*; import java.util.concurrent.atomic.AtomicBoolean; @@ -2559,39 +2560,65 @@ public class MessagingController implements Runnable { processPendingCommands(account); } - public void setFlag(final List messages, final Flag flag, final boolean newState) { + public void setFlag(final Account account, final List messageIds, final Flag flag, + final boolean newState, final boolean threadedList) { - actOnMessages(messages, new MessageActor() { + threadPool.execute(new Runnable() { @Override - public void act(final Account account, final Folder folder, - final List accountMessages) { - - setFlag(account, folder.getName(), accountMessages.toArray(EMPTY_MESSAGE_ARRAY), flag, - newState); + public void run() { + setFlagSynchronous(account, messageIds, flag, newState, threadedList); } }); } - public void setFlagForThreads(final List messages, final Flag flag, - final boolean newState) { + private void setFlagSynchronous(final Account account, final List messageIds, + final Flag flag, final boolean newState, final boolean threadedList) { - actOnMessages(messages, new MessageActor() { - @Override - public void act(final Account account, final Folder folder, - final List accountMessages) { + LocalStore localStore; + try { + localStore = account.getLocalStore(); + } catch (MessagingException e) { + Log.e(K9.LOG_TAG, "Couldn't get LocalStore instance", e); + return; + } - try { - List messagesInThreads = collectMessagesInThreads(account, - accountMessages); + // Update affected messages in the database. This should be as fast as possible so the UI + // can be updated with the new state. + try { + localStore.setFlag(messageIds, flag, newState, threadedList); + } catch (MessagingException e) { + Log.e(K9.LOG_TAG, "Couldn't set flags in local database", e); + } - setFlag(account, folder.getName(), - messagesInThreads.toArray(EMPTY_MESSAGE_ARRAY), flag, newState); + // Notify listeners of changed folder status +// for (MessagingListener l : getListeners()) { +// l.folderStatusChanged(account, folderName, localFolder.getUnreadMessageCount()); +// } - } catch (MessagingException e) { - addErrorMessage(account, "Something went wrong in setFlagForThreads()", e); - } + // Read folder name and UID of messages from the database + Map> folderMap; + try { + folderMap = localStore.getFoldersAndUids(messageIds, threadedList); + } catch (MessagingException e) { + Log.e(K9.LOG_TAG, "Couldn't get folder name and UID of messages", e); + return; + } + + // Loop over all folders + for (Entry> entry : folderMap.entrySet()) { + String folderName = entry.getKey(); + + // The error folder is always a local folder + // TODO: Skip the remote part for all local-only folders + if (account.getErrorFolderName().equals(folderName)) { + continue; } - }); + + // Send flag change to server + String[] uids = entry.getValue().toArray(EMPTY_STRING_ARRAY); + queueSetFlag(account, folderName, Boolean.toString(newState), flag.toString(), uids); + processPendingCommands(account); + } } /** @@ -2848,15 +2875,18 @@ public class MessagingController implements Runnable { false, true)) { if (account.isMarkMessageAsReadOnView() && !message.isSet(Flag.SEEN)) { message.setFlag(Flag.SEEN, true); - setFlag(Collections.singletonList((Message) message), - Flag.SEEN, true); + + setFlagSynchronous(account, Collections.singletonList(Long.valueOf(message.getId())), Flag.SEEN, true, false); +// setFlag(Collections.singletonList((Message) message), +// Flag.SEEN, true); } } return; } if (!message.isSet(Flag.SEEN)) { message.setFlag(Flag.SEEN, true); - setFlag(Collections.singletonList((Message) message), Flag.SEEN, true); + setFlagSynchronous(account, Collections.singletonList(Long.valueOf(message.getId())), Flag.SEEN, true, false); +// setFlag(Collections.singletonList((Message) message), Flag.SEEN, true); } for (MessagingListener l : getListeners(listener)) { diff --git a/src/com/fsck/k9/fragment/MessageListFragment.java b/src/com/fsck/k9/fragment/MessageListFragment.java index 8c5462fa4..9bd081b9a 100644 --- a/src/com/fsck/k9/fragment/MessageListFragment.java +++ b/src/com/fsck/k9/fragment/MessageListFragment.java @@ -1257,22 +1257,25 @@ public class MessageListFragment extends SherlockFragment implements OnItemClick public boolean onContextItemSelected(android.view.MenuItem item) { AdapterContextMenuInfo info = (AdapterContextMenuInfo) item.getMenuInfo(); int adapterPosition = listViewToAdapterPosition(info.position); - Message message = getMessageAtPosition(adapterPosition); switch (item.getItemId()) { case R.id.reply: { + Message message = getMessageAtPosition(adapterPosition); onReply(message); break; } case R.id.reply_all: { + Message message = getMessageAtPosition(adapterPosition); onReplyAll(message); break; } case R.id.forward: { + Message message = getMessageAtPosition(adapterPosition); onForward(message); break; } case R.id.send_again: { + Message message = getMessageAtPosition(adapterPosition); onResendMessage(message); mSelectedCount = 0; break; @@ -1286,40 +1289,45 @@ public class MessageListFragment extends SherlockFragment implements OnItemClick break; } case R.id.delete: { + Message message = getMessageAtPosition(adapterPosition); onDelete(message); break; } case R.id.mark_as_read: { - setFlag(message, Flag.SEEN, true); + setFlag(adapterPosition, Flag.SEEN, true); break; } case R.id.mark_as_unread: { - setFlag(message, Flag.SEEN, false); + setFlag(adapterPosition, Flag.SEEN, false); break; } case R.id.flag: { - setFlag(message, Flag.FLAGGED, true); + setFlag(adapterPosition, Flag.FLAGGED, true); break; } case R.id.unflag: { - setFlag(message, Flag.FLAGGED, false); + setFlag(adapterPosition, Flag.FLAGGED, false); break; } // only if the account supports this case R.id.archive: { + Message message = getMessageAtPosition(adapterPosition); onArchive(message); break; } case R.id.spam: { + Message message = getMessageAtPosition(adapterPosition); onSpam(message); break; } case R.id.move: { + Message message = getMessageAtPosition(adapterPosition); onMove(message); break; } case R.id.copy: { + Message message = getMessageAtPosition(adapterPosition); onCopy(message); break; } @@ -1968,19 +1976,59 @@ public class MessageListFragment extends SherlockFragment implements OnItemClick mActionModeCallback.showFlag(isBatchFlag); } - private void setFlag(Message message, final Flag flag, final boolean newState) { - setFlag(Collections.singletonList(message), flag, newState); - } - - private void setFlag(List messages, final Flag flag, final boolean newState) { - if (messages.size() == 0) { + private void setFlag(int adapterPosition, final Flag flag, final boolean newState) { + if (adapterPosition == AdapterView.INVALID_POSITION) { return; } - if (mThreadedList) { - mController.setFlagForThreads(messages, flag, newState); - } else { - mController.setFlag(messages, flag, newState); + Cursor cursor = (Cursor) mAdapter.getItem(adapterPosition); + Account account = mPreferences.getAccount(cursor.getString(ACCOUNT_UUID_COLUMN)); + long id = cursor.getLong(ID_COLUMN); + + mController.setFlag(account, Collections.singletonList(Long.valueOf(id)), flag, newState, + mThreadedList); + + computeBatchDirection(); + } + + private void setFlagForSelected(final Flag flag, final boolean newState) { + if (mSelected.size() == 0) { + return; + } + + Map> accountMapping = new HashMap>(); + Set accounts = new HashSet(); + + for (int position = 0, end = mAdapter.getCount(); position < end; position++) { + Cursor cursor = (Cursor) mAdapter.getItem(position); + long uniqueId = cursor.getLong(mUniqueIdColumn); + + if (mSelected.contains(uniqueId)) { + String uuid = cursor.getString(ACCOUNT_UUID_COLUMN); + Account account = mPreferences.getAccount(uuid); + + accounts.add(account); + List messageIdList = accountMapping.get(account); + if (messageIdList == null) { + messageIdList = new ArrayList(); + accountMapping.put(account, messageIdList); + } + + long selectionId; + if (mThreadedList) { + selectionId = (cursor.isNull(THREAD_ROOT_COLUMN)) ? + cursor.getLong(ID_COLUMN) : cursor.getLong(THREAD_ROOT_COLUMN); + } else { + selectionId = cursor.getLong(ID_COLUMN); + } + + messageIdList.add(selectionId); + } + } + + for (Account account : accounts) { + List messageIds = accountMapping.get(account); + mController.setFlag(account, messageIds, flag, newState, mThreadedList); } computeBatchDirection(); @@ -2408,8 +2456,6 @@ public class MessageListFragment extends SherlockFragment implements OnItemClick @Override public boolean onActionItemClicked(ActionMode mode, MenuItem item) { - List messages = getCheckedMessages(); - /* * In the following we assume that we can't move or copy * mails to the same folder. Also that spam isn't available if we are @@ -2419,26 +2465,25 @@ public class MessageListFragment extends SherlockFragment implements OnItemClick */ switch (item.getItemId()) { case R.id.delete: { + List messages = getCheckedMessages(); onDelete(messages); - - //FIXME mSelectedCount = 0; break; } case R.id.mark_as_read: { - setFlag(messages, Flag.SEEN, true); + setFlagForSelected(Flag.SEEN, true); break; } case R.id.mark_as_unread: { - setFlag(messages, Flag.SEEN, false); + setFlagForSelected(Flag.SEEN, false); break; } case R.id.flag: { - setFlag(messages, Flag.FLAGGED, true); + setFlagForSelected(Flag.FLAGGED, true); break; } case R.id.unflag: { - setFlag(messages, Flag.FLAGGED, false); + setFlagForSelected(Flag.FLAGGED, false); break; } case R.id.select_all: { @@ -2448,21 +2493,25 @@ public class MessageListFragment extends SherlockFragment implements OnItemClick // only if the account supports this case R.id.archive: { + List messages = getCheckedMessages(); onArchive(messages); mSelectedCount = 0; break; } case R.id.spam: { + List messages = getCheckedMessages(); onSpam(messages); mSelectedCount = 0; break; } case R.id.move: { + List messages = getCheckedMessages(); onMove(messages); mSelectedCount = 0; break; } case R.id.copy: { + List messages = getCheckedMessages(); onCopy(messages); mSelectedCount = 0; break; @@ -2608,6 +2657,11 @@ public class MessageListFragment extends SherlockFragment implements OnItemClick return getMessageAtPosition(adapterPosition); } + private int getAdapterPositionForSelectedMessage() { + int listViewPosition = mListView.getSelectedItemPosition(); + return listViewToAdapterPosition(listViewPosition); + } + private Message getMessageAtPosition(int adapterPosition) { if (adapterPosition == AdapterView.INVALID_POSITION) { return null; @@ -2654,11 +2708,23 @@ public class MessageListFragment extends SherlockFragment implements OnItemClick toggleMessageSelect(mListView.getSelectedItemPosition()); } - public void onToggleFlag() { - Message message = getSelectedMessage(); - if (message != null) { - setFlag(message, Flag.FLAGGED, !message.isSet(Flag.FLAGGED)); + public void onToggleFlagged() { + onToggleFlag(Flag.FLAGGED, FLAGGED_COLUMN); + } + + public void onToggleRead() { + onToggleFlag(Flag.SEEN, READ_COLUMN); + } + + private void onToggleFlag(Flag flag, int flagColumn) { + int adapterPosition = getAdapterPositionForSelectedMessage(); + if (adapterPosition == ListView.INVALID_POSITION) { + return; } + + Cursor cursor = (Cursor) mAdapter.getItem(adapterPosition); + boolean flagState = (cursor.getInt(flagColumn) == 1); + setFlag(adapterPosition, flag, !flagState); } public void onMove() { @@ -2682,13 +2748,6 @@ public class MessageListFragment extends SherlockFragment implements OnItemClick } } - public void onToggleRead() { - Message message = getSelectedMessage(); - if (message != null) { - setFlag(message, Flag.SEEN, !message.isSet(Flag.SEEN)); - } - } - public boolean isOutbox() { return (mFolderName != null && mFolderName.equals(mAccount.getOutboxFolderName())); } diff --git a/src/com/fsck/k9/mail/store/LocalStore.java b/src/com/fsck/k9/mail/store/LocalStore.java index 34c12ffe1..cde4941ac 100644 --- a/src/com/fsck/k9/mail/store/LocalStore.java +++ b/src/com/fsck/k9/mail/store/LocalStore.java @@ -111,6 +111,13 @@ public class LocalStore extends Store implements Serializable { */ private static final int UID_CHECK_BATCH_SIZE = 500; + /** + * Number of messages to perform flag updates at once. + * + * @see #setFlag(List, Flag, boolean, boolean) + */ + private static final int FLAG_UPDATE_BATCH_SIZE = 500; + public static final int DB_VERSION = 46; protected String uUid = null; @@ -4092,4 +4099,273 @@ public class LocalStore extends Store implements Serializable { Uri uri = Uri.withAppendedPath(EmailProvider.CONTENT_URI, "account/" + uUid + "/messages"); mContentResolver.notifyChange(uri, null); } + + /** + * Split database operations with a large set of arguments into multiple SQL statements. + * + *

+ * At the time of this writing (2012-12-06) SQLite only supports around 1000 arguments. That's + * why we have to split SQL statements with a large set of arguments into multiple SQL + * statements each working on a subset of the arguments. + *

+ * + * @param selectionCallback + * Supplies the argument set and the code to query/update the database. + * @param batchSize + * The maximum size of the selection set in each SQL statement. + * + * @throws MessagingException + */ + public void doBatchSetSelection(final BatchSetSelection selectionCallback, final int batchSize) + throws MessagingException { + + final List selectionArgs = new ArrayList(); + int start = 0; + + while (start < selectionCallback.getListSize()) { + final StringBuilder selection = new StringBuilder(); + + selection.append(" IN ("); + + int count = Math.min(selectionCallback.getListSize() - start, batchSize); + + for (int i = start, end = start + count; i < end; i++) { + if (i > start) { + selection.append(",?"); + } else { + selection.append("?"); + } + + selectionArgs.add(selectionCallback.getListItem(i)); + } + + selection.append(")"); + + try { + database.execute(true, new DbCallback() { + @Override + public Void doDbWork(final SQLiteDatabase db) throws WrappedException, + UnavailableStorageException { + + selectionCallback.doDbWork(db, selection.toString(), + selectionArgs.toArray(EMPTY_STRING_ARRAY)); + + return null; + } + }); + + selectionCallback.postDbWork(); + + } catch (WrappedException e) { + throw(MessagingException) e.getCause(); + } + + selectionArgs.clear(); + start += count; + } + } + + /** + * Defines the behavior of {@link LocalStore#doBatchSetSelection(BatchSetSelection, int)}. + */ + public interface BatchSetSelection { + /** + * @return The size of the argument list. + */ + int getListSize(); + + /** + * Get a specific item of the argument list. + * + * @param index + * The index of the item. + * + * @return Item at position {@code i} of the argument list. + */ + String getListItem(int index); + + /** + * Execute the SQL statement. + * + * @param db + * Use this {@link SQLiteDatabase} instance for your SQL statement. + * @param selectionSet + * A partial selection string containing place holders for the argument list, e.g. + * {@code " IN (?,?,?)"} (starts with a space). + * @param selectionArgs + * The current subset of the argument list. + * @throws UnavailableStorageException + */ + void doDbWork(SQLiteDatabase db, String selectionSet, String[] selectionArgs) + throws UnavailableStorageException; + + /** + * This will be executed after each invocation of + * {@link #doDbWork(SQLiteDatabase, String, String[])} (after the transaction has been + * committed). + */ + void postDbWork(); + } + + /** + * Change the state of a flag for a list of messages. + * + *

+ * The goal of this method is to be fast. Currently this means using as few SQL UPDATE + * statements as possible.
+ * Current benchmarks show that updating 1000 messages takes about 8 seconds on a Nexus 7. So + * there should be room for further improvement. + *

+ * + * @param messageIds + * A list of primary keys in the "messages" table. + * @param flag + * The flag to change. This must be a flag with a separate column in the database. + * @param newState + * {@code true}, if the flag should be set. {@code false}, otherwise. + * @param threadRootIds + * If this is {@code true}, {@code messageIds} contains the IDs of the messages at the + * root of a thread. In that case the flag is changed for all messages in these threads. + * If this is {@code false} only the messages in {@code messageIds} are changed. + * + * @throws MessagingException + */ + public void setFlag(final List messageIds, final Flag flag, + final boolean newState, final boolean threadRootIds) throws MessagingException { + + final ContentValues cv = new ContentValues(); + + switch (flag) { + case SEEN: { + cv.put("read", newState); + break; + } + case FLAGGED: { + cv.put("flagged", newState); + break; + } + case ANSWERED: { + cv.put("answered", newState); + break; + } + case FORWARDED: { + cv.put("forwarded", newState); + break; + } + default: { + throw new IllegalArgumentException("Flag must be a special column flag"); + } + } + + doBatchSetSelection(new BatchSetSelection() { + + @Override + public int getListSize() { + return messageIds.size(); + } + + @Override + public String getListItem(int index) { + return Long.toString(messageIds.get(index)); + } + + @Override + public void doDbWork(SQLiteDatabase db, String selectionSet, String[] selectionArgs) + throws UnavailableStorageException { + + db.update("messages", cv, "(empty IS NULL OR empty != 1) AND id" + selectionSet, + selectionArgs); + + if (threadRootIds) { + db.update("messages", cv, "(empty IS NULL OR empty != 1) AND thread_root" + + selectionSet, selectionArgs); + } + } + + @Override + public void postDbWork() { + notifyChange(); + } + }, FLAG_UPDATE_BATCH_SIZE); + } + + /** + * Get folder name and UID for the supplied messages. + * + * @param messageIds + * A list of primary keys in the "messages" table. + * @param threadedList + * If this is {@code true}, {@code messageIds} contains the IDs of the messages at the + * root of a thread. In that case return UIDs for all messages in these threads. + * If this is {@code false} only the UIDs for messages in {@code messageIds} are + * returned. + * + * @return The list of UIDs for the messages grouped by folder name. + * + * @throws MessagingException + */ + public Map> getFoldersAndUids(final List messageIds, + final boolean threadedList) throws MessagingException { + + final Map> folderMap = new HashMap>(); + + doBatchSetSelection(new BatchSetSelection() { + + @Override + public int getListSize() { + return messageIds.size(); + } + + @Override + public String getListItem(int index) { + return Long.toString(messageIds.get(index)); + } + + @Override + public void doDbWork(SQLiteDatabase db, String selectionSet, String[] selectionArgs) + throws UnavailableStorageException { + + String sqlPrefix = + "SELECT m.uid, f.name " + + "FROM messages m " + + "JOIN folders f ON (m.folder_id = f.id) " + + "WHERE (m.empty IS NULL OR m.empty != 1) AND "; + + String sql = sqlPrefix + "m.id" + selectionSet; + getDataFromCursor(db.rawQuery(sql, selectionArgs)); + + if (threadedList) { + String threadSql = sqlPrefix + "m.thread_root" + selectionSet; + getDataFromCursor(db.rawQuery(threadSql, selectionArgs)); + } + } + + private void getDataFromCursor(Cursor cursor) { + try { + while (cursor.moveToNext()) { + String uid = cursor.getString(0); + String folderName = cursor.getString(1); + + List uidList = folderMap.get(folderName); + if (uidList == null) { + uidList = new ArrayList(); + folderMap.put(folderName, uidList); + } + + uidList.add(uid); + } + } finally { + cursor.close(); + } + } + + @Override + public void postDbWork() { + notifyChange(); + + } + }, UID_CHECK_BATCH_SIZE); + + return folderMap; + } }