feat(MessageGroup) - group system messages by Antreesy · Pull Request #9777 · nextcloud/spreed

Antreesy · 2023-06-14T15:38:03Z

☑️ Resolves

Fix Frontend - Group system messages from calls #9678
New component and util for handling groups of system messages added
Combine messages for following actions:
- participants being added to the conversations
- participants promoted to / demoted from moderators
- participants joined / left the call
- participant reconnected to the call
Light sort of messages within one group:
user added/removed => moderator promoted/demoted => call started => recording started => call joined/left => call ended

🖼️ Screenshots

🏚️ Before	🏡 After

🚧 Tasks

Design: appearance of header and collapsed messages, toggle button, animation
RFC: re-sorting system messages within one MessagesSystemGroup: define a list of system messages being sorted, and theirs order (sorting of related messages brings weird results , like call_left and call_joined in various combinations, so they're not sorted between each other)
Follow-up - test coverage
Follow-up - messages re-rendering

🏁 Checklist

⛑️ Tests (unit and/or integration) are included or not required
📘 API documentation in docs/ has been updated or is not required
📗 User documentation in https://github.com/nextcloud/documentation/tree/master/user_manual/talk has been updated or is not required
🔖 Capability is added or not needed

src/components/MessagesList/MessagesGroup/MessagesSystemGroup.vue

marcoambrosini

Very nice, could we please always align the button with the date? Meaning to the first line of the system message

src/components/MessagesList/MessagesGroup/MessagesSystemGroup.vue

Antreesy · 2023-07-04T13:06:19Z

follow-up: messages are re-rendered on each fetch, so hidden messages collapse each time

jancborchardt

Very nice @Antreesy! One detail – as per design guidelines we never use italic font as it can be difficult to read for people. Since they are in a container already they do not need to be italic as well. :)

Antreesy · 2023-07-07T16:16:27Z

Rebased on top of refactor+bugfix PR #9897

ShGKme · 2023-07-22T10:49:37Z

Is it still work in progress, or ready for code review?

Antreesy · 2023-07-22T19:59:37Z

Is it still work in progress, or ready for code review?

There still is a room for improvement, but mostly about error cleanup and refactoring.
Main Feature is working, so the code could be checked already

marcoambrosini · 2023-08-02T07:44:09Z

Maybe underlying "and 7 more participants" and making it into an <a> that expands the content on click?

nimishavijay · 2023-08-02T08:14:17Z

Looks really really nice! :)

we can hide this action until the group is hovered

If there are no a11y issues with this, then this sounds great! This would be the ideal solution. Maybe @Pytal or @JuliaKirschenheuter could help with figuring out if showing the button only on hover is okay? :)

Maybe underlying "and 7 more participants" and making it into an that expands the content on click?

I'm not too sure about this as it's not really a link at all, and we rarely (if at all) use link styling to indicate other actions.

Pytal · 2023-08-02T18:29:07Z

Looks really really nice! :)

we can hide this action until the group is hovered

If there are no a11y issues with this, then this sounds great! This would be the ideal solution. Maybe @Pytal or @JuliaKirschenheuter could help with figuring out if showing the button only on hover is okay? :)

Hiding the action to reduce visual clutter sounds good but make sure that it is tabbable and visible when focused/hovered

Antreesy · 2023-08-03T10:34:30Z

@nimishavijay @marcoambrosini stylized toggle button asMessageButtonBar, which we have for normal message (with the same logic). Button is visible on hover, and in open state:

P.S. Updated screenshots in description

nimishavijay

Looks really nice! :)

ShGKme

Seems to works fine. Nothing blocking, but some naming/architecture comments

src/components/MessagesList/MessagesList.vue

src/utils/createCombinedSystemMessage.js

nickvergessen · 2023-08-07T09:54:53Z

Conflicting files
src/components/MessagesList/MessagesGroup/Message/Message.vue

…equently Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>

Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>

Antreesy · 2023-08-07T15:56:49Z

Rebased onto master
@ShGKme please take a look again - i've turned the utiil into composable

ShGKme

Tested manually in basic cases ✅

These components and grouping logic has many edge cases. IMO, we should cover it with tests.

ShGKme · 2023-08-07T21:03:41Z

src/components/MessagesList/MessagesGroup/MessagesSystemGroup.vue

+			<div v-for="messagesCollapsed of messagesGroupedBySystemMessage"
+				:key="messagesCollapsed.id"
+				class="message-group__system">
+				<ul v-if="messagesCollapsed.messages?.length > 1"
+					class="messages messages--header">
+					<Message v-bind="createCombinedSystemMessage(messagesCollapsed)"
+						is-combined-system-message
+						:is-combined-system-message-collapsed="messagesCollapsed.collapsed"
+						:next-message-id="getNextMessageId(messagesCollapsed.messages.at(-1))"
+						:previous-message-id="getPrevMessageId(messagesCollapsed.messages.at(0))"
+						@toggle-combined-system-message="toggleCollapsed(messagesCollapsed)" />
+				</ul>
+				<ul v-show="messagesCollapsed.messages?.length === 1 || !messagesCollapsed.collapsed"
+					class="messages"
+					:class="{'messages--collapsed': messagesCollapsed.messages?.length > 1}">
+					<Message v-for="message of messagesCollapsed.messages"
+						:key="message.id"
+						v-bind="message"
+						:next-message-id="getNextMessageId(message)"
+						:previous-message-id="getPrevMessageId(message)" />
+				</ul>
+			</div>


This component has many methods calls in template (that a called during the re-rendering) and those methods are not small utils or formatting methods but data processing methods.

It won't be a problem if it doesn't have unnecessary re-renderings, but currently MessagesList re-renders each new message.

For example, a user has a chat with mostly only calls, but large calls, for example, 100 people. Then in the history there will be many groups with 100 system messages about leaving the call. And every message, reaction, poll update will re-render the list with recalculating all these methods looping over 100 messages for each group.

IMO, we can keep it in this way but then do one of:

Find a reason of re-renderings

Refactor all these components, remove unnecessary props

Remove these calculating inside rendering

Wait for Vue 3 migration =D

Also, I'd prefer to use in in v-for directives to use only one style. (v-for of is not very often used in our components).

"of" is replaced with "in". Test coverage and components re-rendering moved to follow-up

Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>

…first message line Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>

Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>

…r.vue Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>

Blocking question has been resolved

Antreesy · 2023-08-08T07:26:24Z

/backport to stable27

Antreesy added 2. developing enhancement medium feature: chat 💬 Chat and system messages feature: frontend 🖌️ "Web UI" client labels Jun 14, 2023

Antreesy added this to the 💜 Next Major (28) milestone Jun 14, 2023

Antreesy self-assigned this Jun 14, 2023

nickvergessen reviewed Jun 20, 2023

View reviewed changes

src/components/MessagesList/MessagesGroup/MessagesSystemGroup.vue Outdated Show resolved Hide resolved

Antreesy force-pushed the feat/9678/group-system-messages branch from 279a7c7 to 3d7f282 Compare June 21, 2023 12:59

Antreesy force-pushed the feat/9678/group-system-messages branch 2 times, most recently from 1d53c0f to 0f7d991 Compare July 3, 2023 15:56

Antreesy changed the title ~~Group system messages [web draft]~~ feat(MessageGroup) - group system messages Jul 3, 2023

Antreesy requested review from DorraJaouad, ShGKme and marcoambrosini July 3, 2023 16:00

Antreesy marked this pull request as ready for review July 3, 2023 16:00

marcoambrosini previously requested changes Jul 4, 2023

View reviewed changes

Antreesy commented Jul 4, 2023

View reviewed changes

src/components/MessagesList/MessagesGroup/MessagesSystemGroup.vue Outdated Show resolved Hide resolved

jancborchardt previously requested changes Jul 6, 2023

View reviewed changes

Antreesy force-pushed the feat/9678/group-system-messages branch from 571eb99 to 53fc88d Compare July 7, 2023 16:12

Antreesy marked this pull request as draft July 7, 2023 16:13

Antreesy force-pushed the feat/9678/group-system-messages branch from 53fc88d to 209a8ec Compare July 21, 2023 11:13

Antreesy marked this pull request as ready for review July 22, 2023 19:58

Antreesy force-pushed the feat/9678/group-system-messages branch from 209a8ec to 098c6da Compare July 24, 2023 09:16

Antreesy requested review from jancborchardt and marcoambrosini July 29, 2023 09:14

nimishavijay approved these changes Aug 3, 2023

View reviewed changes

ShGKme approved these changes Aug 4, 2023

View reviewed changes

src/components/MessagesList/MessagesList.vue Outdated Show resolved Hide resolved

src/utils/createCombinedSystemMessage.js Outdated Show resolved Hide resolved

Antreesy added 2 commits August 7, 2023 17:55

create new component for groups of system messages, compare them cons…

574748f

…equently Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>

clear system setups for existing components

cd48735

Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>

Antreesy force-pushed the feat/9678/group-system-messages branch from 81c6d9b to 1dfc54b Compare August 7, 2023 15:56

ShGKme approved these changes Aug 7, 2023

View reviewed changes

Antreesy added 7 commits August 8, 2023 08:57

handle 'user_added'

2474f54

Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>

handle 'call_joined', 'call_left', 'call_reconnected'

f07c49e

Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>

handle 'moderator_promoted', 'moderator_demoted'

301170b

Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>

sort messages within one group

02d771e

Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>

persist toggle state during list re-render, align toggle button with …

dcb7496

…first message line Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>

move createCombinedSystemMessage method to composable

2a5102b

Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>

move toggle button to Message.vue, align styles with MessageButtonsBa…

9b1c9d0

…r.vue Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>

Antreesy force-pushed the feat/9678/group-system-messages branch from 1dfc54b to 9b1c9d0 Compare August 8, 2023 07:24

Antreesy enabled auto-merge August 8, 2023 07:24

backportbot-nextcloud bot added the backport-request label Aug 8, 2023

Antreesy merged commit 5382b03 into master Aug 8, 2023

Antreesy deleted the feat/9678/group-system-messages branch August 8, 2023 07:45

backportbot-nextcloud bot mentioned this pull request Aug 8, 2023

[stable27] feat(MessageGroup) - group system messages #10143

Merged

backportbot-nextcloud bot removed the backport-request label Aug 8, 2023

Antreesy mentioned this pull request Oct 23, 2023

follow-up(SystemMessageGroup) - improve behaviour #10758

Closed

3 tasks

Conversation

Antreesy commented Jun 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

☑️ Resolves

🖼️ Screenshots

🚧 Tasks

🏁 Checklist

Uh oh!

Uh oh!

marcoambrosini left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Antreesy commented Jul 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jancborchardt left a comment

Choose a reason for hiding this comment

Uh oh!

Antreesy commented Jul 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ShGKme commented Jul 22, 2023

Uh oh!

Antreesy commented Jul 22, 2023

Uh oh!

marcoambrosini commented Aug 2, 2023

Uh oh!

nimishavijay commented Aug 2, 2023

Uh oh!

Pytal commented Aug 2, 2023

Uh oh!

Antreesy commented Aug 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nimishavijay left a comment

Choose a reason for hiding this comment

Uh oh!

ShGKme left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nickvergessen commented Aug 7, 2023

Uh oh!

Antreesy commented Aug 7, 2023

Uh oh!

ShGKme left a comment

Choose a reason for hiding this comment

Uh oh!

ShGKme Aug 7, 2023

Choose a reason for hiding this comment

Uh oh!

ShGKme Aug 7, 2023

Choose a reason for hiding this comment

Uh oh!

Antreesy Aug 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Antreesy commented Aug 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Antreesy commented Jun 14, 2023 •

edited

Loading

Antreesy commented Jul 4, 2023 •

edited

Loading

Antreesy commented Jul 7, 2023 •

edited

Loading

Antreesy commented Aug 3, 2023 •

edited

Loading

Antreesy Aug 8, 2023 •

edited

Loading