Skip to content
This repository has been archived by the owner on Nov 25, 2024. It is now read-only.

[Federation] Send typing events #572

Merged
merged 11 commits into from
Aug 10, 2018
Merged

[Federation] Send typing events #572

merged 11 commits into from
Aug 10, 2018

Conversation

APwhitehat
Copy link
Contributor

  • Consume typing events from typing server.
  • Send the events to all joined hosts in a room

P.S. dummy api is used to emulate the type of api.OutputTypingEvent.Event as gomatrixserverlib.Event, will be removed when #569 is fixed.
Signed-off-by: Anant Prakash <anantprakashjsr@gmail.com>

@APwhitehat APwhitehat added this to the Typing Server milestone Aug 4, 2018
@APwhitehat APwhitehat changed the title [Federation] Send typing events [WIP][Federation] Send typing events Aug 5, 2018
erikjohnston
erikjohnston suggested changes Aug 6, 2018
View reviewed changes
Copy link
Member

@erikjohnston erikjohnston left a comment

Choose a reason for hiding this comment

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

Otherwise, LGTM

src/github.com/matrix-org/dendrite/typingserver/dummy/api/api.go Outdated

import "github.com/matrix-org/gomatrixserverlib"

// TODO: Remove this package after, typingserver/api is updated to contain a gomatrixserverlib.Event
Copy link
Member

Choose a reason for hiding this comment

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

We should probably move this TODO out of dummy package now that the plan is no longer to change to using Event

@APwhitehat APwhitehat changed the title [WIP][Federation] Send typing events [Federation] Send typing events Aug 7, 2018
erikjohnston
erikjohnston suggested changes Aug 8, 2018
View reviewed changes
src/github.com/matrix-org/dendrite/typingserver/api/output.go Outdated
@@ -16,16 +16,14 @@ package api
type OutputTypingEvent struct {
// The Event for the typing edu event.
Event TypingEvent `json:"event"`
// Users typing in the room at the event.
TypingUsers []string `json:"typing_users"`
Copy link
Member

Choose a reason for hiding this comment

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

Can you expand the OutputTypingEvent doc comment to explain why we're sending both a TypingEvent and list of currently typing users?

@APwhitehat
Copy link
Contributor Author

Nuances with the current implementation (forgot to mention earlier):

  • federation EDU doesn't have the destination field. link to travis's PR.
    It feels rather unused, since the (federation) transaction would have the destination.
  • processing edus does not increment the destinationQueue.sendCounter. I am not sure if this is correct.

@erikjohnston: Hope you see this before accepting this PR.

@APwhitehat APwhitehat removed their assignment Aug 8, 2018
@erikjohnston
Copy link
Member

federation EDU doesn't have the destination field. link to travis's PR.
It feels rather unused, since the (federation) transaction would have the destination.

I've just got travis to remove the destination and origin fields from his PR ftr

@APwhitehat
Copy link
Contributor Author

I've just got travis to remove the destination and origin fields from his PR ftr

removing the origin field as well.

@APwhitehat
Copy link
Contributor Author

processing edus does not increment the destinationQueue.sendCounter. I am not sure if this is correct.

Took a look at the origin of this, I believe this is used for logging only.
Since EDUs are short lived and can be a lot, we could ignore them in sendCounter.
Pretty easy to add if required.
/cc @erikjohnston

@APwhitehat APwhitehat merged commit 5d52863 into matrix-org:master Aug 10, 2018
@APwhitehat APwhitehat deleted the fed_send_typing branch August 10, 2018 15:27
@APwhitehat APwhitehat restored the fed_send_typing branch August 10, 2018 15:43
@matrixbot matrixbot mentioned this pull request Oct 30, 2024
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@erikjohnston erikjohnston erikjohnston approved these changes

Assignees

@anoadragon453 anoadragon453

@erikjohnston erikjohnston

Labels
C-EDU-Server C-Federation
Projects
None yet
Milestone
Typing Server
Development

Successfully merging this pull request may close these issues.
3 participants
@APwhitehat @erikjohnston @anoadragon453