From f0e18b3b3e5c5a0594291f6a010d2a47bbb49b33 Mon Sep 17 00:00:00 2001
From: Steve Howell <showell@zulipchat.com>
Date: Fri, 31 Jan 2020 19:15:46 +0000
Subject: [PATCH] topic list: Use vdom techniques.

We avoid complicated code to update unread counts
by just using vdom.js.

One small change here is that if click on "more
topics", we replace it with the spinner instead
of putting the spinner after it.  This saves us
a redraw under the new scheme.
---
 frontend_tests/node_tests/stream_list.js |  26 +--
 static/js/stream_list.js                 |  22 +--
 static/js/topic_list.js                  | 237 ++++++++++-------------
 static/js/unread_ui.js                   |   1 +
 static/styles/left-sidebar.scss          |   4 -
 5 files changed, 105 insertions(+), 185 deletions(-)

diff --git a/frontend_tests/node_tests/stream_list.js b/frontend_tests/node_tests/stream_list.js
index c284b91843..8f7d3cb74b 100644
--- a/frontend_tests/node_tests/stream_list.js
+++ b/frontend_tests/node_tests/stream_list.js
@@ -3,8 +3,8 @@ set_global('$', global.make_zjquery());
 set_global('blueslip', global.make_zblueslip());
 set_global('i18n', global.stub_i18n);
 
-const FoldDict = zrequire('fold_dict').FoldDict;
 const IntDict = zrequire('int_dict').IntDict;
+
 zrequire('unread_ui');
 zrequire('Filter', 'js/filter');
 zrequire('util');
@@ -435,7 +435,7 @@ run_test('narrowing', () => {
     };
 
     stream_list.handle_narrow_deactivated();
-    assert.equal(removed_classes, 'active-filter active-sub-filter');
+    assert.equal(removed_classes, 'active-filter');
     assert(topics_closed);
 });
 
@@ -663,28 +663,6 @@ run_test('update_count_in_dom', () => {
     stream_list.update_dom_with_unread_counts(counts);
     assert.equal($('<stream-value>').text(), '99');
     assert(stream_li.hasClass('stream-with-count'));
-
-    let topic_results;
-
-    topic_list.set_count = function (stream_id, topic, count) {
-        topic_results = {
-            stream_id: stream_id,
-            topic: topic,
-            count: count,
-        };
-    };
-
-    const topic_count = new FoldDict();
-    topic_count.set('lunch', '555');
-    counts.topic_count.set(stream_id, topic_count);
-
-    stream_list.update_dom_with_unread_counts(counts);
-
-    assert.deepEqual(topic_results, {
-        stream_id: stream_id,
-        topic: 'lunch',
-        count: 555,
-    });
 });
 
 narrow_state.active = () => false;
diff --git a/static/js/stream_list.js b/static/js/stream_list.js
index e4190f7a90..b79724475c 100644
--- a/static/js/stream_list.js
+++ b/static/js/stream_list.js
@@ -320,26 +320,6 @@ exports.update_dom_with_unread_counts = function (counts) {
     for (const [stream_id, count] of counts.stream_count) {
         set_stream_unread_count(stream_id, count);
     }
-
-    // counts.topic_count maps streams to hashes of topics to counts
-    for (const [stream_id, topic_hash] of counts.topic_count) {
-        // Because the topic_list data structure doesn't keep track of
-        // which topics the "more topics" unread count came from, we
-        // need to compute the correct value from scratch here.
-        let more_topics_total = 0;
-        for (const [topic, count] of topic_hash) {
-            const in_more_topics = topic_list.set_count(stream_id, topic, count);
-            if (in_more_topics === true) {
-                more_topics_total += count;
-            }
-        }
-        if (topic_list.active_stream_id() === stream_id) {
-            // Update the "more topics" unread count; we communicate
-            // this to the `topic_list` library by passing `null` as
-            // the topic.
-            topic_list.set_count(stream_id, null, more_topics_total);
-        }
-    }
 };
 
 exports.rename_stream = function (sub) {
@@ -398,7 +378,7 @@ exports.get_sidebar_stream_topic_info  = function (filter) {
 };
 
 function deselect_stream_items() {
-    $("ul#stream_filters li").removeClass('active-filter active-sub-filter');
+    $("ul#stream_filters li").removeClass('active-filter');
 }
 
 exports.update_stream_sidebar_for_narrow = function (filter) {
diff --git a/static/js/topic_list.js b/static/js/topic_list.js
index 4010b0f4cf..2303f01ed8 100644
--- a/static/js/topic_list.js
+++ b/static/js/topic_list.js
@@ -1,7 +1,6 @@
 const render_more_topics = require('../templates/more_topics.hbs');
 const render_more_topics_spinner = require('../templates/more_topics_spinner.hbs');
 const render_topic_list_item = require('../templates/topic_list_item.hbs');
-const FoldDict = require('./fold_dict').FoldDict;
 const IntDict = require('./int_dict').IntDict;
 const topic_list_data = require('./topic_list_data');
 
@@ -18,6 +17,12 @@ const active_widgets = new IntDict();
 // We know whether we're zoomed or not.
 let zoomed = false;
 
+exports.update = function () {
+    for (const widget of active_widgets.values()) {
+        widget.build();
+    }
+};
+
 exports.remove_expanded_topics = function () {
     stream_popover.hide_topic_popover();
 
@@ -50,43 +55,73 @@ exports.zoom_out = function () {
     exports.rebuild(parent_widget, stream_id);
 };
 
-function update_unread_count(unread_count_elem, count) {
-    // unread_count_elem is a jquery element...we expect DOM
-    // to look like this:
-    //   <div class="topic-unread-count {{#if is_zero}}zero_count{{/if}}">
-    //        <div class="value">{{unread}}</div>
-    //   </div>
-    const value_span = unread_count_elem.find('.value');
+exports.keyed_topic_li = (convo) => {
+    const render = () => {
+        return render_topic_list_item(convo);
+    };
 
-    if (value_span.length === 0) {
-        blueslip.error('malformed dom for unread count');
-        return;
-    }
+    const eq = (other) => {
+        return _.isEqual(convo, other.convo);
+    };
 
-    if (count === 0) {
-        unread_count_elem.addClass("zero_count");
-        value_span.text('');
-    } else {
-        unread_count_elem.removeClass("zero_count");
-        unread_count_elem.show();
-        value_span.text(count);
-    }
-}
+    const key = 't:' + convo.topic_name;
 
-exports.set_count = function (stream_id, topic, count) {
-    const widget = active_widgets.get(stream_id);
+    return {
+        key: key,
+        render: render,
+        convo: convo,
+        eq: eq,
+    };
+};
 
-    if (widget === undefined) {
-        return false;
-    }
+exports.more_li = (more_topics_unreads) => {
+    const render = () => {
+        return render_more_topics({
+            more_topics_unreads: more_topics_unreads,
+        });
+    };
 
-    return widget.set_count(topic, count);
+    const eq = (other) => {
+        return other.more_items &&
+            more_topics_unreads === other.more_topics_unreads;
+    };
+
+    const key = 'more';
+
+    return {
+        key: key,
+        more_items: true,
+        more_topics_unreads: more_topics_unreads,
+        render: render,
+        eq: eq,
+    };
+};
+
+exports.spinner_li = () => {
+    const render = () => {
+        return render_more_topics_spinner();
+    };
+
+    const eq = (other) => {
+        return other.spinner;
+    };
+
+    const key = 'more';
+
+    return {
+        key: key,
+        spinner: true,
+        render: render,
+        eq: eq,
+    };
 };
 
 exports.widget = function (parent_elem, my_stream_id) {
     const self = {};
 
-    self.build_list = function () {
+    self.prior_dom = undefined;
+
+    self.build_list = function (spinner) {
         const list_info = topic_list_data.get_list_info(
             my_stream_id, zoomed);
 
@@ -97,35 +132,25 @@ exports.widget = function (parent_elem, my_stream_id) {
             list_info.items.length === num_possible_topics &&
             topic_data.is_complete_for_stream_id(my_stream_id);
 
-        const ul = $('<ul class="topic-list">');
+        const attrs = [
+            ['class', 'topic-list'],
+        ];
 
-        self.topic_items = new FoldDict();
+        const nodes = _.map(
+            list_info.items, exports.keyed_topic_li);
 
-        // This is the main list of topics:
-        //    topic1
-        //    topic2
-        //    topic3
-        _.each(list_info.items, (topic_info) => {
-            const li = $(render_topic_list_item(topic_info));
-            self.topic_items.set(topic_info.topic_name, li);
-            ul.append(li);
+        if (spinner) {
+            nodes.push(exports.spinner_li());
+        } else if (!is_showing_all_possible_topics) {
+            nodes.push(exports.more_li(more_topics_unreads));
+        }
+
+        const dom = vdom.ul({
+            attrs: attrs,
+            keyed_nodes: nodes,
         });
 
-        // Now, we decide whether we need to show the "more topics"
-        // widget.  We need it if there are at least 5 topics in the
-        // frontend's cache, or if we (possibly) don't have all
-        // historical topics in the browser's cache.
-
-        if (!is_showing_all_possible_topics) {
-            const show_more_html = render_more_topics({
-                more_topics_unreads: more_topics_unreads,
-            });
-            ul.append($(show_more_html));
-
-            const spinner = render_more_topics_spinner();
-            ul.append($(spinner));
-        }
-        return ul;
+        return dom;
     };
 
     self.get_parent = function () {
@@ -136,94 +161,26 @@ exports.widget = function (parent_elem, my_stream_id) {
         return my_stream_id;
     };
 
-    self.get_dom = function () {
-        return self.dom;
-    };
-
     self.remove = function () {
-        self.dom.remove();
+        parent_elem.find('.topic-list').remove();
+        self.prior_dom = undefined;
     };
 
-    self.num_items = function () {
-        return self.topic_items.size;
-    };
+    self.build = function (spinner) {
+        const new_dom = self.build_list(spinner);
 
-    self.set_count = function (topic, count) {
-        let unread_count_elem;
-        if (topic === null) {
-            // null is used for updating the "more topics" count.
-            if (zoomed) {
-                return false;
-            }
-            const unread_count_parent = $(".show-more-topics");
-            if (unread_count_parent.length === 0) {
-                // If no show-more-topics element is present in the
-                // DOM, there are two possibilities.  The most likely
-                // is that there are simply no unreads on that topic
-                // and there should continue to not be a "more topics"
-                // button; we can check this by looking at count.
-                if (count === 0) {
-                    return false;
-                }
-
-                // The alternative is that there is these new messages
-                // create the need for a "more topics" widget with a
-                // nonzero unread count, and we need to create one and
-                // add it to the DOM.
-                //
-                // With our current implementation, this code path
-                // will always have its results overwritten shortly
-                // after, because (1) the can only happen when we just
-                // added unread counts, (not removing them), and (2)
-                // when learning about new (unread) messages,
-                // stream_list.update_dom_with_unread_count is always
-                // immediately followed by
-                // stream_list.update_streams_sidebar, which will
-                // rebuilds the topic list from scratch anyway.
-                //
-                // So this code mostly exists to document this corner
-                // case if in the future we adjust the model for
-                // managing unread counts.  The code for updating this
-                // element would look something like the following:
-                //
-                // var show_more = self.build_more_topics_section(count);
-                // var topic_list_ul = exports.get_stream_li().find(".topic-list").expectOne();
-                // topic_list_ul.append(show_more);
-                return false;
-            }
-            unread_count_elem = unread_count_parent.find(".topic-unread-count");
-            update_unread_count(unread_count_elem, count);
-            return false;
+        function replace_content(html) {
+            self.remove();
+            parent_elem.append(html);
         }
 
-        if (!self.topic_items.has(topic)) {
-            // `topic_li` may not exist if the topic is behind "more
-            // topics"; We need to update the "more topics" count
-            // instead in that case; we do this by returning true to
-            // notify the caller to accumulate these.
-            if (muting.is_topic_muted(my_stream_id, topic)) {
-                // But we don't count unreads in muted topics.
-                return false;
-            }
-            return true;
+        function find() {
+            return parent_elem.find('.topic-list');
         }
 
-        const topic_li = self.topic_items.get(topic);
-        unread_count_elem = topic_li.find('.topic-unread-count');
-        update_unread_count(unread_count_elem, count);
-        return false;
-    };
+        vdom.update(replace_content, find, new_dom, self.prior_dom);
 
-    self.show_spinner = function () {
-        // The spinner will go away once we get results and redraw
-        // the whole list.
-        const spinner = self.dom.find('.searching-for-more-topics');
-        spinner.show();
-    };
-
-    self.build = function () {
-        self.dom = self.build_list();
-        parent_elem.append(self.dom);
+        self.prior_dom = new_dom;
     };
 
     return self;
@@ -251,6 +208,13 @@ exports.get_stream_li = function () {
 };
 
 exports.rebuild = function (stream_li, stream_id) {
+    const active_widget = active_widgets.get(stream_id);
+
+    if (active_widget) {
+        active_widget.build();
+        return;
+    }
+
     exports.remove_expanded_topics();
     const widget = exports.widget(stream_li, stream_id);
     widget.build();
@@ -286,13 +250,14 @@ exports.zoom_in = function () {
             return;
         }
 
-        const widget = active_widgets.get(stream_id);
-
-        exports.rebuild(widget.get_parent(), stream_id);
+        active_widget.build();
     }
 
     ui.get_scroll_element($('#stream-filters-container')).scrollTop(0);
-    active_widget.show_spinner();
+
+    const spinner = true;
+    active_widget.build(spinner);
+
     topic_data.get_server_history(stream_id, on_success);
 };
 
diff --git a/static/js/unread_ui.js b/static/js/unread_ui.js
index 5d0b5378a8..eac828f071 100644
--- a/static/js/unread_ui.js
+++ b/static/js/unread_ui.js
@@ -51,6 +51,7 @@ exports.update_unread_counts = function () {
     top_left_corner.update_dom_with_unread_counts(res);
     stream_list.update_dom_with_unread_counts(res);
     pm_list.update_dom_with_unread_counts(res);
+    topic_list.update();
     notifications.update_pm_count(res.private_message_count);
     const notifiable_unread_count = unread.calculate_notifiable_count(res);
     notifications.update_title_count(notifiable_unread_count);
diff --git a/static/styles/left-sidebar.scss b/static/styles/left-sidebar.scss
index 9d2d9242ab..00f64b293f 100644
--- a/static/styles/left-sidebar.scss
+++ b/static/styles/left-sidebar.scss
@@ -518,10 +518,6 @@ li.expanded_private_message a {
     visibility: hidden;
 }
 
-.searching-for-more-topics {
-    display: none;
-}
-
 .searching-for-more-topics img {
     height: 16px;
     margin-left: 6px;