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

cleaning the rtm code, upgrading to the latest version, rtm event han… #78

Merged
merged 8 commits into from
May 24, 2022

Conversation

Meherdeep
Copy link
Contributor

  • Updated RTC v5.1.0
  • Cleaning the session controller
  • RTM client and channel event handlers

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.

LGTM. Thanks @Meherdeep for your hard work getting this PR ready!

Copy link
Contributor

@tadaspetra tadaspetra left a comment

Choose a reason for hiding this comment

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

Looks good.

Suggestion for the future: I like how the functions are separated into the appropriate files, but the controllers folder maybe isn't a good name for it. The two controllers are rtm_controller.dart and session_controller.dart the other ones are helper files. They still work for the controllers so it doesn't really matter but could be nice to add some more folders to keep things organized.

@Meherdeep
Copy link
Contributor Author

@tadaspetra I completely agree with your point. The only reason why it is still named session_controller is that it not only just holds the RTC operations but it has some generic methods as well which includes both RTC and RTM (and some UI methods too). But probably I can try creating 3 controller files (maybe in a different folder) where session_controller is the main controller file and then we can have extended rtc_controller and rtm_controller files for their individual methods.

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.

None yet

3 participants