-
Notifications
You must be signed in to change notification settings - Fork 271
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
Refactor static access #4729
Refactor static access #4729
Conversation
5ffa2f5
to
7c7c962
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, I get the crash reported in the issue when launching the Compose sample app on an API 28 emulator.
Please keep in mind that it's not perfectly repeatable. It happens often after a clean install, but not always.
917bdd1
to
21f5407
Compare
21f5407
to
b6371c4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
☑️Reviewer Checklist
- UI Components sample runs & works
- Compose sample runs & works
- UI Changes correct (before & after images)
- Bugs validated (bugfixes)
- New feature tested and works
- Release notes and docs clearly describe changes
- All code we touched has new or updated KDocs
...ate/src/main/java/io/getstream/chat/android/state/plugin/factory/StreamStatePluginFactory.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please address the comment and this is good to me.
...am-chat-android-state/src/main/java/io/getstream/chat/android/state/extensions/ChatClient.kt
Show resolved
Hide resolved
@@ -205,7 +204,7 @@ private fun <T> ChatClient.getStateOrNull( | |||
*/ | |||
@CheckResult | |||
public fun ChatClient.setMessageForReply(cid: String, message: Message?): Call<Unit> { | |||
return CoroutineCall(state.scope) { | |||
return CoroutineCall(inheritScope { SupervisorJob(it) }) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @JcMinarro
Would you mind replacing SupervisorJob
with Job
?
I assume we don't need to supervise anything.
🎯 Goal
We were using some classes with the old approach to have it as a singleton and obtain it by
get()
method returning an exception if it wasn't initialized yet.This old way is not needed anymore as we have our own "DepencencyResolver" to find dependencies from plugins that were added to the SDK.
Fix: #4717
🛠 Implementation details
Configure
StatePlugin
to provide needed dependencies.ChatClient::resolveDependency()
method has been update to provide better error on the case plugins wasn't initialized properly☑️Contributor Checklist
General
develop
branchCode & documentation
☑️Reviewer Checklist
🎉 GIF