Skip to content

Expose complete reconnect mode cycle #694

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bombruch
Copy link

Issue
@pblazej did an amazing job in #642 with exposing reconnect mode.
But it happens to give info when SDK goes into .quick or .full reconnect but not back to normal connect state.
So with current logic there is no way to get info when SDK establishes connection back and it is time to hide reconnect UI elements.

Solution
Expose full cycle for ReconnectMode. Since SDK uses Swift optional enum ReconnectMode? there is no way to notify ObjC delegate with changes. Idea is to present .none ReconnectMode option and use it instead of optional. With such approach it is feasible to notify ObjC delegates. With such logic SDK can tell clients that reconnection process is over. Still not sure wether .none is the best name for such option so may be reviewers could suggest any better.

@CLAassistant
Copy link

CLAassistant commented May 12, 2025

CLA assistant check
All committers have signed the CLA.

@pblazej
Copy link
Contributor

pblazej commented May 12, 2025

Just fyi my initial idea was to use it in conjunction with the global state so that we avoid changes in the reconnect state machine.

I'm open to re-discussing this one cc @hiroshihorie

@bombruch bombruch force-pushed the expose_complete_reconnect_mode_cycle branch from b459344 to a704de1 Compare May 12, 2025 12:07
@pblazej
Copy link
Contributor

pblazej commented May 19, 2025

@bombruch can you confirm that the above solution may work for you first?

@bombruch
Copy link
Author

@pblazej of corse it does. All below is based on assumption that ConnectOptions is created with:

 ConnectOptions(
    reconnectAttempts: 30,
    reconnectAttemptDelay: 1
)

Consider 2 cases:

First case) WiFi is turned off for a long period (~20 seconds). This case is covered with existing logic in LK SDK. As shortened logs look like:

18:22:54.629 didUpdateReconnectMode, reconnectMode: .quick

Time to show reconnect UI.
Wait for ~20 seconds
Turn on WiFi (it takes time to recover WiFi)

18:23:36.961 didUpdateReconnectMode, reconnectMode: .full
18:23:36.963 didUpdateConnectionState: .reconnecting, oldValue: .connected, error: empty error
18:23:36.966 roomIsReconnecting
18:23:38.452 didUpdateConnectionState: .connected, oldValue: .reconnecting, error: empty error
18:23:38.462 roomDidReconnect

Based on last 2 lines in logs above - there is info to hide reconnect UI

Second case) WiFi is turned off for a short period (~5 seconds).

11:29:59.375 didUpdateReconnectMode, reconnectMode: .quick

After WiFi connection is restored it can tell about didUpdateConnectionQuality (it happens mostly if camera is active) but that is basically it. I mean no logs as no delegate methods are fired.

With fix for second case I can clearly see logs like:

11:47:28.638 didUpdateReconnectMode, reconnectMode: .quick
11:47:35.871 didUpdateReconnectMode, reconnectMode: .none

During preparation of this comment I noticed that I've broken your logic with reconnect vs connect state. For sure I can't use Xcode search tool. So I've updated PR with second commit and got a bit embarrassed - seems like too many changes to get full cycle in delegate. May be something like separate Enum for ObjC can be presented and used for delegate. While in SDK still original is used. In other words this PR could be like in screen shot below:

Screenshot 2025-05-20 at 12 04 32

Pls tell me your thoughts on all above. Thanks!

@bombruch bombruch force-pushed the expose_complete_reconnect_mode_cycle branch from a704de1 to 97c6e0a Compare May 20, 2025 09:11
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants