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

Refactors AgoraClient's initialization #17

Closed

Conversation

Ascenio
Copy link
Contributor

@Ascenio Ascenio commented Jul 2, 2021

Closes #13.

Moves initialization of AgoraClient from its constructor (implicit) to a proper function (explicit).
This means that it's the user responsability to check for it, but it also gives the power to know when it's ready.

Release Notes

  • BREAKING CHANGE: Must call initialize on AgoraClient before further usage.

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • The GitHub Actions pass building and linting. Linter returns no warnings or errors.
  • The QA checklist below has been completed

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

The initialization is triggered by the constructor of AgoraClient at initialization and if it happens to failure, it can't recover from it.

Issue Number: #13

What is the new behavior?

The initialization is triggered by a specific function by the client. If failure happens the client gets to know and can handle it. Also the same instance can be reused.

Does this introduce a breaking change?

  • Yes
  • No

QA Checklist

UIKit Update Checklist (Minor or Patch Release)

  • Using the latest version of Agora's Video SDK
  • Example apps are all functional
  • Core features are still working (both ways across platforms)
    • Camera + Mic muting works for local and remote users
    • Users are added and removed correctly when they join and leave the channel
    • Older versions of the library gracefully handle changes (Create issue if not)
    • Builtin buttons all work as expected
  • Any newly deprecated methods are flagged as such inline and in documentation

@Ascenio Ascenio changed the title Refactos AgoraClient's initialization Refactors AgoraClient's initialization Jul 2, 2021
@tadaspetra tadaspetra changed the base branch from main to dev July 2, 2021 18:55
Copy link
Contributor

@maxxfrazer maxxfrazer left a comment

Choose a reason for hiding this comment

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

Just a couple of edits from me, @Meherdeep and/or @tadaspetra will take a deeper look!

example/lib/main.dart Outdated Show resolved Hide resolved
example/lib/main.dart Outdated Show resolved Hide resolved
@tadaspetra
Copy link
Contributor

Me and @Meherdeep discussed this yesterday, and we think this is the right approach. But we want to go over a couple more things with this including

  • Making sure this breaking change doesn't have adverse effects
  • Adding dispose as well

@Ascenio
Copy link
Contributor Author

Ascenio commented Jul 7, 2021

@tadaspetra that is true, I've noticed there was no dispose and that bugs me. Should I include it in this PR? I think the SessionController should be disposed, but I'm not sure in regards of RtcEngine.

@tadaspetra
Copy link
Contributor

@tadaspetra that is true, I've noticed there was no dispose and that bugs me. Should I include it in this PR? I think the SessionController should be disposed, but I'm not sure in regards of RtcEngine.

Of course, we would love it if you could add it 😄

@Ascenio
Copy link
Contributor Author

Ascenio commented Jul 7, 2021

@tadaspetra I've found out that RtcEngine has a destroy method which unfortunately is async. How would we address that? Im thinking on disposing both RtcEngine and SessionController on AgoraClient's dispose just because we can't make SessionController's dispose async.

@tadaspetra
Copy link
Contributor

@tadaspetra I've found out that RtcEngine has a destroy method which unfortunately is async. How would we address that? Im thinking on disposing both RtcEngine and SessionController on AgoraClient's dispose just because we can't make SessionController's dispose async.

So that might work, but one thing to look out for in the endCall() function we are already calling engine.destroy. I'm thinking this should be done in a dispose as well somewhere in case someone leaves without "ending the call" (maybe using back button or something).

It might be a little more complex solution for this.

@tadaspetra
Copy link
Contributor

tadaspetra commented Sep 22, 2021

@Ascenio

Thank you for the idea, and your code was really useful in the merge of #45.

Thank you for all the contributions and hope to see you around here more :)

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.

[PROPOSAL] Move initialization into it's own function for AgoraClient
3 participants