DEV: Refactor presence manager to deal with multiple composer states.

DEV: Refactor presence manager to deal with multiple composer states.

This change amends it so we use a static service to keep track of the typing presence.

It correct various edge cases the initial implementation had

  • Faster close messages
  • When composing on topic 1 and viewing topic 2 we had incorrect presence
  • Changing a running composer to reply as new topic or reply to a differet topic would not correctly shift presence

Authored by tgxworld, with contributions by sam

diff --git a/plugins/discourse-presence/assets/javascripts/discourse/components/composer-presence-display.js.es6 b/plugins/discourse-presence/assets/javascripts/discourse/components/composer-presence-display.js.es6
index b1329b1..a371e5f 100644
--- a/plugins/discourse-presence/assets/javascripts/discourse/components/composer-presence-display.js.es6
+++ b/plugins/discourse-presence/assets/javascripts/discourse/components/composer-presence-display.js.es6
@@ -1,11 +1,17 @@
 import Component from "@ember/component";
+import { getOwner } from "@ember/application";
 import { cancel } from "@ember/runloop";
-import { equal, gt, readOnly } from "@ember/object/computed";
+import { equal, gt } from "@ember/object/computed";
 import discourseComputed, {
   observes,
   on
 } from "discourse-common/utils/decorators";
-import { REPLYING, CLOSED, EDITING } from "../lib/presence-manager";
+import {
+  REPLYING,
+  CLOSED,
+  EDITING,
+  COMPOSER_TYPE
+} from "../lib/presence-manager";
 import { REPLY, EDIT } from "discourse/models/composer";
 
 export default Component.extend({
@@ -16,46 +22,72 @@ export default Component.extend({
   reply: null,
   title: null,
   isWhispering: null,
+  presenceManager: null,
+
+  init() {
+    this._super(...arguments);
+
+    this.setProperties({
+      presenceManager: getOwner(this).lookup("presence-manager:main")
+    });
+  },
+
+  @discourseComputed("topic.id")
+  users(topicId) {
+    return this.presenceManager.users(topicId);
+  },
+
+  @discourseComputed("topic.id")
+  editingUsers(topicId) {
+    return this.presenceManager.editingUsers(topicId);
+  },
 
-  presenceManager: readOnly("topic.presenceManager"),
-  users: readOnly("presenceManager.users"),
-  editingUsers: readOnly("presenceManager.editingUsers"),
   isReply: equal("action", "reply"),
 
   @on("didInsertElement")
   subscribe() {
-    this.presenceManager.subscribe();
+    this.presenceManager.subscribe(this.get("topic.id"), COMPOSER_TYPE);
   },
 
   @discourseComputed(
     "post.id",
     "editingUsers.@each.last_seen",
-    "users.@each.last_seen"
+    "users.@each.last_seen",
+    "action"
   )
-  presenceUsers(postId, editingUsers, users) {
-    if (postId) {
+  presenceUsers(postId, editingUsers, users, action) {
+    if (action === EDIT) {
       return editingUsers.filterBy("post_id", postId);
-    } else {
+    } else if (action === REPLY) {
       return users;
     }
+    return [];
   },
 
   shouldDisplay: gt("presenceUsers.length", 0),
 
   @observes("reply", "title")
   typing() {
-    let action = this.action;
+    const action = this.action;
 
     if (action !== REPLY && action !== EDIT) {
       return;
     }
 
-    const postId = this.get("post.id");
+    let data = {
+      topicId: this.get("topic.id"),
+      state: action === EDIT ? EDITING : REPLYING,
+      whisper: this.whisper,
+      postId: this.get("post.id")
+    };
+
+    this._prevPublishData = data;
 
     this._throttle = this.presenceManager.throttlePublish(
-      action === EDIT ? EDITING : REPLYING,
-      this.whisper,
-      action === EDIT ? postId : undefined
+      data.topicId,
+      data.state,
+      data.whisper,
+      data.postId
     );
   },
 
@@ -64,20 +96,30 @@ export default Component.extend({
     this._cancelThrottle();
   },
 
-  @observes("post.id")
-  stopEditing() {
-    if (!this.get("post.id")) {
-      this.presenceManager.publish(CLOSED, this.whisper);
+  @observes("action", "topic.id")
+  composerState() {
+    if (this._prevPublishData) {
+      this.presenceManager.publish(
+        this._prevPublishData.topicId,
+        CLOSED,
+        this._prevPublishData.whisper,
+        this._prevPublishData.postId
+      );
+      this._prevPublishData = null;
     }
   },
 
   @on("willDestroyElement")
-  composerClosing() {
+  closeComposer() {
     this._cancelThrottle();
-    this.presenceManager.publish(CLOSED, this.whisper);
+    this._prevPublishData = null;
+    this.presenceManager.cleanUpPresence(COMPOSER_TYPE);
   },
 
   _cancelThrottle() {
-    cancel(this._throttle);
+    if (this._throttle) {
+      cancel(this._throttle);
+      this._throttle = null;
+    }
   }
 });
diff --git a/plugins/discourse-presence/assets/javascripts/discourse/components/topic-presence-display.js.es6 b/plugins/discourse-presence/assets/javascripts/discourse/components/topic-presence-display.js.es6
index bedb292..2f683ca 100644
--- a/plugins/discourse-presence/assets/javascripts/discourse/components/topic-presence-display.js.es6
+++ b/plugins/discourse-presence/assets/javascripts/discourse/components/topic-presence-display.js.es6
@@ -1,21 +1,32 @@
 import Component from "@ember/component";
-import { gt, readOnly } from "@ember/object/computed";
-import { on } from "discourse-common/utils/decorators";
+import { getOwner } from "@ember/application";
+import { gt } from "@ember/object/computed";
+import discourseComputed, { on } from "discourse-common/utils/decorators";
+import { TOPIC_TYPE } from "../lib/presence-manager";
 
 export default Component.extend({
   topic: null,
+  presenceManager: null,
+
+  init() {
+    this._super(...arguments);
+    this.set("presenceManager", getOwner(this).lookup("presence-manager:main"));
+  },
+
+  @discourseComputed("topic.id")
+  users(topicId) {
+    return this.presenceManager.users(topicId);
+  },
 
-  presenceManager: readOnly("topic.presenceManager"),
-  users: readOnly("presenceManager.users"),
   shouldDisplay: gt("users.length", 0),
 
   @on("didInsertElement")
   subscribe() {
-    this.presenceManager.subscribe();
+    this.presenceManager.subscribe(this.get("topic.id"), TOPIC_TYPE);
   },
 
   @on("willDestroyElement")
   _destroyed() {
-    this.presenceManager.unsubscribe();
+    this.presenceManager.unsubscribe(this.get("topic.id"), TOPIC_TYPE);
   }
 });
diff --git a/plugins/discourse-presence/assets/javascripts/discourse/initializers/discourse-presence.js.es6 b/plugins/discourse-presence/assets/javascripts/discourse/initializers/discourse-presence.js.es6
new file mode 100644
index 0000000..1009fc1
--- /dev/null
+++ b/plugins/discourse-presence/assets/javascripts/discourse/initializers/discourse-presence.js.es6
@@ -0,0 +1,32 @@
+import { withPluginApi } from "discourse/lib/plugin-api";
+import PresenceManager from "../lib/presence-manager";
+import ENV from "discourse-common/config/environment";
+
+function initializeDiscoursePresence(api, { app }) {
+  const currentUser = api.getCurrentUser();
+
+  if (currentUser) {
+    app.register(
+      "presence-manager:main",
+      PresenceManager.create({
+        currentUser,
+        messageBus: api.container.lookup("message-bus:main"),
+        siteSettings: api.container.lookup("site-settings:main")
+      }),
+      { instantiate: false }
+    );
+  }
+}
+
+export default {
+  name: "discourse-presence",
+  after: "message-bus",
+
+  initialize(container, app) {
+    const siteSettings = container.lookup("site-settings:main");
+
+    if (siteSettings.presence_enabled && ENV.environment !== "test") {
+      withPluginApi("0.8.40", initializeDiscoursePresence, { app });
+    }
+  }
+};
diff --git a/plugins/discourse-presence/assets/javascripts/discourse/lib/presence-manager.js.es6 b/plugins/discourse-presence/assets/javascripts/discourse/lib/presence-manager.js.es6
index 5943f76..1be3564 100644
--- a/plugins/discourse-presence/assets/javascripts/discourse/lib/presence-manager.js.es6
+++ b/plugins/discourse-presence/assets/javascripts/discourse/lib/presence-manager.js.es6
@@ -26,11 +26,14 @@ export const REPLYING = "replying";
 export const EDITING = "editing";
 export const CLOSED = "closed";
 
-const PresenceManager = EmberObject.extend({
+export const TOPIC_TYPE = "topic";
+export const COMPOSER_TYPE = "composer";
+
+const Presence = EmberObject.extend({
   users: null,
   editingUsers: null,
-  subscribed: null,
-  topic: null,
+  subscribers: null,
+  topicId: null,
   currentUser: null,
   messageBus: null,
   siteSettings: null,

[... diff too long, it was truncated ...]

GitHub sha: 9e827eb4

I agree with the suggestion that we should follow this up with a service. It’s the Ember way and actually slightly simpler.

@eviltrout I was mainly following Dependency Injection - Application Concerns - Ember Guides when introducing the singleton and am wondering if there is an advantage of a service over a registered factory?

@tgxworld from that article:

Generally, Services are Ember’s primary method for sharing state via dependency injection. In most cases, you shouldn’t need to learn about how to work with Ember’s DI system directly, or how to manually register and setup dependencies.

The big advantage of the factory api is to automatically inject objects by type, which you are not doing in this case. Even then, over time I have noticed that most objects in our app have a bunch of dependencies automatically attached even if they aren’t used.

The service API is cleaner and far fewer lines of code to do the same thing you’re doing here.