Commit 1fa7eb81 authored by Michael Gratton's avatar Michael Gratton 🤞

Merge branch 'wip/289-folder-not-fully-populated' into 'mainline'

Fix ConversationMonitor sometimes not loading more from remote

See #289

See merge request !195

(cherry picked from commit dc656d7a)

3a44628d Fix ConversationMonitor sometimes not loading more from remote
d6ca47ba Ensure ImapDb.Folder updates email total when marking removed
03dbe0e0 Don't unncessarily check for more conversations to load
6e5c51cd Improve how the client triggers conversation auto-loading
df80569b Remove some unused code
parent 8cff1f06
Pipeline #73618 passed with stages
in 28 minutes and 53 seconds
......@@ -3069,11 +3069,14 @@ public class GearyController : Geary.BaseObject {
}
}
private void on_scan_completed() {
private void on_scan_completed(Geary.App.ConversationMonitor monitor) {
// Done scanning. Check if we have enough messages to fill
// the conversation list; if not, trigger a load_more();
if (!main_window.conversation_list_has_scrollbar()) {
debug("Not enough messages, loading more for folder %s", current_folder.to_string());
if (!main_window.conversation_list_has_scrollbar() &&
monitor == this.current_conversations &&
monitor.can_load_more) {
debug("Not enough messages, loading more for folder %s",
current_folder.to_string());
on_load_more();
}
}
......
......@@ -12,9 +12,6 @@ public class ConversationListView : Gtk.TreeView, Geary.BaseInterface {
private bool enable_load_more = true;
// Used to avoid repeated calls to load_more(). Contains the last "upper" bound of the
// scroll adjustment seen at the call to load_more().
private double last_upper = -1.0;
private bool reset_adjustment = false;
private Geary.App.ConversationMonitor? conversation_monitor;
private Gee.Set<Geary.App.Conversation>? current_visible_conversations = null;
......@@ -101,7 +98,6 @@ public class ConversationListView : Gtk.TreeView, Geary.BaseInterface {
old_store.rows_reordered.disconnect(on_rows_changed);
old_store.row_changed.disconnect(on_rows_changed);
old_store.row_deleted.disconnect(on_rows_changed);
old_store.row_deleted.disconnect(on_row_deleted);
old_store.destroy();
}
......@@ -110,7 +106,6 @@ public class ConversationListView : Gtk.TreeView, Geary.BaseInterface {
new_store.rows_reordered.connect(on_rows_changed);
new_store.row_changed.connect(on_rows_changed);
new_store.row_deleted.connect(on_rows_changed);
new_store.row_deleted.connect(on_row_deleted);
new_store.conversations_removed.connect(on_conversations_removed);
new_store.conversations_added.connect(on_conversations_added);
}
......@@ -140,6 +135,20 @@ public class ConversationListView : Gtk.TreeView, Geary.BaseInterface {
}
}
private void check_load_more() {
// Check if we're at the very bottom of the list. If we are,
// it's time to issue a load_more signal.
Gtk.Adjustment adjustment = ((Gtk.Scrollable) this).get_vadjustment();
double upper = adjustment.get_upper();
double threshold = upper - adjustment.page_size - LOAD_MORE_HEIGHT;
if (this.conversation_monitor.can_load_more &&
adjustment.get_value() >= threshold) {
load_more();
}
schedule_visible_conversations_changed();
}
private void on_conversation_monitor_changed() {
if (conversation_monitor != null) {
conversation_monitor.scan_started.disconnect(on_scan_started);
......@@ -155,11 +164,12 @@ public class ConversationListView : Gtk.TreeView, Geary.BaseInterface {
}
private void on_scan_started() {
enable_load_more = false;
this.enable_load_more = false;
}
private void on_scan_completed() {
enable_load_more = true;
this.enable_load_more = true;
check_load_more();
// Select the first conversation, if autoselect is enabled,
// nothing has been selected yet and we're not composing.
......@@ -323,20 +333,9 @@ public class ConversationListView : Gtk.TreeView, Geary.BaseInterface {
}
private void on_value_changed() {
if (!enable_load_more)
return;
// Check if we're at the very bottom of the list. If we are, it's time to
// issue a load_more signal.
Gtk.Adjustment adjustment = ((Gtk.Scrollable) this).get_vadjustment();
double upper = adjustment.get_upper();
if (adjustment.get_value() >= upper - adjustment.page_size - LOAD_MORE_HEIGHT &&
upper > last_upper) {
load_more();
last_upper = upper;
if (this.enable_load_more) {
check_load_more();
}
schedule_visible_conversations_changed();
}
private static Gtk.TreeViewColumn create_column(ConversationListStore.Column column,
......@@ -463,13 +462,6 @@ public class ConversationListView : Gtk.TreeView, Geary.BaseInterface {
}
}
private void on_row_deleted(Gtk.TreePath path) {
// if one or more rows are deleted in the model, reset the last upper limit so scrolling to
// the bottom will always activate a reload (this is particularly important if the model
// is cleared)
last_upper = -1.0;
}
private void on_rows_changed() {
schedule_visible_conversations_changed();
}
......
......@@ -85,6 +85,16 @@ public class Geary.App.ConversationMonitor : BaseObject {
/** Determines if this monitor is monitoring the base folder. */
public bool is_monitoring { get; private set; default = false; }
/** Determines if more conversations can be loaded. */
public bool can_load_more {
get {
return (
this.base_folder.properties.email_total >
this.folder_window_size
);
}
}
/** Minimum number of emails large conversations should contain. */
public int min_window_count {
get { return _min_window_count; }
......@@ -103,6 +113,13 @@ public class Geary.App.ConversationMonitor : BaseObject {
/** The set of all conversations loaded by the monitor. */
internal ConversationSet conversations { get; private set; }
/** The number of messages currently loaded from the base folder. */
internal uint folder_window_size {
get {
return (this.window.is_empty) ? 0 : this.window.size;
}
}
/** The oldest message from the base folder in the loaded window. */
internal EmailIdentifier? window_lowest {
owned get {
......@@ -116,10 +133,10 @@ public class Geary.App.ConversationMonitor : BaseObject {
private Cancellable? operation_cancellable = null;
// Set of known, in-folder emails, explicitly loaded for the
// monitor's window. This exists purely to support the
// window_lowest property above, but we need to maintain a sorted
// set of all known messages since if the last known email is
// removed, we won't know what the next lowest is. Only email
// monitor's window. This exists purely to support the window_size
// and window_lowest properties above, but we need to maintain a
// sorted set of all known messages since if the last known email
// is removed, we won't know what the next lowest is. Only email
// listed by one of the load_by_*_id methods are added here. Other
// in-folder messages pulled in for a conversation aren't added,
// since they may not be within the load window.
......@@ -345,7 +362,9 @@ public class Geary.App.ConversationMonitor : BaseObject {
/** Ensures enough conversations are present, otherwise loads more. */
internal void check_window_count() {
if (this.is_monitoring && this.conversations.size < this.min_window_count) {
if (this.is_monitoring &&
this.can_load_more &&
this.conversations.size < this.min_window_count) {
this.queue.add(new FillWindowOperation(this));
}
}
......
......@@ -46,10 +46,12 @@ private class Geary.App.FillWindowOperation : ConversationOperation {
this.monitor.window_lowest, num_to_load
);
// Check to see if we need any more, but only if we actually
// loaded some, so we don't keep loop loading when we have
// already loaded all in the folder.
if (loaded == num_to_load) {
// Check to see if we need any more, but only if there might
// be some more to load either locally or from the remote. If
// we loaded the full amount, there might be some more
// locally, so try that. If not, but the monitor thinks there
// are more to load, then we have go check the remote.
if (loaded == num_to_load || this.monitor.can_load_more) {
this.monitor.check_window_count();
}
}
......
......@@ -38,8 +38,5 @@ private class Geary.App.InsertOperation : ConversationOperation {
this.monitor.base_folder.to_string(),
this.inserted_ids.size);
yield this.monitor.load_by_sparse_id(to_insert);
// Check to see if we need any more
this.monitor.check_window_count();
}
}
......@@ -40,12 +40,6 @@ private class Geary.App.RemoveOperation : ConversationOperation {
trimmed,
(this.source_folder == this.monitor.base_folder) ? this.removed_ids : null
);
// Check we still have enough conversations if any were
// removed
if (!removed.is_empty) {
this.monitor.check_window_count();
}
}
}
......@@ -156,7 +156,6 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics {
* unless update_uid_info is true.
*/
public async void update_folder_status(Geary.Imap.FolderProperties remote_properties,
bool update_uid_info,
bool respect_marked_for_remove,
Cancellable? cancellable)
throws Error {
......@@ -199,9 +198,6 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics {
stmt.bind_rowid(2, this.folder_id);
stmt.exec(cancellable);
if (update_uid_info)
do_update_uid_info(cx, remote_properties, cancellable);
if (remote_properties.status_messages >= 0) {
do_update_last_seen_status_total(
cx, remote_properties.status_messages, cancellable
......@@ -218,11 +214,6 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics {
this.properties.recent = remote_properties.recent;
this.properties.attrs = remote_properties.attrs;
if (update_uid_info) {
this.properties.uid_validity = remote_properties.uid_validity;
this.properties.uid_next = remote_properties.uid_next;
}
// only update STATUS MESSAGES count if previously set, but use this count as the
// "authoritative" value until another SELECT/EXAMINE or MESSAGES response
if (remote_properties.status_messages >= 0) {
......@@ -1057,6 +1048,7 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics {
public async Gee.Set<ImapDB.EmailIdentifier>? mark_removed_async(
Gee.Collection<ImapDB.EmailIdentifier>? ids, bool mark_removed, Cancellable? cancellable)
throws Error {
int total_changed = 0;
int unread_count = 0;
Gee.Set<ImapDB.EmailIdentifier> removed_ids = new Gee.HashSet<ImapDB.EmailIdentifier>();
yield db.exec_transaction_async(Db.TransactionType.RW, (cx) => {
......@@ -1069,6 +1061,7 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics {
if (locs == null || locs.size == 0)
return Db.TransactionOutcome.DONE;
total_changed = locs.size;
unread_count = do_get_unread_count_for_ids(cx, ids, cancellable);
Gee.HashSet<Imap.UID> uids = new Gee.HashSet<Imap.UID>();
......@@ -1077,14 +1070,26 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics {
removed_ids.add(location.email_id);
}
if (uids.size > 0)
do_mark_unmark_removed(cx, uids, mark_removed, cancellable);
do_mark_unmark_removed(cx, uids, mark_removed, cancellable);
do_add_to_unread_count(cx, -unread_count, cancellable);
return Db.TransactionOutcome.DONE;
}, cancellable);
// Update the folder properties so client sees the changes
// right away
// Email total
if (mark_removed) {
total_changed = -total_changed;
}
int total = this.properties.select_examine_messages + total_changed;
if (total >= 0) {
this.properties.set_select_examine_message_count(total);
}
// Unread total
if (unread_count > 0)
properties.set_status_unseen(properties.email_unread - unread_count);
......
......@@ -1266,7 +1266,7 @@ internal class Geary.ImapEngine.UpdateRemoteFolders : AccountOperation {
// it's opened
try {
yield minimal_folder.local_folder.update_folder_status(
remote_folder.properties, false, false, cancellable
remote_folder.properties, false, cancellable
);
} catch (Error update_error) {
debug("Unable to update local folder %s with remote properties: %s",
......@@ -1402,7 +1402,7 @@ internal class Geary.ImapEngine.RefreshFolderUnseen : FolderOperation {
this.folder.to_string())) {
yield local_folder.update_folder_status(
remote_folder.properties, false, true, cancellable
remote_folder.properties, true, cancellable
);
((GenericAccount) this.account).update_folder(this.folder);
......
......@@ -46,7 +46,7 @@ public class Geary.MockFolder : Folder, MockObject {
SpecialFolderType type,
ProgressMonitor? monitor) {
this._account = account;
this._properties = properties;
this._properties = properties ?? new MockFolderPoperties();
this._path = path;
this._type = type;
this._opening_monitor = monitor;
......
/*
* Copyright 2019 Michael Gratton <mike@vee.net>
*
* This software is licensed under the GNU Lesser General Public License
* (version 2.1 or later). See the COPYING file in this distribution.
*/
public class Geary.MockFolderPoperties : FolderProperties {
public MockFolderPoperties() {
base(
0,
0,
Trillian.UNKNOWN,
Trillian.UNKNOWN,
Trillian.UNKNOWN,
false,
false,
false
);
}
}
......@@ -235,9 +235,6 @@ class Geary.App.ConversationMonitorTest : TestCase {
assert_int(2, monitor.size, "Initial conversation count");
assert_equal(e2.id, monitor.window_lowest, "Lowest window id");
// Removing a message will trigger another async load
this.base_folder.expect_call("list_email_by_id_async");
this.base_folder.email_removed(new Gee.ArrayList<EmailIdentifier>.wrap({e2.id}));
wait_for_signal(monitor, "conversations-removed");
assert_int(1, monitor.size, "Conversation count");
......
......@@ -18,6 +18,7 @@ geary_test_engine_sources = [
'engine/api/geary-email-identifier-mock.vala',
'engine/api/geary-email-properties-mock.vala',
'engine/api/geary-folder-mock.vala',
'engine/api/geary-folder-properties-mock.vala',
'engine/api/geary-account-information-test.vala',
'engine/api/geary-attachment-test.vala',
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment