From 9e827eb420c7aefd166fba1e7724eced81aaffe7 Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Thu, 30 Apr 2020 13:49:47 +0800 Subject: [PATCH] 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 --- .../composer-presence-display.js.es6 | 86 +++++++--- .../components/topic-presence-display.js.es6 | 23 ++- .../initializers/discourse-presence.js.es6 | 32 ++++ .../discourse/lib/presence-manager.js.es6 | 155 ++++++++++++++---- .../composer-fields/presence.js.es6 | 8 +- .../presence.js.es6 | 6 +- .../initializers/discourse-presence.js.es6 | 43 ----- 7 files changed, 236 insertions(+), 117 deletions(-) create mode 100644 plugins/discourse-presence/assets/javascripts/discourse/initializers/discourse-presence.js.es6 delete mode 100644 plugins/discourse-presence/assets/javascripts/initializers/discourse-presence.js.es6 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 b1329b1ddd3..a371e5f4a91 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 bedb2921447..2f683ca741b 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 00000000000..1009fc1453a --- /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 5943f76ce57..1be35640caa 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, @@ -41,46 +44,57 @@ const PresenceManager = EmberObject.extend({ this.setProperties({ users: [], editingUsers: [], - subscribed: false + subscribers: new Set() }); }, - subscribe() { - if (this.subscribed) return; + subscribe(type) { + if (this.subscribers.size === 0) { + this.messageBus.subscribe( + this.channel, + message => { + const { user, state } = message; + if (this.get("currentUser.id") === user.id) return; - this.messageBus.subscribe( - this.channel, - message => { - const { user, state } = message; - if (this.get("currentUser.id") === user.id) return; + switch (state) { + case REPLYING: + this._appendUser(this.users, user); + break; + case EDITING: + this._appendUser(this.editingUsers, user, { + post_id: parseInt(message.post_id, 10) + }); + break; + case CLOSED: + this._removeUser(user); + break; + } + }, + MESSAGE_BUS_LAST_ID + ); + } - switch (state) { - case REPLYING: - this._appendUser(this.users, user); - break; - case EDITING: - this._appendUser(this.editingUsers, user, { - post_id: parseInt(message.post_id, 10) - }); - break; - case CLOSED: - this._removeUser(user); - break; - } - }, - MESSAGE_BUS_LAST_ID - ); - - this.set("subscribed", true); + this.subscribers.add(type); }, - unsubscribe() { - this.messageBus.unsubscribe(this.channel); - this._stopTimer(); - this.set("subscribed", false); + unsubscribe(type) { + this.subscribers.delete(type); + const noSubscribers = this.subscribers.size === 0; + + if (noSubscribers) { + this.messageBus.unsubscribe(this.channel); + this._stopTimer(); + + this.setProperties({ + users: [], + editingUsers: [] + }); + } + + return noSubscribers; }, - @discourseComputed("topic.id") + @discourseComputed("topicId") channel(topicId) { return `/presence/${topicId}`; }, @@ -101,14 +115,14 @@ const PresenceManager = EmberObject.extend({ const data = { state, - topic_id: this.get("topic.id") + topic_id: this.topicId }; if (whisper) { data.is_whisper = 1; } - if (postId) { + if (postId && state === EDITING) { data.post_id = postId; } @@ -200,4 +214,73 @@ const PresenceManager = EmberObject.extend({ } }); +const PresenceManager = EmberObject.extend({ + presences: null, + currentUser: null, + messageBus: null, + siteSettings: null, + + init() { + this._super(...arguments); + + this.setProperties({ + presences: {} + }); + }, + + subscribe(topicId, type) { + if (!topicId) return; + this._getPresence(topicId).subscribe(type); + }, + + unsubscribe(topicId, type) { + if (!topicId) return; + const presence = this._getPresence(topicId); + + if (presence.unsubscribe(type)) { + delete this.presences[topicId]; + } + }, + + users(topicId) { + if (!topicId) return []; + return this._getPresence(topicId).users; + }, + + editingUsers(topicId) { + if (!topicId) return []; + return this._getPresence(topicId).editingUsers; + }, + + throttlePublish(topicId, state, whisper, postId) { + if (!topicId) return; + return this._getPresence(topicId).throttlePublish(state, whisper, postId); + }, + + publish(topicId, state, whisper, postId) { + if (!topicId) return; + return this._getPresence(topicId).publish(state, whisper, postId); + }, + + cleanUpPresence(type) { + Object.keys(this.presences).forEach(key => { + this.publish(key, CLOSED); + this.unsubscribe(key, type); + }); + }, + + _getPresence(topicId) { + if (!this.presences[topicId]) { + this.presences[topicId] = Presence.create({ + messageBus: this.messageBus, + siteSettings: this.siteSettings, + currentUser: this.currentUser, + topicId + }); + } + + return this.presences[topicId]; + } +}); + export default PresenceManager; diff --git a/plugins/discourse-presence/assets/javascripts/discourse/templates/connectors/composer-fields/presence.js.es6 b/plugins/discourse-presence/assets/javascripts/discourse/templates/connectors/composer-fields/presence.js.es6 index 0240e680276..5165da1a212 100644 --- a/plugins/discourse-presence/assets/javascripts/discourse/templates/connectors/composer-fields/presence.js.es6 +++ b/plugins/discourse-presence/assets/javascripts/discourse/templates/connectors/composer-fields/presence.js.es6 @@ -1,9 +1,5 @@ export default { - shouldRender(args, component) { - return ( - component.siteSettings.presence_enabled && - args.model.topic && - args.model.topic.presenceManager - ); + shouldRender(_, component) { + return component.siteSettings.presence_enabled; } }; diff --git a/plugins/discourse-presence/assets/javascripts/discourse/templates/connectors/topic-above-footer-buttons/presence.js.es6 b/plugins/discourse-presence/assets/javascripts/discourse/templates/connectors/topic-above-footer-buttons/presence.js.es6 index db452d54c7a..5165da1a212 100644 --- a/plugins/discourse-presence/assets/javascripts/discourse/templates/connectors/topic-above-footer-buttons/presence.js.es6 +++ b/plugins/discourse-presence/assets/javascripts/discourse/templates/connectors/topic-above-footer-buttons/presence.js.es6 @@ -1,7 +1,5 @@ export default { - shouldRender(args, component) { - return ( - component.siteSettings.presence_enabled && args.model.presenceManager - ); + shouldRender(_, component) { + return component.siteSettings.presence_enabled; } }; diff --git a/plugins/discourse-presence/assets/javascripts/initializers/discourse-presence.js.es6 b/plugins/discourse-presence/assets/javascripts/initializers/discourse-presence.js.es6 deleted file mode 100644 index 98ec8131987..00000000000 --- a/plugins/discourse-presence/assets/javascripts/initializers/discourse-presence.js.es6 +++ /dev/null @@ -1,43 +0,0 @@ -import { withPluginApi } from "discourse/lib/plugin-api"; -import PresenceManager from "../discourse/lib/presence-manager"; -import ENV from "discourse-common/config/environment"; - -function initializeDiscoursePresence(api) { - const currentUser = api.getCurrentUser(); - const siteSettings = api.container.lookup("site-settings:main"); - - if (currentUser) { - api.modifyClass("model:topic", { - presenceManager: null - }); - - api.modifyClass("route:topic-from-params", { - setupController() { - this._super(...arguments); - - this.modelFor("topic").set( - "presenceManager", - PresenceManager.create({ - topic: this.modelFor("topic"), - currentUser, - messageBus: api.container.lookup("message-bus:main"), - siteSettings - }) - ); - } - }); - } -} - -export default { - name: "discourse-presence", - after: "message-bus", - - initialize(container) { - const siteSettings = container.lookup("site-settings:main"); - - if (siteSettings.presence_enabled && ENV.environment !== "test") { - withPluginApi("0.8.40", initializeDiscoursePresence); - } - } -};