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

Allow setting disconnect reason. Map SIP reasons. #537

Merged
merged 1 commit into from
Oct 3, 2024
Merged

Conversation

dennwc
Copy link
Contributor

@dennwc dennwc commented Oct 2, 2024

Allow setting disconnect reason. Map new SIP disconnect reasons to SDK constants.

Requires livekit/protocol#828

@dennwc dennwc requested review from boks1971 and a team October 2, 2024 16:30
@dennwc dennwc self-assigned this Oct 2, 2024
"canReconnect", leave.GetCanReconnect(),
)
if e.OnDisconnected != nil {
e.OnDisconnected(LeaveRequested)
e.OnDisconnected(GetDisconnectionReason(reason))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

not about this PR, but we need to update this SDK to support LeaveRequest.Action at some point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add a TODO here 👍

LeaveRequested DisconnectionReason = "leave requested by room"
UserUnavailable DisconnectionReason = "remote user unavailable"
RejectedByUser DisconnectionReason = "rejected by remote user"
Failed DisconnectionReason = "connection to room failed"
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to move protocol DisconnectReason -> string to common place like protocol and use it both here and also in livekit server. Comment not related to this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually this might be the right time. Since I need to update both anyway, I could do it as well.

The only problem is that OnDisconnected callback accepts this string type currently and changing it to something else would not be backward-compatible. Will have to add a new callback, probably.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know if somebody is telling on that string value? Guess it is impossible to say.

We can keep the same string for the two existing there and add the rest.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh string type. Got it. Yeah that's messy

@dennwc dennwc force-pushed the disconnect-reason branch from 86ab5c2 to d259aea Compare October 3, 2024 08:47
@dennwc dennwc force-pushed the disconnect-reason branch from d259aea to b482794 Compare October 3, 2024 08:51
@dennwc dennwc merged commit b42e5a1 into main Oct 3, 2024
2 checks passed
@dennwc dennwc deleted the disconnect-reason branch October 3, 2024 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants