Revamp state tracking for outbound typing indicators.

This change moves most of the logic related to starting and
stopping outbound typing indicators to a new module called
typing_status.js that is heavily unit tested.

While this was in some sense a rewrite, the logic was mostly
inspired by the existing code.

This change does fix one known bug, which is that when we
were changing recipients before (while typing was active), we
were not stopping and starting typing indicators.  This was
a fairly minor bug, since usually users leave the compose
box to change recipients, and we would do stop/start under
that scenario.  Now we also handle the case where the user
does not leave the compose box to change recipients.

.eslintrc.json

@@ -37,6 +37,7 @@
         "loading": false,
         "typing": false,
         "typing_data": false,
+        "typing_status": false,
         "compose": false,
         "compose_actions": false,
         "compose_state": false,

frontend_tests/node_tests/typing_status.js

@@ -0,0 +1,280 @@
+var typing_status = require('js/typing_status');
+
+function return_false() { return false; }
+function return_true() { return true; }
+function return_alice() { return "alice"; }
+function return_bob() { return "bob"; }
+
+function make_time(secs) {
+    // make times semi-realistic
+    return 1000000 + 1000 * secs;
+}
+
+function returns_time(secs) {
+    return function () { return make_time(secs); };
+}
+
+(function test_basics() {
+
+    // invalid conversation basically does nothing
+    var worker = {
+        get_recipient: return_alice,
+        is_valid_conversation: return_false,
+    };
+    typing_status.handle_text_input(worker);
+
+    // Start setting up more testing state.
+    typing_status.initialize_state();
+
+    var events = {};
+
+    function set_timeout(f, delay) {
+        assert.equal(delay, 5000);
+        events.idle_callback = f;
+        return 'idle_timer_stub';
+    }
+
+    function clear_timeout() {
+        events.timer_cleared = true;
+    }
+
+    global.patch_builtin('setTimeout', set_timeout);
+    global.patch_builtin('clearTimeout', clear_timeout);
+
+    function notify_server_start(recipient) {
+        assert.equal(recipient, "alice");
+        events.started = true;
+    }
+
+    function notify_server_stop(recipient) {
+        assert.equal(recipient, "alice");
+        events.stopped = true;
+    }
+
+    function clear_events() {
+        events.idle_callback = undefined;
+        events.started = false;
+        events.stopped = false;
+        events.timer_cleared = false;
+    }
+
+    function call_handler() {
+        clear_events();
+        typing_status.handle_text_input(worker);
+    }
+
+    function call_stop() {
+        clear_events();
+        typing_status.stop(worker);
+    }
+
+    worker = {
+        get_recipient: return_alice,
+        is_valid_conversation: return_true,
+        get_current_time: returns_time(5),
+        notify_server_start: notify_server_start,
+        notify_server_stop: notify_server_stop,
+    };
+
+    // Start talking to alice.
+    call_handler();
+    assert.deepEqual(typing_status.state, {
+        next_send_start_time: make_time(5 + 10),
+        idle_timer: 'idle_timer_stub',
+        current_recipient: 'alice',
+    });
+    assert.deepEqual(events, {
+        idle_callback: events.idle_callback,
+        started: true,
+        stopped: false,
+        timer_cleared: false,
+    });
+    assert(events.idle_callback);
+
+    // type again 3 seconds later
+    worker.get_current_time = returns_time(8);
+    call_handler();
+    assert.deepEqual(typing_status.state, {
+        next_send_start_time: make_time(5 + 10),
+        idle_timer: 'idle_timer_stub',
+        current_recipient: 'alice',
+    });
+    assert.deepEqual(events, {
+        idle_callback: events.idle_callback,
+        started: false,
+        stopped: false,
+        timer_cleared: true,
+    });
+    assert(events.idle_callback);
+
+    // type after 15 secs, so that we can notify the server
+    // again
+    worker.get_current_time = returns_time(18);
+    call_handler();
+    assert.deepEqual(typing_status.state, {
+        next_send_start_time: make_time(18 + 10),
+        idle_timer: 'idle_timer_stub',
+        current_recipient: 'alice',
+    });
+    assert.deepEqual(events, {
+        idle_callback: events.idle_callback,
+        started: true,
+        stopped: false,
+        timer_cleared: true,
+    });
+
+    // Now call alice's idle callback that we captured earlier.
+    var callback = events.idle_callback;
+    clear_events();
+    callback();
+    assert.deepEqual(typing_status.state, {
+        next_send_start_time: undefined,
+        idle_timer: undefined,
+        current_recipient: undefined,
+    });
+    assert.deepEqual(events, {
+        idle_callback: undefined,
+        started: false,
+        stopped: true,
+        timer_cleared: true,
+    });
+
+    // Call stop with nothing going on.
+    call_stop();
+    assert.deepEqual(typing_status.state, {
+        next_send_start_time: undefined,
+        idle_timer: undefined,
+        current_recipient: undefined,
+    });
+    assert.deepEqual(events, {
+        idle_callback: undefined,
+        started: false,
+        stopped: false,
+        timer_cleared: false,
+    });
+
+    // Start talking to alice again.
+    worker.get_current_time = returns_time(50);
+    call_handler();
+    assert.deepEqual(typing_status.state, {
+        next_send_start_time: make_time(50 + 10),
+        idle_timer: 'idle_timer_stub',
+        current_recipient: 'alice',
+    });
+    assert.deepEqual(events, {
+        idle_callback: events.idle_callback,
+        started: true,
+        stopped: false,
+        timer_cleared: false,
+    });
+    assert(events.idle_callback);
+
+    // Explicitly stop alice.
+    call_stop();
+    assert.deepEqual(typing_status.state, {
+        next_send_start_time: undefined,
+        idle_timer: undefined,
+        current_recipient: undefined,
+    });
+    assert.deepEqual(events, {
+        idle_callback: undefined,
+        started: false,
+        stopped: true,
+        timer_cleared: true,
+    });
+
+    // Start talking to alice again.
+    worker.get_current_time = returns_time(80);
+    call_handler();
+    assert.deepEqual(typing_status.state, {
+        next_send_start_time: make_time(80 + 10),
+        idle_timer: 'idle_timer_stub',
+        current_recipient: 'alice',
+    });
+    assert.deepEqual(events, {
+        idle_callback: events.idle_callback,
+        started: true,
+        stopped: false,
+        timer_cleared: false,
+    });
+    assert(events.idle_callback);
+
+    // Switch to an invalid conversation.
+    worker.get_recipient = function () {
+        return 'not-alice';
+    };
+    worker.is_valid_conversation = return_false;
+    call_handler();
+    assert.deepEqual(typing_status.state, {
+        next_send_start_time: undefined,
+        idle_timer: undefined,
+        current_recipient: undefined,
+    });
+    assert.deepEqual(events, {
+        idle_callback: undefined,
+        started: false,
+        stopped: true,
+        timer_cleared: true,
+    });
+
+    // Switch to another invalid conversation.
+    worker.get_recipient = function () {
+        return 'another-bogus-one';
+    };
+    worker.is_valid_conversation = return_false;
+    call_handler();
+    assert.deepEqual(typing_status.state, {
+        next_send_start_time: undefined,
+        idle_timer: undefined,
+        current_recipient: undefined,
+    });
+    assert.deepEqual(events, {
+        idle_callback: undefined,
+        started: false,
+        stopped: false,
+        timer_cleared: false,
+    });
+
+    // Start talking to alice again.
+    worker.get_recipient = return_alice;
+    worker.is_valid_conversation = return_true;
+    worker.get_current_time = returns_time(170);
+    call_handler();
+    assert.deepEqual(typing_status.state, {
+        next_send_start_time: make_time(170 + 10),
+        idle_timer: 'idle_timer_stub',
+        current_recipient: 'alice',
+    });
+    assert.deepEqual(events, {
+        idle_callback: events.idle_callback,
+        started: true,
+        stopped: false,
+        timer_cleared: false,
+    });
+    assert(events.idle_callback);
+
+    // Switch to bob now.
+    worker.get_recipient = return_bob;
+    worker.is_valid_conversation = return_true;
+    worker.get_current_time = returns_time(171);
+
+    worker.notify_server_start = function (recipient) {
+        assert.equal(recipient, "bob");
+        events.started = true;
+    };
+
+    call_handler();
+    assert.deepEqual(typing_status.state, {
+        next_send_start_time: make_time(171 + 10),
+        idle_timer: 'idle_timer_stub',
+        current_recipient: 'bob',
+    });
+    assert.deepEqual(events, {
+        idle_callback: events.idle_callback,
+        started: true,
+        stopped: true,
+        timer_cleared: true,
+    });
+    assert(events.idle_callback);
+
+}());

static/js/typing.js

@@ -3,22 +3,12 @@ var exports = {};
 // How long before we assume a client has gone away
 // and expire its typing status
 var TYPING_STARTED_EXPIRY_PERIOD = 15000; // 15s
-// How frequently 'still typing' notifications are sent
-// to extend the expiry
-var TYPING_STARTED_SEND_FREQUENCY = 10000; // 10s
-// How long after someone stops editing in the compose box
-// do we send a 'stopped typing' notification
-var TYPING_STOPPED_WAIT_PERIOD = 5000; // 5s
 
-var current_recipient;
+// Note!: There are also timing constants in typing_status.js
+// that make typing indicators work.
+
 var stop_typing_timers = new Dict();
 
-// Our logic is a bit too complex to encapsulate in
-// _.throttle/_.debounce (since we need to cancel things), so we do it
-// manually.
-var stop_timer;
-var last_start_time;
-
 function send_typing_notification_ajax(recipients, operation) {
     channel.post({
         url: '/json/typing',
@@ -33,61 +23,59 @@ function send_typing_notification_ajax(recipients, operation) {
     });
 }
 
-function check_and_send(operation) {
+function get_recipient() {
     var compose_recipient = compose_state.recipient();
-    var compose_nonempty = compose_state.has_message_content();
-
-    // If we currently have an active typing notification out, and we
-    // want to send a stop notice, or the compose recipient changed
-    // (and implicitly we're sending a start notice), send a stop
-    // notice to the old recipient.
-    if (current_recipient !== undefined &&
-        (operation === 'stop' ||
-         current_recipient !== compose_recipient)) {
-        send_typing_notification_ajax(current_recipient, 'stop');
-        // clear the automatic stop notification timer and recipient.
-        clearTimeout(stop_timer);
-        stop_timer = undefined;
-        current_recipient = undefined;
-    }
-    if (operation === 'start') {
-        if (compose_recipient !== undefined && compose_recipient !== "" && compose_nonempty) {
-            current_recipient = compose_recipient;
-            send_typing_notification_ajax(compose_recipient, operation);
-        }
+    if (compose_recipient === "") {
+        return undefined;
     }
+    return compose_recipient;
 }
 
-// Note: Because we don't make sure we send a final start notification
-// at the last time a user typed something, we require that
-// TYPING_STARTED_SEND_FREQUENCY + TYPING_STOPPED_WAIT_PERIOD <= TYPING_STARTED_EXPIRY_PERIOD
+function is_valid_conversation(recipient) {
+    // TODO: Check to make sure we're in a PM conversation
+    //       with valid emails.
+    if (!recipient) {
+        return false;
+    }
+
+    var compose_empty = !compose_state.has_message_content();
+    if (compose_empty) {
+        return false;
+    }
+
+    return true;
+}
+
+function get_current_time() {
+    return new Date();
+}
+
+function notify_server_start(recipients) {
+    send_typing_notification_ajax(recipients, "start");
+}
+
+function notify_server_stop(recipients) {
+    send_typing_notification_ajax(recipients, "stop");
+}
+
+var worker = {
+    get_recipient: get_recipient,
+    is_valid_conversation: is_valid_conversation,
+    get_current_time: get_current_time,
+    notify_server_start: notify_server_start,
+    notify_server_stop: notify_server_stop,
+};
+
 $(document).on('input', '#new_message_content', function () {
     // If our previous state was no typing notification, send a
     // start-typing notice immediately.
-    var current_time = new Date();
-    if (current_recipient === undefined ||
-        current_time - last_start_time > TYPING_STARTED_SEND_FREQUENCY) {
-        last_start_time = current_time;
-        check_and_send("start");
-    }
-
-    // Then, regardless of whether we changed state, reset the
-    // stop-notification timeout to TYPING_STOPPED_WAIT_PERIOD from
-    // now, so that we'll send a stop notice exactly that long after
-    // stopping typing.
-    if (stop_timer !== undefined) {
-        // Clear an existing stop_timer, if any.
-        clearTimeout(stop_timer);
-    }
-    stop_timer = setTimeout(function () {
-        check_and_send('stop');
-    }, TYPING_STOPPED_WAIT_PERIOD);
+    typing_status.handle_text_input(worker);
 });
 
 // We send a stop-typing notification immediately when compose is
 // closed/cancelled
 $(document).on('compose_canceled.zulip compose_finished.zulip', function () {
-    check_and_send('stop');
+    typing_status.stop(worker);
 });
 
 function get_users_typing_for_narrow() {

static/js/typing_status.js

@@ -0,0 +1,139 @@
+var typing_status = (function () {
+
+var exports = {};
+
+// The following constants are tuned to work with
+// TYPING_STARTED_EXPIRY_PERIOD, which is what the other
+// users will use to time out our messages.  (Or us,
+// depending on your perspective.) See typing.js.
+
+// How frequently 'still typing' notifications are sent
+// to extend the expiry
+var TYPING_STARTED_WAIT_PERIOD = 10000; // 10s
+// How long after someone stops editing in the compose box
+// do we send a 'stopped typing' notification
+var TYPING_STOPPED_WAIT_PERIOD = 5000; // 5s
+
+/*
+
+    Our parent should pass in a worker object with the following
+    callbacks:
+
+        notify_server_start
+        notify_server_stop
+        get_recipient
+        get_current_time
+        is_valid_conversation
+
+    See typing.js for the implementations of the above. (Our
+    node tests also act as workers and will stub those functions
+    appropriately.)
+*/
+
+exports.state = {};
+
+exports.initialize_state = function () {
+    exports.state.current_recipient =  undefined;
+    exports.state.next_send_start_time =  undefined;
+    exports.state.idle_timer = undefined;
+};
+
+exports.initialize_state();
+
+function stop_last_notification(worker) {
+    var state = exports.state;
+    if (state.idle_timer) {
+        clearTimeout(state.idle_timer);
+    }
+    worker.notify_server_stop(state.current_recipient);
+    exports.initialize_state();
+}
+
+function start_or_extend_idle_timer(worker) {
+    var state = exports.state;
+    function on_idle_timeout() {
+        // We don't do any real error checking here, because
+        // if we've been idle, we need to tell folks, and if
+        // our current recipient has changed, previous code will
+        // have stopped the timer.
+        stop_last_notification(worker);
+    }
+
+    if (state.idle_timer) {
+        clearTimeout(state.idle_timer);
+    }
+    state.idle_timer = setTimeout(
+        on_idle_timeout,
+        TYPING_STOPPED_WAIT_PERIOD
+    );
+}
+
+function set_next_start_time(current_time) {
+    exports.state.next_send_start_time = current_time + TYPING_STARTED_WAIT_PERIOD;
+}
+
+function actually_ping_server(worker, recipient, current_time) {
+    worker.notify_server_start(recipient);
+    set_next_start_time(current_time);
+}
+
+function maybe_ping_server(worker, recipient) {
+    var current_time = worker.get_current_time();
+    if (current_time > exports.state.next_send_start_time) {
+        actually_ping_server(worker, recipient, current_time);
+    }
+}
+
+exports.handle_text_input = function (worker) {
+    var new_recipient = worker.get_recipient();
+    var current_recipient = exports.state.current_recipient;
+
+    if (current_recipient) {
+        if (new_recipient === current_recipient) {
+            // Nothing has really changed, except we may need
+            // to send a ping to the server.
+            maybe_ping_server(worker, new_recipient);
+
+            // We can also extend out our idle time.
+            start_or_extend_idle_timer(worker);
+
+            return;
+        }
+
+        // We apparently stopped talking to our old recipient,
+        // so we must stop the old notification.  Don't return
+        // yet, because we may have a new recipient.
+        stop_last_notification(worker);
+
+    }
+
+    if (!worker.is_valid_conversation(new_recipient)) {
+        // If we are not talking to somebody we care about,
+        // then there is no more action to take.
+        return;
+    }
+
+    // We just started talking to this recipient, so notify
+    // the server.
+    exports.state.current_recipient = new_recipient;
+    var current_time = worker.get_current_time();
+    actually_ping_server(worker, new_recipient, current_time);
+    start_or_extend_idle_timer(worker);
+};
+
+exports.stop = function (worker) {
+    // We get this if somebody closes the compose box, but
+    // it doesn't necessarily mean we had typing indicators
+    // active before this.
+    if (exports.state.current_recipient) {
+        stop_last_notification(worker);
+    }
+};
+
+
+return exports;
+}());
+
+if (typeof module !== 'undefined') {
+    module.exports = typing_status;
+}

zproject/settings.py

@@ -902,6 +902,7 @@ JS_SPECS = {
             'js/bot_data.js',
             'js/reactions.js',
             'js/typing.js',
+            'js/typing_status.js',
             'js/typing_data.js',
             'js/ui_init.js',
             'js/shim.js',