From c93cfc38b18af6cf9849423d7d2495e6c3eea80b Mon Sep 17 00:00:00 2001 From: Michael Gratton Date: Mon, 9 Dec 2019 12:35:02 +1100 Subject: [PATCH] Clean up Engine search API Clean up search API so that query string parsing and error handling can be pushed out to the client. Rename Geary.Account::open_search factory method to ::new_search_query to better reflect what it actually does. Update Geary.SearchFolder::search to be async so it can throw errors and the caller can catch them, require SearchQuery objects instead of plain text query strings. Add Geary.SearchQuery::owner property so that the query's account can be sanity checked. Update API documentation to reflect this. --- .../application/application-main-window.vala | 68 +++++--- .../conversation-viewer.vala | 18 ++- src/engine/api/geary-account.vala | 11 +- src/engine/api/geary-search-folder.vala | 59 +++---- src/engine/api/geary-search-query.vala | 35 ++-- .../imap-db/search/imap-db-search-folder.vala | 153 +++++++++--------- .../imap-db/search/imap-db-search-query.vala | 7 +- .../imap-engine-generic-account.vala | 12 +- test/engine/api/geary-account-mock.vala | 13 +- 9 files changed, 215 insertions(+), 161 deletions(-) diff --git a/src/client/application/application-main-window.vala b/src/client/application/application-main-window.vala index 24b54256..9f00d46a 100644 --- a/src/client/application/application-main-window.vala +++ b/src/client/application/application-main-window.vala @@ -902,37 +902,53 @@ public class Application.MainWindow : return closed; } - public void show_search(string text, bool is_interactive) { - Geary.SearchFolder? search_folder = null; - if (this.selected_account != null) { - search_folder = this.selected_account.get_special_folder( + internal async void start_search(string query_text, bool is_interactive) { + var account = this.selected_account; + if (account != null) { + var folder = account.get_special_folder( SEARCH ) as Geary.SearchFolder; - } + // Stop any search in progress + this.search_open.cancel(); + var cancellable = this.search_open = new GLib.Cancellable(); + + var strategy = this.application.config.get_search_strategy(); + try { + var query = yield account.new_search_query( + query_text, + strategy, + cancellable + ); + this.folder_list.set_search(this.application.engine, folder); + yield folder.search(query, cancellable); + } catch (GLib.Error error) { + handle_error(account.information, error); + } + } + } + + internal void stop_search(bool is_interactive) { // Stop any search in progress this.search_open.cancel(); - var cancellable = this.search_open = new GLib.Cancellable(); + this.search_open = new GLib.Cancellable(); - if (Geary.String.is_empty_or_whitespace(text)) { - if (this.previous_non_search_folder != null && - this.selected_folder is Geary.SearchFolder) { - this.select_folder.begin( - this.previous_non_search_folder, is_interactive - ); - } - this.folder_list.remove_search(); - if (search_folder != null) { - search_folder.clear(); - } - } else if (search_folder != null) { - search_folder.search( - text, this.application.config.get_search_strategy(), cancellable - ); - this.folder_list.set_search( - this.application.engine, search_folder + if (this.previous_non_search_folder != null && + this.selected_folder is Geary.SearchFolder) { + this.select_folder.begin( + this.previous_non_search_folder, is_interactive ); } + this.folder_list.remove_search(); + + if (this.selected_account != null) { + var folder = this.selected_account.get_special_folder( + SEARCH + ) as Geary.SearchFolder; + if (folder != null) { + folder.clear(); + } + } } internal bool select_first_inbox(bool is_interactive) { @@ -2061,7 +2077,11 @@ public class Application.MainWindow : } private void on_search(string text) { - show_search(text, true); + if (Geary.String.is_empty_or_whitespace(text)) { + stop_search(true); + } else { + this.start_search.begin(text, true); + } } private void on_visible_conversations_changed(Gee.Set visible) { diff --git a/src/client/conversation-viewer/conversation-viewer.vala b/src/client/conversation-viewer/conversation-viewer.vala index de5e185e..6ffeba11 100644 --- a/src/client/conversation-viewer/conversation-viewer.vala +++ b/src/client/conversation-viewer/conversation-viewer.vala @@ -296,10 +296,9 @@ public class ConversationViewer : Gtk.Stack, Geary.BaseInterface { conversation.base_folder.account, null ); if (query == null) { - Geary.SearchFolder? search_folder = - conversation.base_folder as Geary.SearchFolder; + var search_folder = conversation.base_folder as Geary.SearchFolder; if (search_folder != null) { - query = search_folder.search_query; + query = search_folder.query; } } @@ -425,8 +424,11 @@ public class ConversationViewer : Gtk.Stack, Geary.BaseInterface { // opening every message in the conversation as soon as // the user presses a key if (text.length >= 2) { - query = yield account.open_search( - text, this.config.get_search_strategy(), cancellable + var strategy = this.config.get_search_strategy(); + query = yield account.new_search_query( + text, + strategy, + cancellable ); } } @@ -458,10 +460,10 @@ public class ConversationViewer : Gtk.Stack, Geary.BaseInterface { ); this.conversation_find_undo.reset(); if (search_folder != null) { - Geary.SearchQuery? search_query = search_folder.search_query; - if (search_query != null) { + Geary.SearchQuery? query = search_folder.query; + if (query != null) { this.current_list.search.highlight_matching_email.begin( - search_query, + query, true ); } diff --git a/src/engine/api/geary-account.vala b/src/engine/api/geary-account.vala index aad174bd..0bb90aba 100644 --- a/src/engine/api/geary-account.vala +++ b/src/engine/api/geary-account.vala @@ -436,13 +436,12 @@ public abstract class Geary.Account : BaseObject, Logging.Source { * baked into the caller's code is up to the caller. CONSERVATIVE is designed to be a good * default. * - * The SearchQuery object can only be used with calls into this Account. - * - * Dropping the last reference to the SearchQuery will close it. + * The resulting object can only be used with calls into this + * account instance. */ - public abstract async Geary.SearchQuery open_search(string query, - SearchQuery.Strategy strategy, - GLib.Cancellable? cancellable) + public abstract async SearchQuery new_search_query(string query, + SearchQuery.Strategy strategy, + GLib.Cancellable? cancellable) throws GLib.Error; /** diff --git a/src/engine/api/geary-search-folder.vala b/src/engine/api/geary-search-folder.vala index 9a2d0cb8..da034c69 100644 --- a/src/engine/api/geary-search-folder.vala +++ b/src/engine/api/geary-search-folder.vala @@ -1,12 +1,13 @@ -/* Copyright 2016 Software Freedom Conservancy Inc. +/* + * Copyright 2016 Software Freedom Conservancy Inc. + * Copyright 2019 Michael Gratton * * This software is licensed under the GNU Lesser General Public License - * (version 2.1 or later). See the COPYING file in this distribution. + * (version 2.1 or later). See the COPYING file in this distribution. */ /** - * Special local {@link Folder} used to query and display search results of {@link Email} from - * across the {@link Account}'s local storage. + * A local folder to execute and collect results of search queries. * * SearchFolder is merely specified to be a Folder, but implementations may add various * {@link FolderSupport} interfaces. In particular {@link FolderSupport.Remove} should be supported, @@ -17,8 +18,9 @@ * translate those EmailIdentifiers to their own type for ordering reasons, but in general the * expectation is that the results of SearchFolder can then be applied to operations on Email in * other remote-backed folders. + * + * @see SearchQuery */ - public abstract class Geary.SearchFolder : Geary.AbstractLocalFolder { private weak Account _account; public override Account account { get { return _account; } } @@ -35,13 +37,13 @@ public abstract class Geary.SearchFolder : Geary.AbstractLocalFolder { } } - public Geary.SearchQuery? search_query { get; protected set; default = null; } - /** - * Fired when the search query has changed. This signal is fired *after* the search - * has completed. - */ - public signal void search_query_changed(Geary.SearchQuery? query); + /** The query being evaluated by this folder, if any. */ + public Geary.SearchQuery? query { get; protected set; default = null; } + + /** Emitted when the current query has been fully evaluated. */ + public signal void query_evaluation_complete(); + protected SearchFolder(Account account, FolderProperties properties, FolderPath path) { _account = account; @@ -49,33 +51,36 @@ public abstract class Geary.SearchFolder : Geary.AbstractLocalFolder { _path = path; } - protected virtual void notify_search_query_changed(SearchQuery? query) { - search_query_changed(query); - } - /** - * Sets the keyword string for this search. + * Sets the query to be evaluated for this folder. * - * This is a nonblocking call that initiates a background search which can be stopped with the - * supplied Cancellable. - * - * When the search is completed, {@link search_query_changed} will be fired. It's possible for - * the {@link search_query} property to change before completion. + * Executes an asynchronous search, which can be stopped via the + * supplied cancellable. When the search is complete, {@link + * query_evaluation_complete} will be emitted. if an error occurs, + * the signal will not be invoked. It is possible for the {@link + * query} property to change before completion. */ - public abstract void search(string query, SearchQuery.Strategy strategy, Cancellable? cancellable = null); + public abstract async void search(SearchQuery query, + GLib.Cancellable? cancellable = null) + throws GLib.Error; /** * Clears the search query and results. * - * {@link search_query_changed} will be fired and {@link search_query} will be set to null. + * The {@link query_evaluation_complete} signal will be emitted + * and {@link query} will be set to null. */ public abstract void clear(); /** - * Given a list of mail IDs, returns a set of casefolded words that match for the current - * search query. + * Returns a set of case-folded words matched by the current query. + * + * The set contains words from the given collection of email that + * match any of the non-negated text operators in {@link query}. */ public abstract async Gee.Set? get_search_matches_async( - Gee.Collection ids, Cancellable? cancellable = null) throws Error; -} + Gee.Collection ids, + GLib.Cancellable? cancellable = null + ) throws GLib.Error; +} diff --git a/src/engine/api/geary-search-query.vala b/src/engine/api/geary-search-query.vala index 17df026a..4c4ec8a3 100644 --- a/src/engine/api/geary-search-query.vala +++ b/src/engine/api/geary-search-query.vala @@ -1,20 +1,27 @@ -/* Copyright 2016 Software Freedom Conservancy Inc. +/* + * Copyright 2016 Software Freedom Conservancy Inc. + * Copyright 2019 Michael Gratton * * This software is licensed under the GNU Lesser General Public License - * (version 2.1 or later). See the COPYING file in this distribution. + * (version 2.1 or later). See the COPYING file in this distribution. */ /** - * An object to hold state for various search subsystems that might need to - * parse the same text string different ways. + * Specifies an expression for searching email in a search folder. * - * The only interaction the API user should have with this is creating new ones and then passing - * them to the search methods in the Engine. + * New instances can be constructed via {@link + * Account.new_search_query} and then passed to search methods on + * {@link Account} or {@link SearchFolder}. * - * @see Geary.Account.open_search + * @see Account.new_search_query + * @see Account.local_search_async + * @see Account.get_search_matches_async + * @see SearchFolder.search */ public abstract class Geary.SearchQuery : BaseObject { + + /** * An advisory parameter regarding search quality, scope, and breadth. * @@ -50,8 +57,14 @@ public abstract class Geary.SearchQuery : BaseObject { HORIZON } + + /** The account that owns this query. */ + public Account owner { get; private set; } + /** - * The original user search text. + * The original search text. + * + * This is used mostly for debugging. */ public string raw { get; private set; } @@ -60,7 +73,11 @@ public abstract class Geary.SearchQuery : BaseObject { */ public Strategy strategy { get; private set; } - protected SearchQuery(string raw, Strategy strategy) { + + protected SearchQuery(Account owner, + string raw, + Strategy strategy) { + this.owner = owner; this.raw = raw; this.strategy = strategy; } diff --git a/src/engine/imap-db/search/imap-db-search-folder.vala b/src/engine/imap-db/search/imap-db-search-folder.vala index 34e3ee71..a3eb8ac6 100644 --- a/src/engine/imap-db/search/imap-db-search-folder.vala +++ b/src/engine/imap-db/search/imap-db-search-folder.vala @@ -52,13 +52,15 @@ private class Geary.ImapDB.SearchFolder : Geary.SearchFolder, Geary.FolderSuppor account.email_removed.disconnect(on_account_email_removed); } - private async void append_new_email_async(Geary.SearchQuery query, Geary.Folder folder, - Gee.Collection ids, Cancellable? cancellable) throws Error { + private async void append_new_email_async(Geary.Folder folder, + Gee.Collection ids, + GLib.Cancellable? cancellable) + throws GLib.Error { int result_mutex_token = yield result_mutex.claim_async(); Error? error = null; try { - yield do_search_async(query, ids, null, cancellable); + yield do_search_async(ids, null, cancellable); } catch(Error e) { error = e; } @@ -69,11 +71,13 @@ private class Geary.ImapDB.SearchFolder : Geary.SearchFolder, Geary.FolderSuppor throw error; } - private async void handle_removed_email_async(Geary.SearchQuery query, Geary.Folder folder, - Gee.Collection ids, Cancellable? cancellable) throws Error { + private async void handle_removed_email_async(Geary.Folder folder, + Gee.Collection ids, + GLib.Cancellable? cancellable) + throws GLib.Error { int result_mutex_token = yield result_mutex.claim_async(); - Error? error = null; + GLib.Error? error = null; try { Gee.ArrayList relevant_ids = Geary.traverse(ids) @@ -81,9 +85,10 @@ private class Geary.ImapDB.SearchFolder : Geary.SearchFolder, Geary.FolderSuppor id => ImapDB.SearchEmailIdentifier.collection_get_email_identifier(search_results, id)) .to_array_list(); - if (relevant_ids.size > 0) - yield do_search_async(query, null, relevant_ids, cancellable); - } catch(Error e) { + if (relevant_ids.size > 0) { + yield do_search_async(null, relevant_ids, cancellable); + } + } catch (GLib.Error e) { error = e; } @@ -97,56 +102,40 @@ private class Geary.ImapDB.SearchFolder : Geary.SearchFolder, Geary.FolderSuppor * Clears the search query and results. */ public override void clear() { - Gee.Collection local_results = search_results; + var local_results = this.search_results; clear_search_results(); notify_email_removed(local_results); notify_email_count_changed(0, Geary.Folder.CountChangeReason.REMOVED); - if (search_query != null) { - search_query = null; - notify_search_query_changed(null); + if (this.query != null) { + this.query = null; + this.query_evaluation_complete(); } } /** * Sets the keyword string for this search. */ - public override void search(string query, Geary.SearchQuery.Strategy strategy, Cancellable? cancellable = null) { - set_search_query_async.begin(query, strategy, cancellable, on_set_search_query_complete); - } - - private void on_set_search_query_complete(Object? source, AsyncResult result) { - try { - set_search_query_async.end(result); - } catch(Error e) { - debug("Search error: %s", e.message); - } - } - - private async void set_search_query_async(string query, - Geary.SearchQuery.Strategy strategy, - Cancellable? cancellable) + public override async void search(Geary.SearchQuery query, + GLib.Cancellable? cancellable = null) throws GLib.Error { - Geary.SearchQuery search_query = yield account.open_search( - query, strategy, cancellable - ); - int result_mutex_token = yield result_mutex.claim_async(); - Error? error = null; + this.query = query; + GLib.Error? error = null; try { - yield do_search_async(search_query, null, null, cancellable); + yield do_search_async(null, null, cancellable); } catch(Error e) { error = e; } result_mutex.release(ref result_mutex_token); - this.search_query = search_query; - notify_search_query_changed(search_query); - - if (error != null) + if (error != null) { throw error; + } + + this.query_evaluation_complete(); } // NOTE: you must call this ONLY after locking result_mutex_token. @@ -155,8 +144,8 @@ private class Geary.ImapDB.SearchFolder : Geary.SearchFolder, Geary.FolderSuppor // considered to be a delta and are added or subtracted from the full set. // add_ids are new ids to search for, remove_ids are ids in our result set // that will be removed if this search doesn't turn them up. - private async void do_search_async(Geary.SearchQuery query, Gee.Collection? add_ids, - Gee.Collection? remove_ids, Cancellable? cancellable) throws Error { + private async void do_search_async(Gee.Collection? add_ids, + Gee.Collection? remove_ids, Cancellable? cancellable) throws Error { // There are three cases here: 1) replace full result set, where the // *_ids parameters are both null, 2) add to result set, where just // remove_ids is null, and 3) remove from result set, where just @@ -167,9 +156,17 @@ private class Geary.ImapDB.SearchFolder : Geary.SearchFolder, Geary.FolderSuppor // smarter about only fetching the search results in list_email_async() // etc., but this leads to some more complications when redoing the // search. - Gee.ArrayList results - = ImapDB.SearchEmailIdentifier.array_list_from_results(yield account.local_search_async( - query, MAX_RESULT_EMAILS, 0, exclude_folders, add_ids ?? remove_ids, cancellable)); + Gee.ArrayList results = + SearchEmailIdentifier.array_list_from_results( + yield account.local_search_async( + this.query, + MAX_RESULT_EMAILS, + 0, + exclude_folders, + add_ids ?? remove_ids, + cancellable + ) + ); Gee.List added = Gee.List.empty(); @@ -338,11 +335,16 @@ private class Geary.ImapDB.SearchFolder : Geary.SearchFolder, Geary.FolderSuppor * search query. */ public override async Gee.Set? get_search_matches_async( - Gee.Collection ids, Cancellable? cancellable = null) throws Error { - if (search_query == null) - return null; - - return yield account.get_search_matches_async(search_query, ids, cancellable); + Gee.Collection ids, + GLib.Cancellable? cancellable = null + ) throws GLib.Error { + Gee.Set? results = null; + if (this.query != null) { + results = yield account.get_search_matches_async( + this.query, ids, cancellable + ); + } + return results; } private void include_folder(Geary.Folder folder) { @@ -358,8 +360,9 @@ private class Geary.ImapDB.SearchFolder : Geary.SearchFolder, Geary.FolderSuppor } private void clear_search_results() { - search_results = new Gee.TreeSet( - ImapDB.SearchEmailIdentifier.compare_descending); + this.search_results = new Gee.TreeSet( + SearchEmailIdentifier.compare_descending + ); } private void on_folders_available_unavailable(Gee.Collection? available, @@ -384,40 +387,42 @@ private class Geary.ImapDB.SearchFolder : Geary.SearchFolder, Geary.FolderSuppor private void on_email_locally_complete(Geary.Folder folder, Gee.Collection ids) { - if (search_query != null) { + if (this.query != null) { this.append_new_email_async.begin( - search_query, folder, ids, null, on_append_new_email_complete + folder, ids, null, + (obj, res) => { + try { + this.append_new_email_async.end(res); + } catch (GLib.Error error) { + this.account.report_problem( + new Geary.AccountProblemReport( + this.account.information, error + ) + ); + } + } ); } } - private void on_append_new_email_complete(GLib.Object? source, - GLib.AsyncResult result) { - try { - this.append_new_email_async.end(result); - } catch (GLib.Error e) { - debug("Error appending new email to search results: %s", e.message); - } - } - private void on_account_email_removed(Geary.Folder folder, Gee.Collection ids) { - if (search_query != null) { + if (this.query != null) { this.handle_removed_email_async.begin( - search_query, folder, ids, null, - on_handle_removed_email_complete + folder, ids, null, + (obj, res) => { + try { + this.handle_removed_email_async.end(res); + } catch (GLib.Error error) { + this.account.report_problem( + new Geary.AccountProblemReport( + this.account.information, error + ) + ); + } + } ); } } - private void on_handle_removed_email_complete(GLib.Object? source, - GLib.AsyncResult result) { - try { - this.handle_removed_email_async.end(result); - } catch (GLib.Error e) { - debug("Error removing removed email from search results: %s", - e.message); - } - } - } diff --git a/src/engine/imap-db/search/imap-db-search-query.vala b/src/engine/imap-db/search/imap-db-search-query.vala index 35c71d7c..ea00787a 100644 --- a/src/engine/imap-db/search/imap-db-search-query.vala +++ b/src/engine/imap-db/search/imap-db-search-query.vala @@ -261,12 +261,13 @@ private class Geary.ImapDB.SearchQuery : Geary.SearchQuery { // A list of all search terms, regardless of search op field name private Gee.ArrayList all = new Gee.ArrayList(); - public async SearchQuery(ImapDB.Account account, + public async SearchQuery(Geary.Account owner, + ImapDB.Account local, string query, Geary.SearchQuery.Strategy strategy, GLib.Cancellable? cancellable) { - base(query, strategy); - this.account = account; + base(owner, query, strategy); + this.account = local; switch (strategy) { case Strategy.EXACT: diff --git a/src/engine/imap-engine/imap-engine-generic-account.vala b/src/engine/imap-engine/imap-engine-generic-account.vala index c0394112..4edcc80e 100644 --- a/src/engine/imap-engine/imap-engine-generic-account.vala +++ b/src/engine/imap-engine/imap-engine-generic-account.vala @@ -200,6 +200,7 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { // Halt internal tasks early so they stop using local and // remote connections. + this.search_folder.clear(); this.refresh_folder_timer.reset(); this.open_cancellable.cancel(); this.processor.stop(); @@ -524,11 +525,14 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { return yield local.fetch_email_async(check_id(email_id), required_fields, cancellable); } - public override async Geary.SearchQuery open_search(string query, - SearchQuery.Strategy strategy, - GLib.Cancellable? cancellable) + /** {@inheritDoc} */ + public override async SearchQuery new_search_query(string query, + SearchQuery.Strategy strategy, + GLib.Cancellable? cancellable) throws GLib.Error { - return yield new ImapDB.SearchQuery(local, query, strategy, cancellable); + return yield new ImapDB.SearchQuery( + this, local, query, strategy, cancellable + ); } public override async Gee.Collection? local_search_async(Geary.SearchQuery query, diff --git a/test/engine/api/geary-account-mock.vala b/test/engine/api/geary-account-mock.vala index 1a1aeeb1..07551eb1 100644 --- a/test/engine/api/geary-account-mock.vala +++ b/test/engine/api/geary-account-mock.vala @@ -10,8 +10,9 @@ public class Geary.MockAccount : Account, MockObject { public class MockSearchQuery : SearchQuery { - internal MockSearchQuery() { - base("", SearchQuery.Strategy.EXACT); + internal MockSearchQuery(Account owner, + string raw) { + base(owner, raw, SearchQuery.Strategy.EXACT); } } @@ -214,11 +215,11 @@ public class Geary.MockAccount : Account, MockObject { ); } - public override async SearchQuery open_search(string query, - SearchQuery.Strategy strategy, - GLib.Cancellable? cancellable) + public override async SearchQuery new_search_query(string raw, + SearchQuery.Strategy strategy, + GLib.Cancellable? cancellable) throws GLib.Error { - return new MockSearchQuery(); + return new MockSearchQuery(this, raw); } public override async Gee.Collection?