[geary/mjog/search-update: 13/17] Geary.App.SearchFolder: Substantial implementation rework
- From: Michael Gratton <mjog src gnome org>
- To: commits-list gnome org
- Cc:
- Subject: [geary/mjog/search-update: 13/17] Geary.App.SearchFolder: Substantial implementation rework
- Date: Mon, 28 Dec 2020 09:33:40 +0000 (UTC)
commit fe1854897a072e0a7a6c8fcd9a2b49471a48cf8a
Author: Michael Gratton <mike vee net>
Date: Thu Nov 5 23:03:45 2020 +1100
Geary.App.SearchFolder: Substantial implementation rework
This does the following:
* Makes updating the search query a non-async call, so that apps can
call it and forget about it
* Makes updating search results only update the folder's contents
when finished, and not if cancelled
* Cancels any existing search tasks if the query is updated
* Swaps sending removed signals before inserted signals to make the
conversation viewer happier
src/client/application/application-controller.vala | 2 +-
.../application/application-main-window.vala | 17 +-
src/engine/api/geary-search-query.vala | 2 +-
src/engine/app/app-search-folder.vala | 346 ++++++++++++---------
4 files changed, 199 insertions(+), 168 deletions(-)
---
diff --git a/src/client/application/application-controller.vala
b/src/client/application/application-controller.vala
index 2237aa618..6aec77acf 100644
--- a/src/client/application/application-controller.vala
+++ b/src/client/application/application-controller.vala
@@ -1088,7 +1088,7 @@ internal class Application.Controller :
update_account_status();
// Stop any background processes
- context.search.clear();
+ context.search.clear_query();
context.contacts.close();
context.cancellable.cancel();
diff --git a/src/client/application/application-main-window.vala
b/src/client/application/application-main-window.vala
index 394111026..e34d30a53 100644
--- a/src/client/application/application-main-window.vala
+++ b/src/client/application/application-main-window.vala
@@ -313,7 +313,6 @@ public class Application.MainWindow :
private GLib.Cancellable action_update_cancellable = new GLib.Cancellable();
private GLib.Cancellable folder_open = new GLib.Cancellable();
- private GLib.Cancellable search_open = new GLib.Cancellable();
private Geary.TimeoutManager update_ui_timeout;
private int64 update_ui_last = 0;
@@ -946,10 +945,8 @@ public class Application.MainWindow :
internal void start_search(string query_text, bool is_interactive) {
var context = get_selected_account_context();
if (context != null) {
- // Stop any search in progress
- this.search_open.cancel();
- var cancellable = this.search_open = new GLib.Cancellable();
-
+ // If the current folder is not the search folder, save it
+ // so it can be re-selected later when search is closed
if (this.previous_non_search_folder == null &&
this.selected_folder != null &&
this.selected_folder.used_as != SEARCH) {
@@ -968,7 +965,7 @@ public class Application.MainWindow :
this.folder_list.set_search(
this.application.engine, context.search
);
- yield context.search.search(query, cancellable);
+ context.search.update_query(query);
} catch (GLib.Error error) {
handle_error(context.account.information, error);
}
@@ -976,10 +973,8 @@ public class Application.MainWindow :
}
internal void stop_search(bool is_interactive) {
- // Stop any search in progress
- this.search_open.cancel();
- this.search_open = new GLib.Cancellable();
-
+ // If the search folder is current selected, deselect and
+ // re-select any previously selected folder
if (this.selected_folder == null ||
this.selected_folder.used_as == SEARCH) {
var to_select = this.previous_non_search_folder;
@@ -1001,7 +996,7 @@ public class Application.MainWindow :
this.folder_list.remove_search();
foreach (var context in this.controller.get_account_contexts()) {
- context.search.clear();
+ context.search.clear_query();
}
}
diff --git a/src/engine/api/geary-search-query.vala b/src/engine/api/geary-search-query.vala
index 60c6b62f5..62f7416d7 100644
--- a/src/engine/api/geary-search-query.vala
+++ b/src/engine/api/geary-search-query.vala
@@ -26,7 +26,7 @@
* @see Account.new_search_query
* @see Account.local_search_async
* @see Account.get_search_matches_async
- * @see App.SearchFolder.search
+ * @see App.SearchFolder.update_query
*/
public abstract class Geary.SearchQuery : BaseObject {
diff --git a/src/engine/app/app-search-folder.vala b/src/engine/app/app-search-folder.vala
index 981cb5a79..a8fa9404e 100644
--- a/src/engine/app/app-search-folder.vala
+++ b/src/engine/app/app-search-folder.vala
@@ -110,10 +110,10 @@ public class Geary.App.SearchFolder :
new Gee.HashSet<FolderPath?>();
// The email present in the folder, sorted
- private Gee.TreeSet<EmailEntry> contents;
+ private Gee.SortedSet<EmailEntry> entries;
// Map of engine ids to search ids
- private Gee.Map<EmailIdentifier,EmailEntry> id_map;
+ private Gee.Map<EmailIdentifier,EmailEntry> ids;
private Nonblocking.Mutex result_mutex = new Nonblocking.Mutex();
@@ -131,7 +131,8 @@ public class Geary.App.SearchFolder :
account.email_removed.connect(on_account_email_removed);
account.email_locally_removed.connect(on_account_email_removed);
- new_contents();
+ this.entries = new_entry_set();
+ this.ids = new_id_map();
// Always exclude emails that don't live anywhere from search
// results.
@@ -147,53 +148,40 @@ public class Geary.App.SearchFolder :
}
/**
- * Executes the given query over the account's local email.
+ * Sets the current search query for the folder.
*
- * Calling this will block until the search is complete.
+ * Calling this method will start the search folder asynchronously
+ * in the background. If the given query is not equal to the
+ * existing query, the folder's contents will be updated to
+ * reflect the changed query.
*/
- public async void search(SearchQuery query, GLib.Cancellable? cancellable)
- throws GLib.Error {
+ public void update_query(SearchQuery query) {
if (this.query == null || !this.query.equal_to(query)) {
- int result_mutex_token = yield result_mutex.claim_async();
-
- clear();
-
- if (cancellable != null) {
- GLib.Cancellable @internal = this.executing;
- cancellable.cancelled.connect(() => { @internal.cancel(); });
- }
+ this.executing.cancel();
+ this.executing = new GLib.Cancellable();
this.query = query;
- GLib.Error? error = null;
- try {
- yield do_search_async(null, null, this.executing);
- } catch(Error e) {
- error = e;
- }
-
- result_mutex.release(ref result_mutex_token);
-
- if (error != null) {
- throw error;
- }
+ this.update.begin();
}
}
/**
* Cancels and clears the search query and results.
*
- * The {@link query} property will be cleared.
+ * The {@link query} property will be set to null.
*/
- public void clear() {
+ public void clear_query() {
this.executing.cancel();
this.executing = new GLib.Cancellable();
- var old_ids = this.id_map;
- new_contents();
+ this.query = null;
+ var old_ids = this.ids;
+
+ this.entries = new_entry_set();
+ this.ids = new_id_map();
+
notify_email_removed(old_ids.keys);
notify_email_count_changed(0, REMOVED);
-
- this.query = null;
}
/**
@@ -220,10 +208,16 @@ public class Geary.App.SearchFolder :
Gee.Collection<Geary.EmailIdentifier> ids,
GLib.Cancellable? cancellable = null)
throws GLib.Error {
+ debug("Waiting for checking contains");
+ int result_mutex_token = yield this.result_mutex.claim_async(cancellable);
+ var existing_ids = this.ids;
+ result_mutex.release(ref result_mutex_token);
+
+ debug("Checking contains");
return Geary.traverse(
ids
).filter(
- (id) => this.id_map.has_key(id)
+ (id) => existing_ids.has_key(id)
).to_hash_set();
}
@@ -234,17 +228,22 @@ public class Geary.App.SearchFolder :
Folder.ListFlags flags,
Cancellable? cancellable = null
) throws GLib.Error {
- int result_mutex_token = yield result_mutex.claim_async();
+ debug("Waiting to list email");
+ int result_mutex_token = yield this.result_mutex.claim_async(cancellable);
+ var existing_entries = this.entries;
+ var existing_ids = this.ids;
+ result_mutex.release(ref result_mutex_token);
+ debug("Listing email");
var engine_ids = new Gee.LinkedList<EmailIdentifier>();
if (Folder.ListFlags.OLDEST_TO_NEWEST in flags) {
EmailEntry? oldest = null;
- if (!this.contents.is_empty) {
+ if (!existing_entries.is_empty) {
if (initial_id == null) {
- oldest = this.contents.last();
+ oldest = existing_entries.last();
} else {
- oldest = this.id_map.get(initial_id);
+ oldest = existing_ids.get(initial_id);
if (oldest == null) {
throw new EngineError.NOT_FOUND(
@@ -253,13 +252,13 @@ public class Geary.App.SearchFolder :
}
if (!(Folder.ListFlags.INCLUDING_ID in flags)) {
- oldest = contents.higher(oldest);
+ oldest = existing_entries.higher(oldest);
}
}
}
if (oldest != null) {
var iter = (
- this.contents.iterator_at(oldest) as
+ existing_entries.iterator_at(oldest) as
Gee.BidirIterator<EmailEntry>
);
engine_ids.add(oldest.id);
@@ -270,11 +269,11 @@ public class Geary.App.SearchFolder :
} else {
// Newest to oldest
EmailEntry? newest = null;
- if (!this.contents.is_empty) {
+ if (!existing_entries.is_empty) {
if (initial_id == null) {
- newest = this.contents.first();
+ newest = existing_entries.first();
} else {
- newest = this.id_map.get(initial_id);
+ newest = existing_ids.get(initial_id);
if (newest == null) {
throw new EngineError.NOT_FOUND(
@@ -283,13 +282,13 @@ public class Geary.App.SearchFolder :
}
if (!(Folder.ListFlags.INCLUDING_ID in flags)) {
- newest = contents.lower(newest);
+ newest = existing_entries.lower(newest);
}
}
}
if (newest != null) {
var iter = (
- this.contents.iterator_at(newest) as
+ existing_entries.iterator_at(newest) as
Gee.BidirIterator<EmailEntry>
);
engine_ids.add(newest.id);
@@ -313,8 +312,6 @@ public class Geary.App.SearchFolder :
}
}
- result_mutex.release(ref result_mutex_token);
-
if (list_error != null) {
throw list_error;
}
@@ -392,7 +389,7 @@ public class Geary.App.SearchFolder :
private void require_id(EmailIdentifier id)
throws EngineError.NOT_FOUND {
- if (!this.id_map.has_key(id)) {
+ if (!this.ids.has_key(id)) {
throw new EngineError.NOT_FOUND(
"Id not found: %s", id.to_string()
);
@@ -403,17 +400,114 @@ public class Geary.App.SearchFolder :
Gee.Collection<EmailIdentifier> to_check
) {
var available = new Gee.LinkedList<EmailIdentifier>();
- var id_map = this.id_map;
+ var ids = this.ids;
var iter = to_check.iterator();
while (iter.next()) {
var id = iter.get();
- if (id_map.has_key(id)) {
+ if (ids.has_key(id)) {
available.add(id);
}
}
return available;
}
+ private async void append(Folder folder,
+ Gee.Collection<EmailIdentifier> ids) {
+ // Grab the cancellable before the lock so that if the current
+ // search is cancelled while waiting, this doesn't go and try
+ // to update the new results.
+ var cancellable = this.executing;
+
+ debug("Waiting to append to search results");
+ try {
+ int result_mutex_token = yield this.result_mutex.claim_async(
+ cancellable
+ );
+ try {
+ if (!this.exclude_folders.contains(folder.path)) {
+ yield do_search_async(ids, null, cancellable);
+ }
+ } catch (GLib.Error error) {
+ this.account.report_problem(
+ new AccountProblemReport(this.account.information, error)
+ );
+ }
+
+ this.result_mutex.release(ref result_mutex_token);
+ } catch (GLib.IOError.CANCELLED mutex_err) {
+ // all good
+ } catch (GLib.Error mutex_err) {
+ warning("Error acquiring lock: %s", mutex_err.message);
+ }
+ }
+
+ private async void update() {
+ // Grab the cancellable before the lock so that if the current
+ // search is cancelled while waiting, this doesn't go and try
+ // to update the new results.
+ var cancellable = this.executing;
+
+ debug("Waiting to update search results");
+ try {
+ int result_mutex_token = yield this.result_mutex.claim_async(
+ cancellable
+ );
+ try {
+ yield do_search_async(null, null, cancellable);
+ } catch (GLib.Error error) {
+ this.account.report_problem(
+ new AccountProblemReport(this.account.information, error)
+ );
+ }
+
+ this.result_mutex.release(ref result_mutex_token);
+ } catch (GLib.IOError.CANCELLED mutex_err) {
+ // all good
+ } catch (GLib.Error mutex_err) {
+ warning("Error acquiring lock: %s", mutex_err.message);
+ }
+ }
+
+ private async void remove(Folder folder,
+ Gee.Collection<EmailIdentifier> ids) {
+
+ // Grab the cancellable before the lock so that if the current
+ // search is cancelled while waiting, this doesn't go and try
+ // to update the new results.
+ var cancellable = this.executing;
+
+ debug("Waiting to remove from search results");
+ try {
+ int result_mutex_token = yield this.result_mutex.claim_async(
+ cancellable
+ );
+
+ // Ensure this happens inside the lock so it is working with
+ // up-to-date data
+ var id_map = this.ids;
+ var relevant_ids = (
+ traverse(ids)
+ .filter(id => id_map.has_key(id))
+ .to_linked_list()
+ );
+ if (relevant_ids.size > 0) {
+ try {
+ yield do_search_async(null, relevant_ids, cancellable);
+ } catch (GLib.Error error) {
+ this.account.report_problem(
+ new AccountProblemReport(this.account.information, error)
+ );
+ }
+ }
+
+ this.result_mutex.release(ref result_mutex_token);
+ } catch (GLib.IOError.CANCELLED mutex_err) {
+ // all good
+ } catch (GLib.Error mutex_err) {
+ warning("Error acquiring lock: %s", mutex_err.message);
+ }
+ }
+
// NOTE: you must call this ONLY after locking result_mutex_token.
// If both *_ids parameters are null, the results of this search are
// considered to be the full new set. If non-null, the results are
@@ -424,11 +518,15 @@ public class Geary.App.SearchFolder :
Gee.Collection<EmailIdentifier>? remove_ids,
GLib.Cancellable? cancellable)
throws GLib.Error {
- var id_map = this.id_map;
- var contents = this.contents;
+ debug("Processing search results");
+ var entries = new_entry_set();
+ var ids = new_id_map();
var added = new Gee.LinkedList<EmailIdentifier>();
var removed = new Gee.LinkedList<EmailIdentifier>();
+ entries.add_all(this.entries);
+ ids.set_all(this.ids);
+
if (remove_ids == null) {
// Adding email to the search, either searching all local
// email if to_add is null, or adding only a matching
@@ -465,24 +563,24 @@ public class Geary.App.SearchFolder :
var hashed_results = new Gee.HashSet<EmailIdentifier>();
hashed_results.add_all(id_results);
- var existing = id_map.map_iterator();
+ var existing = ids.map_iterator();
while (existing.next()) {
if (!hashed_results.contains(existing.get_key())) {
var entry = existing.get_value();
existing.unset();
- contents.remove(entry);
+ entries.remove(entry);
removed.add(entry.id);
}
}
}
foreach (var email in email_results) {
- if (!id_map.has_key(email.id)) {
+ if (!ids.has_key(email.id)) {
var entry = new EmailEntry(
email.id, email.properties.date_received
);
- contents.add(entry);
- id_map.set(email.id, entry);
+ entries.add(entry);
+ ids.set(email.id, entry);
added.add(email.id);
}
}
@@ -491,89 +589,53 @@ public class Geary.App.SearchFolder :
// Removing email, can just remove them directly
foreach (var id in remove_ids) {
EmailEntry entry;
- if (id_map.unset(id, out entry)) {
- contents.remove(entry);
+ if (ids.unset(id, out entry)) {
+ entries.remove(entry);
removed.add(id);
}
}
}
- this._properties.set_total(this.contents.size);
-
- // Note that we probably shouldn't be firing these signals from inside
- // our mutex lock. Keep an eye on it, and if there's ever a case where
- // it might cause problems, it shouldn't be too hard to move the
- // firings outside.
-
- Folder.CountChangeReason reason = CountChangeReason.NONE;
- if (added.size > 0) {
- // TODO: we'd like to be able to use APPENDED here when applicable,
- // but because of the potential to append a thousand results at
- // once and the ConversationMonitor's inability to handle that
- // gracefully (#7464), we always use INSERTED for now.
- notify_email_inserted(added);
- reason |= Folder.CountChangeReason.INSERTED;
- }
- if (removed.size > 0) {
- notify_email_removed(removed);
- reason |= Folder.CountChangeReason.REMOVED;
- }
- if (reason != CountChangeReason.NONE)
- notify_email_count_changed(this.contents.size, reason);
- }
-
- private async void do_append(Folder folder,
- Gee.Collection<EmailIdentifier> ids,
- GLib.Cancellable? cancellable)
- throws GLib.Error {
- int result_mutex_token = yield result_mutex.claim_async();
-
- GLib.Error? error = null;
- try {
- if (!this.exclude_folders.contains(folder.path)) {
- yield do_search_async(ids, null, cancellable);
- }
- } catch (GLib.Error e) {
- error = e;
- }
-
- result_mutex.release(ref result_mutex_token);
+ if (!cancellable.is_cancelled()) {
+ this.entries = entries;
+ this.ids = ids;
- if (error != null)
- throw error;
- }
+ this._properties.set_total(entries.size);
- private async void do_remove(Folder folder,
- Gee.Collection<EmailIdentifier> ids,
- GLib.Cancellable? cancellable)
- throws GLib.Error {
- int result_mutex_token = yield result_mutex.claim_async();
+ // Note that we probably shouldn't be firing these signals from inside
+ // our mutex lock. Keep an eye on it, and if there's ever a case where
+ // it might cause problems, it shouldn't be too hard to move the
+ // firings outside.
- GLib.Error? error = null;
- try {
- var id_map = this.id_map;
- var relevant_ids = (
- traverse(ids)
- .filter(id => id_map.has_key(id))
- .to_linked_list()
- );
-
- if (relevant_ids.size > 0) {
- yield do_search_async(null, relevant_ids, cancellable);
+ Folder.CountChangeReason reason = CountChangeReason.NONE;
+ if (removed.size > 0) {
+ notify_email_removed(removed);
+ reason |= Folder.CountChangeReason.REMOVED;
+ }
+ if (added.size > 0) {
+ // TODO: we'd like to be able to use APPENDED here
+ // when applicable, but because of the potential to
+ // append a thousand results at once and the
+ // ConversationMonitor's inability to handle that
+ // gracefully (#7464), we always use INSERTED for now.
+ notify_email_inserted(added);
+ reason |= Folder.CountChangeReason.INSERTED;
}
- } catch (GLib.Error e) {
- error = e;
+ if (reason != CountChangeReason.NONE) {
+ notify_email_count_changed(this.entries.size, reason);
+ }
+ debug("Processing done, entries/ids: %d/%d", entries.size, ids.size);
+ } else {
+ debug("Processing cancelled, dropping entries/ids: %d/%d", entries.size, ids.size);
}
+ }
- result_mutex.release(ref result_mutex_token);
-
- if (error != null)
- throw error;
+ private inline Gee.SortedSet<EmailEntry> new_entry_set() {
+ return new Gee.TreeSet<EmailEntry>(EmailEntry.compare_to);
}
- private inline void new_contents() {
- this.contents = new Gee.TreeSet<EmailEntry>(EmailEntry.compare_to);
- this.id_map = new Gee.HashMap<EmailIdentifier,EmailEntry>();
+ private inline Gee.Map<EmailIdentifier,EmailEntry> new_id_map() {
+ return new Gee.HashMap<EmailIdentifier,EmailEntry>();
}
private void include_folder(Folder folder) {
@@ -613,40 +675,14 @@ public class Geary.App.SearchFolder :
private void on_email_locally_complete(Folder folder,
Gee.Collection<EmailIdentifier> ids) {
if (this.query != null) {
- this.do_append.begin(
- folder, ids, null,
- (obj, res) => {
- try {
- this.do_append.end(res);
- } catch (GLib.Error error) {
- this.account.report_problem(
- new AccountProblemReport(
- this.account.information, error
- )
- );
- }
- }
- );
+ this.append.begin(folder, ids);
}
}
private void on_account_email_removed(Folder folder,
Gee.Collection<EmailIdentifier> ids) {
if (this.query != null) {
- this.do_remove.begin(
- folder, ids, null,
- (obj, res) => {
- try {
- this.do_remove.end(res);
- } catch (GLib.Error error) {
- this.account.report_problem(
- new AccountProblemReport(
- this.account.information, error
- )
- );
- }
- }
- );
+ this.remove.begin(folder, ids);
}
}
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
