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

Start with system chat messages #1067

Merged
merged 19 commits into from
Jul 30, 2018
Merged

Conversation

nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Jul 20, 2018

Fix #614

Messages

  • Join/leave a call
  • Last one leaves: call summary similar to activity
  • Add/remove a user
  • (Dis-)Allow guests
  • Password changes/set/unset
  • Promotion/demotion to moderator
  • Room created
  • Room renamed

Special design:

  • message is 0.5 opacity like date/name on others
  • mentions are not bold and not coloured
  • no avatar and user name on those rows

bildschirmfoto von 2018-07-20 17-27-01

@nickvergessen nickvergessen added this to the Next Major milestone Jul 20, 2018
@nickvergessen nickvergessen added 2. developing enhancement feature: chat 💬 Chat and system messages feature: api 🛠️ OCS API for conversations, chats and participants mobile apps labels Jul 20, 2018
@nickvergessen nickvergessen force-pushed the feature/614/system-chat-messages branch from f1384a9 to 3ab43b8 Compare July 23, 2018 11:07
@nickvergessen
Copy link
Member Author

Ready for review @Ivansss

@danxuliu can you help with the layout when the chat is in the sidebar? The timestamp seems to overlap the message there, due to the missing author row parts.

@jancborchardt if you want to comment on the look, do it soon, please, not 2 days before the release ;)

@danxuliu danxuliu force-pushed the feature/614/system-chat-messages branch from b07fe70 to 8c3ee6d Compare July 23, 2018 16:55
@danxuliu
Copy link
Member

@nickvergessen I have fixed the date on system messages. The fix itself is the first commit; the second commit provides a nicer layout (at least, in my opinion), but I added it in a separate commit easily droppable just in case I am the only one that likes it ;-)

Original:
system-messages-date-original

Date on its own row (well, in this screenshot the text of the date when there is an author is not right-aligned, but it is fixed in the pushed commit):
system-messages-date-own-row

Date and system messages merged:
system-messages-date-merged

docs/api-v1.md Outdated
@@ -549,6 +549,7 @@ Base endpoint is: `/ocs/v2.php/apps/spreed/api/v1`

* `joined_call` - {actor} joined the call
* `left_call` - {actor} left the call
* `call_ended` - Call with {user1}, {user2}, {user3}, {user4} and {user5} (Duration 30:23)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't it be Duration {duration} instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, similar to the room name on rename its not a placeholder (not a known rich object string type)

@nickvergessen nickvergessen force-pushed the feature/614/system-chat-messages branch from 8c3ee6d to 7f49c42 Compare July 24, 2018 08:43
@danxuliu danxuliu force-pushed the feature/614/system-chat-messages branch from 7f49c42 to 8c3ee6d Compare July 24, 2018 08:47
@fancycode
Copy link
Member

The system chat messages should trigger an event to the standalone signaling server, so the clients get notified that they should load the new messages (similar to the regular chat messages).

With #1074 the ChatManager now has a dispatcher that could be used for this.

nickvergessen and others added 10 commits July 27, 2018 11:01
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
System messages have no author, so the author row only contains the
date. As the date was shown using an absolute position it had no
influence on the width calculation of other elements and thus sometimes
overlapped them (for example, when a wide system message was shown in
the sidebar, but also if the user name was too long). Now the date uses
a static position with an automatic left margin, which in practice
causes it to be right aligned (both with and without author) due to
being a child of a flex display.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
System messages have no author, so instead of showing the date on its
own row and the message below it now the system message and the date are
merged in a single row using flexboxes.

When there is little available width the message and the date compress
each other and use several lines as needed. The width of the date is
limited to its content, while the message expands itself when possible,
which causes the date to be aligned to the right.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen force-pushed the feature/614/system-chat-messages branch from 4a9e44a to 15ad813 Compare July 27, 2018 09:06
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
Copy link
Member Author

@fancycode after adding the event+listener to update the chat, the signaling backend tests fail because the message is different:

1) OCA\Spreed\Tests\php\Signaling\BackendNotifierTest::testRoomInvite
Failed asserting that Array &0 (
    'type' => 'message'
    'message' => Array &1 (
        'data' => Array &2 (
            'type' => 'chat'
            'chat' => Array &3 (
                'refresh' => true
            )
        )
    )
) is identical to Array &0 (
    'type' => 'invite'
    'invite' => Array &1 (
        'userids' => Array &2 (
            0 => 'testUser'
        )
        'alluserids' => Array &3 (
            0 => 'testUser'
        )
        'properties' => Array &4 (
            'name' => ''
            'type' => 3
        )
    )
).

Can you help with that?

@fancycode
Copy link
Member

Can you help with that?

The tests only remember the last message that was received:

With the system messages there will be more now: first the invite that would be checked by the test, then the refresh triggered by the system chat. You could either filter the chat messages in the test, or keep a list of messages and check if the expected message is in the list.

… the form anyway

Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
public function clearLastRequest() {
$this->lastRequest = null;
}
public function clearLastRequest() {
Copy link
Member

Choose a reason for hiding this comment

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

Please rename to clearRequests to match the other function names.

Signed-off-by: Joas Schilling <coding@schilljs.com>
The specification of acceptance tests for chats do not take into account
the system messages, so now the selector for chat messages filters them
out and leaves only messages sent by users.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@nickvergessen nickvergessen merged commit fa292a6 into master Jul 30, 2018
@nickvergessen nickvergessen deleted the feature/614/system-chat-messages branch July 30, 2018 06:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review enhancement feature: api 🛠️ OCS API for conversations, chats and participants feature: chat 💬 Chat and system messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants