From f61ecef13878097dcee9f86c04612006a9f27d93 Mon Sep 17 00:00:00 2001 From: Steve Howell Date: Mon, 14 May 2018 10:39:34 +0000 Subject: [PATCH] refactor: Cleanly divide data/view for add_messages. Before this change, the way to add messages had a lot of ping-pong-ing between MessageList and MessageListData, where first the data got triaged, but not actually inserted into data structures, and then subsequent calls would add the data and get filtered results. Now we have a simple API for MessageListData.add_messages that does all the data stuff up front. Having a fully function MLD.add_messages not only makes the ML.add_messages function about four lines shorter, it also sets us up to easily build standalone MLD objects before making the heavier ML objects. --- .../node_tests/message_list_data.js | 13 ++------ static/js/message_list.js | 10 +++---- static/js/message_list_data.js | 30 ++++++++++++++++--- 3 files changed, 34 insertions(+), 19 deletions(-) diff --git a/frontend_tests/node_tests/message_list_data.js b/frontend_tests/node_tests/message_list_data.js index 42d918efa9..66dd403aeb 100644 --- a/frontend_tests/node_tests/message_list_data.js +++ b/frontend_tests/node_tests/message_list_data.js @@ -31,7 +31,7 @@ run_test('basics', () => { assert.equal(mld.is_search(), false); - mld.add(make_msgs([35, 25, 15, 45])); + mld.add_anywhere(make_msgs([35, 25, 15, 45])); function assert_contents(msg_ids) { const msgs = mld.all_messages(); @@ -40,7 +40,7 @@ run_test('basics', () => { assert_contents([15, 25, 35, 45]); const new_msgs = make_msgs([10, 20, 30, 40, 50, 60, 70]); - const info = mld.triage_messages(new_msgs); + const info = mld.add_messages(new_msgs); assert.deepEqual(info, { top_messages: make_msgs([10]), @@ -48,13 +48,6 @@ run_test('basics', () => { bottom_messages: make_msgs([50, 60, 70]), }); - mld.prepend(info.top_messages); - mld.append(info.bottom_messages); - - assert_contents([10, 15, 25, 35, 45, 50, 60, 70]); - - mld.add(info.interior_messages); - assert_contents([10, 15, 20, 25, 30, 35, 40, 45, 50, 60, 70]); assert.equal(mld.selected_id(), -1); @@ -86,7 +79,7 @@ run_test('basics', () => { assert.equal(mld.closest_id(99), -1); assert.equal(mld.get_last_message_sent_by_me(), undefined); - mld.add(make_msgs([120, 125.01, 130, 140])); + mld.add_messages(make_msgs([120, 125.01, 130, 140])); assert_contents([120, 125.01, 130, 140]); mld.set_selected_id(125.01); assert.equal(mld.selected_id(), 125.01); diff --git a/static/js/message_list.js b/static/js/message_list.js index 4ded946638..88623ed0a0 100644 --- a/static/js/message_list.js +++ b/static/js/message_list.js @@ -27,24 +27,24 @@ exports.MessageList = function (table_name, filter, opts) { exports.MessageList.prototype = { add_messages: function MessageList_add_messages(messages, opts) { var self = this; - var info = this.data.triage_messages(messages); + + // This adds all messages to our data, but only returns + // the currently viewable ones. + var info = this.data.add_messages(messages); + var top_messages = info.top_messages; var bottom_messages = info.bottom_messages; var interior_messages = info.interior_messages; if (interior_messages.length > 0) { - var all_messages = top_messages.concat(interior_messages).concat(bottom_messages); - self.data.add(all_messages); self.view.rerender_the_whole_thing(); return true; } if (top_messages.length > 0) { - top_messages = this.data.prepend(top_messages); self.view.prepend(top_messages); } if (bottom_messages.length > 0) { - bottom_messages = this.data.append(bottom_messages); self.append_to_view(bottom_messages, opts); } diff --git a/static/js/message_list_data.js b/static/js/message_list_data.js index 841b7a21f0..d1259e70aa 100644 --- a/static/js/message_list_data.js +++ b/static/js/message_list_data.js @@ -163,7 +163,7 @@ MessageListData.prototype = { }); }, - triage_messages: function (messages) { + add_messages: function (messages) { var self = this; var top_messages = []; var bottom_messages = []; @@ -197,15 +197,37 @@ MessageListData.prototype = { }); } - return { + // We return probably more info than our actual callers + // need, but this is useful for tests and not actually + // expensive. + var info = { top_messages: top_messages, bottom_messages: bottom_messages, interior_messages: interior_messages, }; + + if (interior_messages.length > 0) { + var all_messages = top_messages.concat(interior_messages).concat(bottom_messages); + self.add_anywhere(all_messages); + return info; + } + + if (top_messages.length > 0) { + top_messages = self.prepend(top_messages); + } + + if (bottom_messages.length > 0) { + bottom_messages = self.append(bottom_messages); + } + + return info; }, - add: function (messages) { - // Caller should have already filtered + add_anywhere: function (messages) { + // Caller should have already filtered messages. + // This should be used internally when we have + // "interior" messages to add and can't optimize + // things by only doing prepend or only doing append. var viewable_messages; if (this.muting_enabled) { this._all_items = messages.concat(this._all_items);