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

[Federation] Send typing events#572

Merged
APwhitehat merged 11 commits intomatrix-org:masterfrom
APwhitehat:fed_send_typing
Aug 10, 2018
Merged

[Federation] Send typing events#572
APwhitehat merged 11 commits intomatrix-org:masterfrom
APwhitehat:fed_send_typing

Conversation

@APwhitehat
Copy link
Copy Markdown
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 changed the title [Federation] Send typing events [WIP][Federation] Send typing events Aug 5, 2018
Copy link
Copy Markdown
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


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

// TODO: Remove this package after, typingserver/api is updated to contain a gomatrixserverlib.Event
Copy link
Copy Markdown
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
// 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants