IMAPsearch #129

Closed
wants to merge 23 commits into
from

Conversation

Projects
None yet
5 participants
Contributor

rbayer commented Mar 8, 2012

Basic remote/server-side search functionality for IMAP servers. Activate in preferences on a per-account basis.

please wrap these lines like elsewhere in the code.

isRemoteSearchFullText()

Code Cleanup
getRemoteSearchFullText -> isRemoteSearchFullText
line wraps for preference items

add space after for. i haven't used Iterator since school where we were restricted to 1.4. some other code i'm reviewing has some of that clutter.

rbayer added some commits Mar 5, 2012

Merge remote-tracking branch 'remotes/upstream/master' into IMAPsearch
Conflicts:
	src/com/fsck/k9/mail/store/ImapStore.java
	src/com/fsck/k9/mail/store/LocalStore.java

actually, this was correct, even though it's not like that in much of the code.

Owner

rbayer replied Mar 8, 2012

Indeed, though those braces only showed up because my IDE put them in at an earlier edit and so I removed them here in order to remove extraneous diffs with master vs IMAPsearch/HEAD.

i suppose that's a good reason. kinda wishing you didn't change folderName back to folder, though! but leave it alone now.

i don't completely like this indenting (though it's much better than it was), but i can't find any good examples in the code, either. so leave it as-is (this note is to catch the eye of anyone else who happens to review).

+ }
+ public void searchRemoteMessagesSynchronous(final String acctUuid, final String folderName, final String query,
+ final Flag[] requiredFlags, final Flag[] forbiddenFlags, final MessagingListener listener) {
+ final Account acct = Preferences.getPreferences(mApplication.getApplicationContext()).getAccount(acctUuid);
@rabcyr

rabcyr Mar 9, 2012

Member

don't abbreviate names. use "account" instead of "acct".

Member

rabcyr commented Mar 9, 2012

this modified loadMessageForView() to dowload message if neither X_DOWNLOADED_FULL nor X_DOWNLOADED_PARTIAL. it will download the entire message, as i couldn't figure out yet how to set messages under the size limit to X_DOWNLOADED_FULL (couldn't get their size easily).
https://github.com/ashleywillis/k-9/commit/0149c9202ca98ce6b0f65969f8c1aa0d4c5fd15a

Owner

obra commented Mar 11, 2012

Is there a reason to make it toggled by a preference rather than just 'always on'?

Skobi commented Mar 19, 2012

Hi rbayer. Thank you very much for making android email clients aware of the (really necessary) IMAP "search" feature!
But as the (phy/soft) 'search button' is not present on every phone/tablet (esp. after 3.1), I'm voting for an integration to include the server-side results in the normal search, to offer a button to continue the search on the server on the results page (like the iPhone emailer does) or to have an additional second search button. What do you think?

Owner

obra commented Apr 7, 2012

I'm sorry it's taken me so long to review this patch.

First, OMG YAY! I'm so thrilled you've taken this on.

In terms of user experience - I'd like to find a way for us to decrease the user-facing complexity of this feature - It's a little more daunting than it could be -- and I have a fairly good understanding of what's actually going on.

To that end, I think I'd like you to:

  • Remove the popup when you enable server-side search - It's inconsistent with the rest of K-9's UI.
  • Always enable server-side search when it's available on the server.

Two items I'd love your feedback on:

Instead of prompting the user to choose a local or a remote search when they hit the search key, default to doing a local search with a button in the footer view to "Try this search on the server" when the user has started the search from inside a local folder.

Instead of having a separate preference for # of search results to get at a time, would you consider using the existing local folder size preference, or does that feel too wrong?

If we do that, we can move "Include body text" into Fetching mail -> Advanced -> "Server search includes message bodies"

In the longer term, are you considering building out functionality to search multiple server-side folders at once (by iterating over the folders and presenting the results in one view, I guess)?

Again, thank you so much for working on this and I'm sorry the UX review took me so long.

-jesse

@ghost ghost assigned obra Sep 5, 2012

Owner

cketti commented Mar 10, 2013

Latest 'master' contains this functionality.

@cketti cketti closed this Mar 10, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment