From a7844de7ee5a2847d949f12e72195c8f634510e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Mon, 18 Dec 2017 22:00:55 +0100 Subject: [PATCH] UX: only publish presence when typing a message --- .../composer-presence-display.js.es6 | 190 +++++------------- .../components/topic-presence-display.js.es6 | 72 +++---- .../connectors/composer-fields/presence.hbs | 13 +- plugins/discourse-presence/plugin.rb | 42 ++-- 4 files changed, 109 insertions(+), 208 deletions(-) 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 5361279452c..334f627bbb0 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,10 +1,8 @@ import { ajax } from 'discourse/lib/ajax'; -import { observes, on } from 'ember-addons/ember-computed-decorators'; -import computed from 'ember-addons/ember-computed-decorators'; -import pageVisible from 'discourse/lib/page-visible'; +import { default as computed, observes, on } from 'ember-addons/ember-computed-decorators'; export const keepAliveDuration = 10000; -const bufferTime = 3000; +export const bufferTime = 3000; export default Ember.Component.extend({ composer: Ember.inject.controller(), @@ -14,174 +12,90 @@ export default Ember.Component.extend({ post: null, topic: null, reply: null, + title: null, // Internal variables - oldPresenceState: null, - presenceState: null, - keepAliveTimer: null, - messageBusChannel: null, + previousState: null, + currentState: null, presenceUsers: null, + channel: null, @on('didInsertElement') - composerOpened(){ - this.updateStateObject(); + composerOpened() { + this._lastPublish = new Date(); + Ember.run.once(this, 'updateState'); }, - @on('willDestroyElement') - composerClosing(){ - this.updateStateObject({closing: true}); + @observes('action', 'post.id', 'topic.id') + composerStateChanged() { + Ember.run.once(this, 'updateState'); }, @observes('reply', 'title') - dataChanged() { - if (!this._dataChanged && (new Date() - this._lastPublish) > keepAliveDuration) { - this._dataChanged = true; - this.keepPresenceAlive(); - } else { - this._dataChanged = true; + typing() { + if (new Date() - this._lastPublish > keepAliveDuration) { + this.publish({ current: this.get('currentState') }); } }, - @observes('action', 'post', 'topic') - composerStateChanged(){ - Ember.run.once(this, 'updateStateObject'); + @on('willDestroyElement') + composerClosing() { + this.publish({ previous: this.get('currentState') }); + Ember.run.cancel(this._pingTimer); + Ember.run.cancel(this._clearTimer); }, - updateStateObject(opts){ - const isClosing = opts && opts.closing; + updateState() { + let state = null; + const action = this.get('action'); - var stateObject = null; - - if(!isClosing && this.shouldSharePresence(this.get('action'))){ - stateObject = {}; - - stateObject.action = this.get('action'); - - // Add some context if we're editing or replying - switch(stateObject.action){ - case 'edit': - stateObject.post_id = this.get('post.id'); - break; - case 'reply': - stateObject.topic_id = this.get('topic.id'); - break; - default: - break; // createTopic or privateMessage - } + if (action === 'reply' || action === 'edit') { + state = { action }; + if (action === 'reply') state.topic_id = this.get('topic.id'); + if (action === 'edit') state.post_id = this.get('post.id'); } - this.set('oldPresenceState', this.get('presenceState')); - this.set('presenceState', stateObject); - - if (isClosing) { - Ember.run.cancel(this._timeoutTimer); - } + this.set('previousState', this.get('currentState')); + this.set('currentState', state); }, - _ACTIONS: ['edit', 'reply'], - - shouldSharePresence(action){ - return this._ACTIONS.includes(action); - }, - - @observes('presenceState') - presenceStateChanged(){ - if(this.get('messageBusChannel')){ - this.messageBus.unsubscribe(this.get('messageBusChannel')); - this.set('messageBusChannel', null); + @observes('currentState') + currentStateChanged() { + if (this.get('channel')) { + this.messageBus.unsubscribe(this.get('channel')); + this.set('channel', null); } - this.set('presenceUsers', []); + this.clear(); + this.publish({ response_needed: true, - previous: this.get('oldPresenceState'), - current: this.get('presenceState') - }).then((data) => { - const messageBusChannel = data['messagebus_channel']; - if(messageBusChannel){ - const users = data['users']; - const messageBusId = data['messagebus_id']; - this.set('presenceUsers', users); - this.set('messageBusChannel', messageBusChannel); - this.messageBus.subscribe(messageBusChannel, message => { - this.set('presenceUsers', message['users']); - this.timeoutPresence(); - }, messageBusId); - } - }).catch((error) => { - // This isn't a critical failure, so don't disturb the user - if (window.console && console.error) { - console.error("Error publishing composer status", error); - } + previous: this.get('previousState'), + current: this.get('currentState') + }).then(r => { + this.set('presenceUsers', r.users); + this.set('channel', r.messagebus_channel); + this.messageBus.subscribe(r.messagebus_channel, message => { + if (!this.get('isDestroyed')) this.set('presenceUsers', message.users); + this._clearTimer = Ember.run.debounce(this, 'clear', keepAliveDuration + bufferTime); + }, r.messagebus_id); }); - - Ember.run.cancel(this.get('keepAliveTimer')); - if(this.shouldSharePresence(this.get('presenceState.action'))){ - // Send presence data every 10 seconds - this.set('keepAliveTimer', Ember.run.later(this, 'keepPresenceAlive', keepAliveDuration)); - } }, - timeoutPresence() { - Ember.run.cancel(this._timeoutTimer); - this._timeoutTimer = Ember.run.later( - this, - () => { this.set("presenceUsers", []); }, - keepAliveDuration + bufferTime - ); + clear() { + if (!this.get('isDestroyed')) this.set('presenceUsers', []); }, publish(data) { this._lastPublish = new Date(); - this._dataChanged = false; - - return ajax('/presence/publish', { - type: 'POST', - data: data - }); - }, - - keepPresenceAlive(){ - // If we're not replying or editing, - // don't update anything, and don't schedule this task again - if(!this.shouldSharePresence(this.get('presenceState.action'))){ - return; - } - - if (this._dataChanged) { - this._dataChanged = false; - const browserInFocus = pageVisible(); - - // Only send the keepalive message if the browser has focus - if(browserInFocus){ - this.publish({ - current: this.get('presenceState') - }).catch((error) => { - // This isn't a critical failure, so don't disturb the user - if (window.console && console.error) { - console.error("Error publishing composer status", error); - } - }); - } - } - - // Schedule again in another 10 seconds - Ember.run.cancel(this.get('keepAliveTimer')); - this.set('keepAliveTimer', Ember.run.later(this, 'keepPresenceAlive', keepAliveDuration)); + return ajax('/presence/publish', { type: 'POST', data }); }, @computed('presenceUsers', 'currentUser.id') - users(presenceUsers, currentUser_id){ - return (presenceUsers || []).filter(user => user.id !== currentUser_id); + users(users, currentUserId) { + return (users || []).filter(user => user.id !== currentUserId); }, - @computed('presenceState.action') - isReply(action){ - return action === 'reply'; - }, - - @computed('users.length') - shouldDisplay(length){ - return length > 0; - } + isReply: Ember.computed.equal('action', 'reply'), + shouldDisplay: Ember.computed.gt('users.length', 0) }); 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 2d9e1c84e0b..9e7e9bb443e 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,68 +1,42 @@ -import { on } from 'ember-addons/ember-computed-decorators'; -import computed from 'ember-addons/ember-computed-decorators'; -import { keepAliveDuration } from 'discourse/plugins/discourse-presence/discourse/components/composer-presence-display'; +import { default as computed, on } from 'ember-addons/ember-computed-decorators'; +import { keepAliveDuration, bufferTime } from 'discourse/plugins/discourse-presence/discourse/components/composer-presence-display'; -const bufferTime = 3000; +const MB_GET_LAST_MESSAGE = -2; export default Ember.Component.extend({ topicId: null, - - messageBusChannel: null, presenceUsers: null, + clear() { + if (!this.get('isDestroyed')) this.set('presenceUsers', []); + }, + @on('didInsertElement') _inserted() { - this.set("presenceUsers", []); - const messageBusChannel = `/presence/topic/${this.get('topicId')}`; - this.set('messageBusChannel', messageBusChannel); + this.clear(); - var firstMessage = true; - - this.messageBus.subscribe(messageBusChannel, message => { - - let users = message.users; - - // account for old messages, - // we only do this once to allow for some bad clocks - if (firstMessage) { - const old = ((new Date()) / 1000) - ((keepAliveDuration / 1000) * 2); - if (message.time && (message.time < old)) { - users = []; - } - firstMessage = false; - } - - Em.run.cancel(this._expireTimer); - - this.set("presenceUsers", users); - - this._expireTimer = Em.run.later( - this, - () => { - this.set("presenceUsers", []); - }, - keepAliveDuration + bufferTime - ); - }, -2); /* subscribe at position -2 so we get last message */ + this.messageBus.subscribe(this.get('channel'), message => { + if (!this.get('isDestroyed')) this.set('presenceUsers', message.users); + this._clearTimer = Ember.run.debounce(this, 'clear', keepAliveDuration + bufferTime); + }, MB_GET_LAST_MESSAGE); }, @on('willDestroyElement') _destroyed() { - const channel = this.get("messageBusChannel"); - if (channel) { - Em.run.cancel(this._expireTimer); - this.messageBus.unsubscribe(channel); - this.set("messageBusChannel", null); - } + Ember.run.cancel(this._clearTimer); + this.messageBus.unsubscribe(this.get('channel')); + }, + + @computed('topicId') + channel(topicId) { + return `/presence/topic/${topicId}`; }, @computed('presenceUsers', 'currentUser.id') - users(presenceUsers, currentUser_id){ - return (presenceUsers || []).filter(user => user.id !== currentUser_id); + users(users, currentUserId) { + return (users || []).filter(user => user.id !== currentUserId); }, - @computed('users.length') - shouldDisplay(length){ - return length > 0; - } + shouldDisplay: Ember.computed.gt('users.length', 0) + }); diff --git a/plugins/discourse-presence/assets/javascripts/discourse/templates/connectors/composer-fields/presence.hbs b/plugins/discourse-presence/assets/javascripts/discourse/templates/connectors/composer-fields/presence.hbs index 87021d261fc..af4de7c592b 100644 --- a/plugins/discourse-presence/assets/javascripts/discourse/templates/connectors/composer-fields/presence.hbs +++ b/plugins/discourse-presence/assets/javascripts/discourse/templates/connectors/composer-fields/presence.hbs @@ -1,6 +1,7 @@ -{{composer-presence-display - action=model.action - post=model.post - topic=model.topic - reply=model.reply - }} +{{composer-presence-display + action=model.action + post=model.post + topic=model.topic + reply=model.reply + title=model.title +}} diff --git a/plugins/discourse-presence/plugin.rb b/plugins/discourse-presence/plugin.rb index a7729600fc7..d553a50478a 100644 --- a/plugins/discourse-presence/plugin.rb +++ b/plugins/discourse-presence/plugin.rb @@ -20,6 +20,8 @@ after_initialize do end module ::Presence::PresenceManager + MAX_BACKLOG_AGE ||= 60 + def self.get_redis_key(type, id) "presence:#{type}:#{id}" end @@ -28,24 +30,23 @@ after_initialize do "/presence/#{type}/#{id}" end + # return true if a key was added def self.add(type, id, user_id) - # return true if a key was added key = get_redis_key(type, id) result = $redis.hset(key, user_id, Time.zone.now) - $redis.expire(key, 60) + $redis.expire(key, MAX_BACKLOG_AGE) result end + # return true if a key was deleted def self.remove(type, id, user_id) key = get_redis_key(type, id) - $redis.expire(key, 60) - # return true if a key was deleted + $redis.expire(key, MAX_BACKLOG_AGE) $redis.hdel(key, user_id) > 0 end def self.get_users(type, id) user_ids = $redis.hkeys(get_redis_key(type, id)).map(&:to_i) - # TODO: limit the # of users returned User.where(id: user_ids) end @@ -59,9 +60,19 @@ after_initialize do if topic.archetype == Archetype.private_message user_ids = User.where('admin OR moderator').pluck(:id) + topic.allowed_users.pluck(:id) - MessageBus.publish(messagebus_channel, message.as_json, user_ids: user_ids, max_backlog_age: 60) + MessageBus.publish( + messagebus_channel, + message.as_json, + user_ids: user_ids, + max_backlog_age: MAX_BACKLOG_AGE + ) else - MessageBus.publish(messagebus_channel, message.as_json, group_ids: topic.secure_group_ids, max_backlog_age: 60) + MessageBus.publish( + messagebus_channel, + message.as_json, + group_ids: topic.secure_group_ids, + max_backlog_age: MAX_BACKLOG_AGE + ) end users @@ -89,7 +100,8 @@ after_initialize do requires_plugin PLUGIN_NAME before_action :ensure_logged_in - ACTIONS = %w{edit reply}.each(&:freeze) + ACTIONS ||= %w{edit reply}.each(&:freeze) + MAX_USERS ||= 20 def publish data = params.permit( @@ -109,9 +121,9 @@ after_initialize do if topic guardian.ensure_can_see!(topic) - _removed = Presence::PresenceManager.remove(type, id, current_user.id) - cleaned = Presence::PresenceManager.cleanup(type, id) - users = Presence::PresenceManager.publish(type, id) + Presence::PresenceManager.remove(type, id, current_user.id) + Presence::PresenceManager.cleanup(type, id) + Presence::PresenceManager.publish(type, id) end end @@ -124,9 +136,9 @@ after_initialize do if topic guardian.ensure_can_see!(topic) - _added = Presence::PresenceManager.add(type, id, current_user.id) - cleaned = Presence::PresenceManager.cleanup(type, id) - users = Presence::PresenceManager.publish(type, id) + Presence::PresenceManager.add(type, id, current_user.id) + Presence::PresenceManager.cleanup(type, id) + users = Presence::PresenceManager.publish(type, id) if data[:response_needed] messagebus_channel = Presence::PresenceManager.get_messagebus_channel(type, id) @@ -143,7 +155,7 @@ after_initialize do { messagebus_channel: channel, messagebus_id: MessageBus.last_id(channel), - users: users.map { |u| BasicUserSerializer.new(u, root: false) } + users: users.limit(MAX_USERS).map { |u| BasicUserSerializer.new(u, root: false) } } end