FEATURE: Redesign discourse-presence to track state on the client side. (PR #9487)

GitHub

if it is a whisper you are editing group_ids is only going to be the intersection of “staff” with topic secure groups.

Also the blew away all support for pms… you are going to need to reconstruct that … topic allowed users / groups for messages.

Yup I’ve not tackled that yet. I’ve only gotten replying and closing of the composer to publish the right messages so far.

no worries at all, the approach here looks a-ok to me.

This acts a guard to ensure that memory doesn’t blow up as we store the state of users in the users array. For editingUsers, we can technically still end up storing a lot of users in the array if everyone edits at the same time which seems kind of rare so I’m not tackling it yet. If we’re worried about the memory growth here, we can’t rely on the same messageBus channel that is used for replying to a topic.

return editingUsers.filterBy("post_id", postId)

Should be the same

I think you can just use: this.presenceManager in all the codeblock

        this.whisper,

same remark about presenceManager

same

      presenceManager.publish(CLOSED, this.whisper);

Generally speaking, I’m not a big fan of alias, in your case I think https://api.emberjs.com/ember/3.17/functions/@ember%2Fobject%2Fcomputed/readOnly is a better choice, given you don’t want to do set on these aliased properties

can you try to use the didReceiveAttrs hook instead of all these observers ? We generally try to avoid observers which are mostly an implementation detail of Ember.

I won’t report all of them, but there’s mostly no reason to do this.get(“xxx”) instead of this.xxx, get is only usefull when you have multiple nesting levels: this.get(“foo.bar.baz”)

this computed should now use @discourseComputed

:thinking: Would this work here since the class isn’t an Ember component? I thought observers were provided so that run callbacks when Ember detects that a specific attribute has changed. I don’t think we get the same with didReceiveAttrs.

The title of this pull request changed from “Move PresenceManager to the client side” to "FEATURE: Redesign discourse-presence to track state on the client side.

I would just use this.presenceManager directly, no need for this variable IMO

o good catch :+1:

js code looks good to me now, but this is quite complex, will have to pull the PR and try locally, will try to do it today.