[geary/wip/search-cleanup: 2/9] Clean up client search term highlighting code
- From: Michael Gratton <mjog src gnome org>
- To: commits-list gnome org
- Cc:
- Subject: [geary/wip/search-cleanup: 2/9] Clean up client search term highlighting code
- Date: Mon, 4 Feb 2019 12:14:38 +0000 (UTC)
commit f3ef34531c07781668f97aed31c3ed597f80384a
Author: Michael Gratton <mike vee net>
Date: Sun Feb 3 16:32:00 2019 +1100
Clean up client search term highlighting code
Move all highlighing code from ConversationListBox into a seperate
class, ensure existing any existing highlighting process is cancelled
before launching a new one or clearing highlighting. Don't attempt to do
any highlighting when there are no search terms.
.../conversation-viewer/conversation-list-box.vala | 315 +++++++++++++--------
.../conversation-viewer/conversation-message.vala | 11 +-
.../conversation-viewer/conversation-viewer.vala | 8 +-
.../conversation-viewer/conversation-web-view.vala | 36 ++-
4 files changed, 233 insertions(+), 137 deletions(-)
---
diff --git a/src/client/conversation-viewer/conversation-list-box.vala
b/src/client/conversation-viewer/conversation-list-box.vala
index 35eec3f9..c34687e9 100644
--- a/src/client/conversation-viewer/conversation-list-box.vala
+++ b/src/client/conversation-viewer/conversation-list-box.vala
@@ -48,8 +48,189 @@ public class ConversationListBox : Gtk.ListBox, Geary.BaseInterface {
private const int MARK_READ_PADDING = 50;
+ /** Manages find/search term matching in a conversation. */
+ public class SearchManager : Geary.BaseObject {
+
+
+ // The list that owns this manager
+ private weak ConversationListBox list;
+
+ // Conversation being managed
+ private Geary.App.Conversation conversation;
+
+ // Cached search terms to apply to new messages
+ private Gee.Set<string>? terms = null;
+
+ // Total number of search matches found
+ private uint matches_found = 0;
+
+ // Cancellable used when highlighting search matches
+ private GLib.Cancellable highlight_cancellable = new GLib.Cancellable();
+
+
+ /** Fired when the number of matching emails has changed. */
+ public signal void matches_updated(uint matches);
+
+
+ internal SearchManager(ConversationListBox list,
+ Geary.App.Conversation conversation) {
+ this.list = list;
+ this.conversation = conversation;
+ }
+
+
+ /**
+ * Loads search term matches for this list's emails.
+ */
+ public async void highlight_matching_email(Geary.SearchQuery query)
+ throws GLib.Error {
+ cancel();
+
+ // Keep a copy of the current cancellable so it can't get
+ // changed out from underneath the execution of this method
+ GLib.Cancellable cancellable = this.highlight_cancellable;
+
+ Geary.Account account = this.conversation.base_folder.account;
+ Gee.Collection<Geary.EmailIdentifier>? matching =
+ yield account.local_search_async(
+ query,
+ this.conversation.get_count(),
+ 0,
+ null,
+ this.conversation.get_email_ids(),
+ cancellable
+ );
+
+ if (matching != null) {
+ Gee.Set<string>? terms =
+ yield account.get_search_matches_async(
+ query, matching, cancellable
+ );
+
+ if (cancellable.is_cancelled()) {
+ throw new GLib.IOError.CANCELLED(
+ "Search term highlighting cancelled"
+ );
+ }
+
+ if (terms != null && !terms.is_empty) {
+ this.terms = terms;
+
+ // Scroll to the first matching row first
+ EmailRow? first = null;
+ foreach (Geary.EmailIdentifier id in matching) {
+ EmailRow? row = this.list.get_email_row_by_id(id);
+ if (row != null &&
+ (first == null || row.get_index() < first.get_index())) {
+ first = row;
+ }
+ }
+ if (first != null) {
+ this.list.scroll_to(first);
+ }
+
+ // Now expand them all
+ foreach (Geary.EmailIdentifier id in matching) {
+ EmailRow? row = this.list.get_email_row_by_id(id);
+ if (row != null) {
+ apply_terms(row, terms, cancellable);
+ row.expand.begin();
+ }
+ }
+ }
+ }
+ }
+
+ /**
+ * Highlights matching terms in the given email row, if any.
+ */
+ internal void highlight_row_if_matching(EmailRow row) {
+ if (this.terms != null) {
+ apply_terms(row, this.terms, this.highlight_cancellable);
+ }
+ }
+
+ /**
+ * Removes search term highlighting from all messages.
+ */
+ public void unmark_terms() {
+ cancel();
+
+ this.list.foreach((child) => {
+ EmailRow? row = child as EmailRow;
+ if (row != null) {
+ if (row.is_search_match) {
+ row.is_search_match = false;
+ foreach (ConversationMessage msg_view in row.view) {
+ msg_view.unmark_search_terms();
+ }
+ }
+ }
+ });
+ }
+
+ public void cancel() {
+ this.highlight_cancellable.cancel();
+ this.highlight_cancellable = new Cancellable();
+ this.terms = null;
+ this.matches_found = 0;
+ notify_matches_updated();
+ }
+
+ private void apply_terms(EmailRow row,
+ Gee.Set<string>? terms,
+ GLib.Cancellable cancellable) {
+ if (row.view.message_body_state == COMPLETED) {
+ this.apply_terms_impl.begin(
+ row, terms, cancellable, apply_terms_impl_finished
+ );
+ } else {
+ row.view.notify["message-body-state"].connect(() => {
+ this.apply_terms_impl.begin(
+ row, terms, cancellable, apply_terms_impl_finished
+ );
+ });
+ }
+ }
+
+ // This should only be called from apply_terms above
+ private async uint apply_terms_impl(EmailRow row,
+ Gee.Set<string>? terms,
+ GLib.Cancellable cancellable)
+ throws GLib.IOError.CANCELLED {
+ uint count = 0;
+ foreach (ConversationMessage view in row.view) {
+ if (cancellable.is_cancelled()) {
+ throw new GLib.IOError.CANCELLED(
+ "Applying search terms cancelled"
+ );
+ }
+ count += yield view.highlight_search_terms(terms, cancellable);
+ }
+
+ row.is_search_match = (count > 0);
+ return count;
+ }
+
+ private void apply_terms_impl_finished(GLib.Object? obj,
+ GLib.AsyncResult res) {
+ try {
+ this.matches_found += this.apply_terms_impl.end(res);
+ notify_matches_updated();
+ } catch (GLib.IOError.CANCELLED err) {
+ // All good
+ }
+ }
+
+ private inline void notify_matches_updated() {
+ matches_updated(this.matches_found);
+ }
+
+ }
+
+
// Base class for list rows it the list box
- private abstract class ConversationRow : Gtk.ListBoxRow, Geary.BaseInterface {
+ internal abstract class ConversationRow : Gtk.ListBoxRow, Geary.BaseInterface {
protected const string EXPANDED_CLASS = "geary-expanded";
@@ -76,7 +257,7 @@ public class ConversationListBox : Gtk.ListBox, Geary.BaseInterface {
public signal void should_scroll();
- public ConversationRow(Geary.Email? email) {
+ protected ConversationRow(Geary.Email? email) {
base_ref();
this.email = email;
show();
@@ -122,7 +303,7 @@ public class ConversationListBox : Gtk.ListBox, Geary.BaseInterface {
// Displays a single ConversationEmail in the list box
- private class EmailRow : ConversationRow {
+ internal class EmailRow : ConversationRow {
private const string MATCH_CLASS = "geary-matched";
@@ -181,7 +362,7 @@ public class ConversationListBox : Gtk.ListBox, Geary.BaseInterface {
// Displays a loading widget in the list box
- private class LoadingRow : ConversationRow {
+ internal class LoadingRow : ConversationRow {
protected const string LOADING_CLASS = "geary-loading";
@@ -203,7 +384,7 @@ public class ConversationListBox : Gtk.ListBox, Geary.BaseInterface {
// Displays a single embedded composer in the list box
- private class ComposerRow : ConversationRow {
+ internal class ComposerRow : ConversationRow {
// The embedded composer for this row
public ComposerEmbed view { get; private set; }
@@ -281,6 +462,9 @@ public class ConversationListBox : Gtk.ListBox, Geary.BaseInterface {
/** Conversation being displayed. */
public Geary.App.Conversation conversation { get; private set; }
+ /** Search manager for highlighting search terms in this list. */
+ public SearchManager search { get; private set; }
+
// Used to load messages in conversation.
private Geary.App.EmailStore email_store;
@@ -303,12 +487,6 @@ public class ConversationListBox : Gtk.ListBox, Geary.BaseInterface {
// The id of the draft referred to by the current composer.
private Geary.EmailIdentifier? draft_id = null;
- // Cached search terms to apply to new messages
- private Gee.Set<string>? search_terms = null;
-
- // Total number of search matches found
- private uint search_matches_found = 0;
-
private Geary.TimeoutManager mark_read_timer;
@@ -365,9 +543,6 @@ public class ConversationListBox : Gtk.ListBox, Geary.BaseInterface {
public signal void mark_emails(Gee.Collection<Geary.EmailIdentifier> emails,
Geary.EmailFlags? flags_to_add, Geary.EmailFlags? flags_to_remove);
- /** Fired when an email that matches the current search terms is found. */
- public signal void search_matches_updated(uint matches);
-
/**
* Constructs a new conversation list box instance.
@@ -383,6 +558,8 @@ public class ConversationListBox : Gtk.ListBox, Geary.BaseInterface {
this.avatar_store = avatar_store;
this.config = config;
+ this.search = new SearchManager(this, conversation);
+
this.mark_read_timer = new Geary.TimeoutManager.milliseconds(
MARK_READ_TIMEOUT_MSEC, this.check_mark_read
);
@@ -407,6 +584,7 @@ public class ConversationListBox : Gtk.ListBox, Geary.BaseInterface {
}
public override void destroy() {
+ this.search.cancel();
this.cancellable.cancel();
this.email_rows.clear();
this.mark_read_timer.reset();
@@ -571,75 +749,6 @@ public class ConversationListBox : Gtk.ListBox, Geary.BaseInterface {
}
}
- /**
- * Loads search term matches for this list's emails.
- */
- public async void highlight_matching_email(Geary.SearchQuery query)
- throws GLib.Error {
- this.search_terms = null;
- this.search_matches_found = 0;
-
- Geary.Account account = this.conversation.base_folder.account;
- Gee.Collection<Geary.EmailIdentifier>? matching =
- yield account.local_search_async(
- query,
- this.conversation.get_count(),
- 0,
- null,
- this.conversation.get_email_ids(),
- this.cancellable
- );
-
- if (matching != null) {
- this.search_terms = yield account.get_search_matches_async(
- query, matching, this.cancellable
- );
-
- if (this.search_terms != null) {
- EmailRow? first = null;
- foreach (Geary.EmailIdentifier id in matching) {
- EmailRow? row = this.email_rows.get(id);
- if (row != null &&
- (first == null || row.get_index() < first.get_index())) {
- first = row;
- }
- }
- if (first != null) {
- scroll_to(first);
- }
-
- foreach (Geary.EmailIdentifier id in matching) {
- EmailRow? row = this.email_rows.get(id);
- if (row != null) {
- apply_search_terms(row);
- row.expand.begin();
- }
- }
- }
- }
- }
-
- /**
- * Removes search term highlighting from all messages.
- */
- public void unmark_search_terms() {
- this.search_terms = null;
- this.search_matches_found = 0;
-
- this.foreach((child) => {
- EmailRow? row = child as EmailRow;
- if (row != null) {
- if (row.is_search_match) {
- row.is_search_match = false;
- foreach (ConversationMessage msg_view in row.view) {
- msg_view.unmark_search_terms();
- }
- }
- }
- });
- search_matches_updated(this.search_matches_found);
- }
-
/**
* Increases the magnification level used for displaying messages.
*/
@@ -670,6 +779,11 @@ public class ConversationListBox : Gtk.ListBox, Geary.BaseInterface {
});
}
+ /** Returns the email row for the given id, if any. */
+ internal EmailRow? get_email_row_by_id(Geary.EmailIdentifier id) {
+ return this.email_rows.get(id);
+ }
+
private async void finish_loading(Geary.SearchQuery? query,
Gee.LinkedList<Geary.Email> to_insert,
Gee.LinkedList<Geary.Email> to_append)
@@ -721,7 +835,7 @@ public class ConversationListBox : Gtk.ListBox, Geary.BaseInterface {
// XXX this sucks for large conversations because it can take
// a long time for the load to complete and hence for
// matches to show up.
- yield highlight_matching_email(query);
+ yield this.search.highlight_matching_email(query);
}
}
@@ -762,6 +876,7 @@ public class ConversationListBox : Gtk.ListBox, Geary.BaseInterface {
if (!this.cancellable.is_cancelled()) {
EmailRow row = add_email(full_email);
row.view.load_avatar.begin(this.avatar_store);
+ this.search.highlight_row_if_matching(row);
yield row.expand();
}
}
@@ -815,11 +930,6 @@ public class ConversationListBox : Gtk.ListBox, Geary.BaseInterface {
}
email_added(view);
- // Apply any existing search terms to the new row
- if (this.search_terms != null) {
- apply_search_terms(row);
- }
-
return row;
}
@@ -901,33 +1011,6 @@ public class ConversationListBox : Gtk.ListBox, Geary.BaseInterface {
}
}
- private void apply_search_terms(EmailRow row) {
- if (row.view.message_body_state == COMPLETED) {
- this.apply_search_terms_impl.begin(row);
- } else {
- row.view.notify["message-body-state"].connect(() => {
- this.apply_search_terms_impl.begin(row);
- });
- }
- }
-
- // This should only be called from apply_search_terms above
- private async void apply_search_terms_impl(EmailRow row) {
- bool found = false;
- foreach (ConversationMessage view in row.view) {
- if (this.search_terms == null) {
- break;
- }
- uint count = yield view.highlight_search_terms(this.search_terms);
- if (count > 0) {
- found = true;
- }
- this.search_matches_found += count;
- }
- row.is_search_match = found;
- search_matches_updated(this.search_matches_found);
- }
-
/**
* Returns an new Iterable over all email views in the viewer
*/
diff --git a/src/client/conversation-viewer/conversation-message.vala
b/src/client/conversation-viewer/conversation-message.vala
index c1712851..e4594724 100644
--- a/src/client/conversation-viewer/conversation-message.vala
+++ b/src/client/conversation-viewer/conversation-message.vala
@@ -678,10 +678,9 @@ public class ConversationMessage : Gtk.Grid, Geary.BaseInterface {
* Highlighting includes both in the message headers, and the
* mesage body. returns the number of matching search terms.
*/
- public async uint highlight_search_terms(Gee.Set<string> search_matches) {
- // Remove existing highlights
- this.web_view.get_find_controller().search_finish();
-
+ public async uint highlight_search_terms(Gee.Set<string> search_matches,
+ GLib.Cancellable cancellable)
+ throws GLib.IOError.CANCELLED {
uint headers_found = 0;
uint webkit_found = 0;
foreach(string raw_match in search_matches) {
@@ -701,7 +700,9 @@ public class ConversationMessage : Gtk.Grid, Geary.BaseInterface {
}
}
- webkit_found += yield this.web_view.highlight_search_terms(search_matches);
+ webkit_found += yield this.web_view.highlight_search_terms(
+ search_matches, cancellable
+ );
return headers_found + webkit_found;
}
diff --git a/src/client/conversation-viewer/conversation-viewer.vala
b/src/client/conversation-viewer/conversation-viewer.vala
index 89e1662b..51be6b4b 100644
--- a/src/client/conversation-viewer/conversation-viewer.vala
+++ b/src/client/conversation-viewer/conversation-viewer.vala
@@ -246,7 +246,7 @@ public class ConversationViewer : Gtk.Stack, Geary.BaseInterface {
// are expanded and highlighted as they are added.
this.conversation_find_next.set_sensitive(false);
this.conversation_find_prev.set_sensitive(false);
- new_list.search_matches_updated.connect((count) => {
+ new_list.search.matches_updated.connect((count) => {
bool found = count > 0;
this.conversation_find_entry.set_icon_from_icon_name(
Gtk.EntryIconPosition.PRIMARY,
@@ -383,7 +383,7 @@ public class ConversationViewer : Gtk.Stack, Geary.BaseInterface {
}
} else {
// Find became disabled, re-show search terms if any
- this.current_list.unmark_search_terms();
+ this.current_list.search.unmark_terms();
Geary.SearchFolder? search_folder = (
this.current_list.conversation.base_folder
as Geary.SearchFolder
@@ -391,7 +391,7 @@ public class ConversationViewer : Gtk.Stack, Geary.BaseInterface {
if (search_folder != null) {
Geary.SearchQuery? search_query = search_folder.search_query;
if (search_query != null) {
- this.current_list.highlight_matching_email.begin(
+ this.current_list.search.highlight_matching_email.begin(
search_query
);
}
@@ -409,7 +409,7 @@ public class ConversationViewer : Gtk.Stack, Geary.BaseInterface {
this.current_list.conversation.base_folder.account
);
if (query != null) {
- this.current_list.highlight_matching_email.begin(query);
+ this.current_list.search.highlight_matching_email.begin(query);
}
}
}
diff --git a/src/client/conversation-viewer/conversation-web-view.vala
b/src/client/conversation-viewer/conversation-web-view.vala
index 17058306..7a2f1af6 100644
--- a/src/client/conversation-viewer/conversation-web-view.vala
+++ b/src/client/conversation-viewer/conversation-web-view.vala
@@ -101,7 +101,14 @@ public class ConversationWebView : ClientWebView {
*
* Returns the number of matching search terms.
*/
- public async uint highlight_search_terms(Gee.Collection<string> search_matches) {
+ public async uint highlight_search_terms(Gee.Collection<string> terms,
+ GLib.Cancellable cancellable)
+ throws GLib.IOError.CANCELLED {
+ WebKit.FindController controller = get_find_controller();
+
+ // Remove existing highlights
+ controller.search_finish();
+
// XXX WK2 doesn't deal with the multiple highlighting
// required by search folder matches, only single highlighting
// for a fine-like interface. For now, just highlight the
@@ -109,25 +116,20 @@ public class ConversationWebView : ClientWebView {
uint found = 0;
- WebKit.FindController controller = get_find_controller();
SourceFunc callback = this.highlight_search_terms.callback;
- ulong found_handler = 0;
- ulong not_found_handler = 0;
-
- found_handler = controller.found_text.connect((count) => {
+ ulong found_handler = controller.found_text.connect((count) => {
found = count;
- controller.disconnect(found_handler);
- controller.disconnect(not_found_handler);
callback();
});
- not_found_handler = controller.failed_to_find_text.connect(() => {
- controller.disconnect(found_handler);
- controller.disconnect(not_found_handler);
+ ulong not_found_handler = controller.failed_to_find_text.connect(() => {
+ callback();
+ });
+ ulong cancelled_handler = cancellable.cancelled.connect(() => {
callback();
});
controller.search(
- Geary.Collection.get_first(search_matches),
+ Geary.Collection.get_first(terms),
WebKit.FindOptions.CASE_INSENSITIVE |
WebKit.FindOptions.WRAP_AROUND,
128
@@ -135,6 +137,16 @@ public class ConversationWebView : ClientWebView {
yield;
+ controller.disconnect(found_handler);
+ controller.disconnect(not_found_handler);
+ cancellable.disconnect(cancelled_handler);
+
+ if (cancellable.is_cancelled()) {
+ throw new IOError.CANCELLED(
+ "ConversationWebView highlight search terms cancelled"
+ );
+ }
+
return found;
}
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
