Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NEW] Heavy weight threading (now called Discussions)- with minor model impact #11803

Closed
wants to merge 108 commits into from

Conversation

mrsimpson
Copy link
Collaborator

Addresses #1112 and others

What it does

This PR adds a simple form of threading to Rocket.Chat.

threads-plain-rocketchat mp4

How it's done

Since there has been a lot of discussion about how threads should be implemented in RocketChat, I decided to take a first stab at it which has got the following properties:

  • Minimal changes to the core of RC, to its files as well as the models, publications, subscriptions
  • Configurable in such a way that users not liking it can disable it
  • Compatible with the mobile apps
  • Open to further enhancements

Looking at these non-functional requirements, I came up with the following:

  • Each thread is a room of an existing room type (c or p) => this allows to lifecycle the rooms as usual, e. g. change their privacy, add/remove users and so on
  • The rooms are linked only using the parentRoomId. No other change is applied to the models than adding this property to rooms and subscriptions (for performance)
  • There is no inline-display of the thread in the parent room. Although this was possible with a good performance with this model, this involves significant modifications of the messages list - and it raises the question how to handle notifications.
  • There is no harmonization of the create-room-modal and the thread-creation. This has the downside of bringing in an unwanted button in the sidebar but is less modification prone.

I believe, that with only low effort, we can really provide a configurable way of threading (per server and per user) which will satisfy most divergent demands.

I'm looking forward to your feedback - and contributions, if you want (that is directed to you, @vynmera ;) )

Oliver

mrsimpson and others added 30 commits July 21, 2018 00:21
for renaming-safe linking if necessary
@mrsimpson
Copy link
Collaborator Author

mrsimpson commented Feb 1, 2019

@thiagosanchz appreciate every reaction to how things shall proceed!
However, I feel as if the decisions made are not the critical ones.

1.
All actions that start a thread will open a dialog that the user can edit the name of the thread or create a new one.

We had this on our fork - and user feedback was one click too much. Therefore, we removed the dialog again. However, with defaulting the users to be invited, this can provide meaningful additional value and / or explanation: Once you start a thread from a private group, all members of the parent have to be defaulted, since else they won't have access to the thread (given the current authorization concept).

2.
On the first prototype made by @mrsimpson we can see a button on sidebar to create a new thread. But since thread is related on a channel, we design a way that user can create a new thread but inside this channel (on text area) and not on the sidebar.

We introduced the button in the side panel, since else nobody noticed the new feature. It can be configured to be hidden, since for "experienced" teams, it might look a bit ugly.

With respect to the button inside the channel: It's already implemented, isn't it?
bildschirmfoto 2019-02-01 um 08 42 54

btw: I also added a slash command, but the version deployed on threads-qa is very old

3.
We receive a lot of feedbacks about if the user has inactive threads, the sidebar will be full with inactive items. So, we create a button to hide all threads.

This is not only a problem related to threads: If users created semantical channels without threads in the past, the problem was also there for normal channels.
I even created a PR #12117 for that...

We changed the title of the thread to the top and the channel name to below the thread name.

I like that. Where will the channel description of a thread go? Will is simply be concatenated with the parent channel?

Other than that: All my questions asked about a month ago are still all unanswered. Is there any though about that as well?

Further more: Please let me know what you as maintainers want to implement and what you'd like me to contribute. I'm in general happy to, but I hate working for the bin never merged PRs.

@pshute
Copy link

pshute commented Feb 1, 2019

Reporting a bug found in the demo site at https://threads-qa.rocket.chat.

I started a thread from an existing message, then decided it would be better to reply to it. It appears the Reply option isn't available to messages that are part of a thread, so I got into the thread-channel I'd created, and deleted it.

In #general, the original message is still displayed as being part of a thread, but you can't enter it because it's not there anymore.

@geekgonecrazy
Copy link
Member

https://threads-qa.rocket.chat is now updated to commit: 89a1db88cd20e53e645b5206bca20db6cc8fc892 of this PR. :)

@khaledalyawad
Copy link

I have created a forked branch with working threading

https://github.com/cooderzinc/rocket.chat.reply.threading

image

@mrsimpson
Copy link
Collaborator Author

mrsimpson commented Feb 3, 2019

I have created a forked branch with working threading

@khaledalyawad as far as I've understood from the screenshot, you visualize the thread in the tab bar, including the possibility to reply there. I hope that I'm right guessing that you did not change anything with respect to the backend.
If this is right, it matches my intention: Once you've got the model and mechanisms in place, you can implement different visualization options on top of it.

@khaledalyawad How did you manage reactivity?

P.s.: I don't know what you did to your fork, but I can't get git compare the histories, which would make it possible for myself an others to understand what you changed.

@khaledalyawad
Copy link

khaledalyawad commented Feb 3, 2019

I just added a new property in the message Model called customFields,
the parent message will have customFields.replyIds while the reply messages will have customFields.ref which is the reference id to the parent

you can compare this (because I forked from here) abe1f41

to this

https://github.com/cooderzinc/rocket.chat.reply.threading

@mrsimpson
Copy link
Collaborator Author

you can compare this (because I forked from here) abe1f41

Ah, then this is a completely alternative implementation of threading which actually has got nothing to do with this PR, right? (I'm not disregarding this, just want to know!)
If so, kindly open a new PR so that your code can be deployed, tested and discussed by the community more easily. Thanks!

@khaledalyawad
Copy link

yes, I agree, I was sharing it here just to let you know that I have tackled the same problem with a different approach

@zdumitru
Copy link
Contributor

zdumitru commented Feb 4, 2019

Milestone specified in this issue 0.74.0 is no longer applicable. :-(

@engelgabriel engelgabriel modified the milestones: 0.74.0, 0.75.0 Feb 4, 2019
….git; branch 'develop' of github.com:RocketChat/Rocket.Chat into pr/11803-mrsimpson-core/threading
@@ -7,12 +7,12 @@ import { RocketChat } from 'meteor/rocketchat:lib';
* to race conditions: If multiple updates occur, the current state will be updated
* only if the new state of the thread room is really newer.
*/
Object.assign(RocketChat.models.Messages, {
Object.assign(Messages, {
refreshThreadMetadata(linkMessageId) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ggazzo can we move these functions to inside models package?


import s from 'underscore.string';

Object.assign(RocketChat.models.Rooms, {
Object.assign(Rooms, {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ggazzo can we move these functions to inside models package?

@geekgonecrazy
Copy link
Member

Alright! It has landed! Big huge kudos to @mrsimpson for initial work!

@engelgabriel
Copy link
Member

We have decided that was better to delay our v1 release and get the Threads done properly than get the feature out the way it was initially implemented on the develop branch. We have "rebranded" our current implementation of Threads to Discussions (was it creates a new room type) and will implement Threads exactly the same way as Slack does.

In our vision there are 2 different use cases on how to break out of the main conversation: one very easy and organic, with a single click for small side talk, and other, much more structured, that creates a new conversation with its own life cycle.

We have tried to have a single solution for both, and as a result, no one was happy with it.
So we have given up on having a single approach for both use cases and will implement both: the Slack-like industry standard and our brand new one, called Discussions.

#13843

@engelgabriel engelgabriel changed the title [NEW] Heavy weight threading - with minor model impact [NEW] Heavy weight threading (now called Discussions)- with minor model impact Mar 21, 2019
@engelgabriel engelgabriel added the feat: threads / discussions Threads and Discussions label Mar 21, 2019
@mrsimpson mrsimpson deleted the core/threading branch December 7, 2020 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
August/2018
  
In progress
Development

Successfully merging this pull request may close these issues.

None yet