topic_data.js: Refactor topic history internals.

This commit introduces a per-stream topic_history class
inside of topic_data.js to better encapsulate how we store topic
history.

To the callers, nothing changes here.  (Some of our non-black-box
node tests change their way of setting up data, though, since the
internal data structures are different.)

The new class has the following improvements:

    * We use message_id instead of timestamp as our sorting key.
      (We could have done this in a prep commit, but it wouldn't
      have made the diff much cleaner here.)

    * We use a dictionary instead of a sorted list to store the
      data, so that writes are O(1) instead of O(NlogN).  Reads
      now do sorts, so they're O(NlogN) instead of O(N), but reads
      are fairly infrequent.  (The main goal here isn't actually
      performance, but instead it just simplifies the
      implementation.)

    * We isolate `topic_history` from the format of the messages.
      This prepares us for upcoming changes where updates to the
      data structure may come from topic history queries as well
      as messages.

    * We split out the message-add path from the message-remove
      path.  This prepares us to eventually get rid of the "count"
      mechanism that is kind of fragile and which has to be
      bypassed for historical topics.

frontend_tests/node_tests/search_suggestion.js

@@ -36,7 +36,7 @@ init();
 
 set_global('narrow', {});
 
-topic_data.populate_for_tests({});
+topic_data.reset();
 
 (function test_basic_get_suggestions() {
     var query = 'fred';
@@ -575,25 +575,24 @@ init();
         }
     };
 
-    var recent_data = {};
-    topic_data.populate_for_tests(recent_data);
+    topic_data.reset();
     suggestions = search.get_suggestions('te');
     expected = [
         "te",
     ];
     assert.deepEqual(suggestions.strings, expected);
 
-    recent_data[devel_id] = [
-        {name: 'REXX'},
-    ];
+    topic_data.add_message({
+        stream_id: devel_id,
+        topic_name: 'REXX',
+    });
 
-    recent_data[office_id] = [
-        {name: 'team'},
-        {name: 'ignore'},
-        {name: 'test'},
-    ];
-
-    topic_data.populate_for_tests(recent_data);
+    _.each(['team', 'ignore', 'test'], function (topic_name) {
+        topic_data.add_message({
+            stream_id: office_id,
+            topic_name: topic_name,
+        });
+    });
 
     suggestions = search.get_suggestions('te');
     expected = [
@@ -678,7 +677,7 @@ init();
         return;
     };
 
-    topic_data.populate_for_tests({});
+    topic_data.reset();
 
     var suggestions = search.get_suggestions(query);
 
@@ -698,7 +697,7 @@ init();
         return;
     };
 
-    topic_data.populate_for_tests({});
+    topic_data.reset();
 
     var query = 'stream:of';
     var suggestions = search.get_suggestions(query);
@@ -750,13 +749,7 @@ init();
     people.add(alice);
 
 
-    topic_data.populate_for_tests({
-        office: [
-            {name: 'team'},
-            {name: 'ignore'},
-            {name: 'test'},
-        ],
-    });
+    topic_data.reset();
 
     var suggestions = search.get_suggestions(query);
 
@@ -882,7 +875,7 @@ init();
         return;
     };
 
-    topic_data.populate_for_tests({});
+    topic_data.reset();
 
     // test allowing spaces with quotes surrounding operand
     var query = 'stream:"dev he"';

frontend_tests/node_tests/stream_data.js

@@ -252,37 +252,53 @@ var people = global.people;
 
     stream_data.add_sub('Rome', rome);
 
-    var message = {
+    var message1 = {
         stream_id: stream_id,
-        timestamp: 101,
+        id: 101,
         subject: 'toPic1',
     };
-    topic_data.process_message(message);
+    topic_data.process_message(message1);
 
     var history = topic_data.get_recent_names(rome.stream_id);
     assert.deepEqual(history, ['toPic1']);
 
-    message = {
+    var message2 = {
         stream_id: stream_id,
-        timestamp: 102,
+        id: 102,
         subject: 'Topic1',
     };
-    topic_data.process_message(message);
+    topic_data.process_message(message2);
     history = topic_data.get_recent_names(rome.stream_id);
     assert.deepEqual(history, ['Topic1']);
 
-    message = {
+    var message3 = {
         stream_id: stream_id,
-        timestamp: 103,
+        id: 103,
         subject: 'topic2',
     };
-    topic_data.process_message(message);
+    topic_data.process_message(message3);
     history = topic_data.get_recent_names(rome.stream_id);
     assert.deepEqual(history, ['topic2', 'Topic1']);
 
-    topic_data.process_message(message, true);
+    // Removing first topic1 message has no effect.
+    topic_data.process_message(message1, true);
     history = topic_data.get_recent_names(rome.stream_id);
-    assert.deepEqual(history, ['Topic1']);
+    assert.deepEqual(history, ['topic2', 'Topic1']);
+
+    // Remove second topic1 message removes the topic.
+    topic_data.process_message(message2, true);
+    history = topic_data.get_recent_names(rome.stream_id);
+    assert.deepEqual(history, ['topic2']);
+
+    // Test that duplicate remove does not crash us.
+    topic_data.process_message(message2, true);
+    history = topic_data.get_recent_names(rome.stream_id);
+    assert.deepEqual(history, ['topic2']);
+
+    // get to 100% coverage for defensive code
+    topic_data.remove_message({
+        stream_id: 9999999,
+    });
 }());
 
 (function test_is_active() {
@@ -306,7 +322,7 @@ var people = global.people;
 
     var message = {
         stream_id: 222,
-        timestamp: 108,
+        id: 108,
         subject: 'topic2',
     };
     topic_data.process_message(message);

frontend_tests/node_tests/topic_list.js

@@ -24,11 +24,13 @@ global.compile_template('topic_list_item');
     var active_topic = "testing";
     var max_topics = 5;
 
-    var recent_topics = {};
-    recent_topics[stream_id] = [
-        {name: "coding"},
-    ];
-    topic_data.populate_for_tests(recent_topics);
+    topic_data.reset();
+    topic_data.add_message({
+        stream_id: stream_id,
+        topic_name: 'coding',
+        message_id: 400,
+    });
+
     global.unread.num_unread_for_subject = function () {
         return 1;
     };

static/js/topic_data.js

@@ -8,58 +8,136 @@ exports.stream_has_topics = function (stream_id) {
     return stream_dict.has(stream_id);
 };
 
-exports.process_message = function (message, remove_message) {
-    var current_timestamp = 0;
-    var count = 0;
-    var stream_id = message.stream_id;
-    var canon_topic = message.subject.toLowerCase();
+exports.topic_history = function () {
+    var topics = new Dict({fold_case: true});
 
-    var recents = stream_dict.get(stream_id) || [];
+    var self = {};
 
-    recents = _.filter(recents, function (item) {
-        var is_duplicate = (
-            item.name.toLowerCase() === canon_topic);
-        if (is_duplicate) {
-            current_timestamp = item.timestamp;
-            count = item.count;
+    self.add_or_update = function (opts) {
+        var name = opts.name;
+        var message_id = opts.message_id || 0;
+
+        message_id = parseInt(message_id, 10);
+
+        var existing = topics.get(name);
+
+        if (!existing) {
+            topics.set(opts.name, {
+                message_id: message_id,
+                pretty_name: name,
+                count: 1,
+            });
+            return;
         }
-        return !is_duplicate;
-    });
 
-    if (remove_message !== undefined) {
-        count = count - 1;
-    } else {
-        count = count + 1;
+        existing.count += 1;
+        if (message_id > existing.message_id) {
+            existing.message_id = message_id;
+            existing.pretty_name = name;
+        }
+    };
+
+    self.maybe_remove = function (topic_name) {
+        var existing = topics.get(topic_name);
+
+        if (!existing) {
+            return;
+        }
+
+        if (existing.count <= 1) {
+            topics.del(topic_name);
+            return;
+        }
+
+        existing.count -= 1;
+    };
+
+    self.get_recent_names = function () {
+        var recents = topics.values();
+
+        recents.sort(function (a, b) {
+            return b.message_id - a.message_id;
+        });
+
+        var names = _.map(recents, function (obj) {
+            return obj.pretty_name;
+        });
+
+        return names;
+    };
+
+    return self;
+};
+
+exports.process_message = function (message, remove_message) {
+    if (remove_message) {
+        exports.remove_message({
+            stream_id: message.stream_id,
+            topic_name: message.subject,
+        });
+        return;
     }
 
-    if (count !== 0) {
-        recents.push({name: message.subject,
-                      count: count,
-                      timestamp: Math.max(message.timestamp, current_timestamp)});
+    exports.add_message({
+        stream_id: message.stream_id,
+        topic_name: message.subject,
+        message_id: message.id,
+    });
+};
+
+exports.remove_message = function (opts) {
+    var stream_id = opts.stream_id;
+    var name = opts.topic_name;
+    var history = stream_dict.get(stream_id);
+
+    // This is the special case of "removing" a message from
+    // a topic, which happens when we edit topics.
+
+    if (!history) {
+        return;
     }
 
-    recents.sort(function (a, b) {
-        return b.timestamp - a.timestamp;
-    });
+    // This is the normal case of an incoming message.
+    history.maybe_remove(name);
+};
 
-    stream_dict.set(stream_id, recents);
+exports.find_or_create = function (stream_id) {
+    var history = stream_dict.get(stream_id);
+
+    if (!history) {
+        history = exports.topic_history();
+        stream_dict.set(stream_id, history);
+    }
+
+    return history;
+};
+
+exports.add_message = function (opts) {
+    var stream_id = opts.stream_id;
+    var message_id = opts.message_id;
+    var name = opts.topic_name;
+
+    var history = exports.find_or_create(stream_id);
+
+    history.add_or_update({
+        name: name,
+        message_id: message_id,
+    });
 };
 
 exports.get_recent_names = function (stream_id) {
-    var topic_objs = stream_dict.get(stream_id);
+    var history = stream_dict.get(stream_id);
 
-    if (!topic_objs) {
+    if (!history) {
         return [];
     }
 
-    return _.map(topic_objs, function (obj) {
-        return obj.name;
-    });
+    return history.get_recent_names();
 };
 
-exports.populate_for_tests = function (stream_map) {
+exports.reset = function () {
     // This is only used by tests.
-    stream_dict = Dict.from(stream_map);
+    stream_dict = new Dict();
 };
 
 return exports;