-
Notifications
You must be signed in to change notification settings - Fork 743
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
Moving voice logic to the MessageComposer #4523
Conversation
|
const val PAGINATION_COUNT = 50 | ||
|
||
@JvmStatic | ||
override fun create(viewModelContext: ViewModelContext, state: RoomDetailViewState): RoomDetailViewModel { |
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.
this workaround is needed specifically because of the ActivityScoped
voice helpers, have removed from here and added to the MessageComposer
where it's needed instead
data class ShowRoomUpgradeDialog(val newVersion: String, val isPublic: Boolean) : TextComposerViewEvents() | ||
data class ShowRoomUpgradeDialog(val newVersion: String, val isPublic: Boolean) : MessageComposerViewEvents() | ||
|
||
data class VoicePlaybackOrRecordingFailure(val throwable: Throwable) : MessageComposerViewEvents() |
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.
the VoicePlaybackOrRecordingFailure
is RoomDetailViewEvents.Failure
… updated) from the RoomDetailViewModel
…ce not just text)
db6e6c6
to
cca50ed
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.
LGTM, 2 small questions/remarks
@MavericksViewModelKey(TextComposerViewModel::class) | ||
fun textComposerViewModelFactory(factory: TextComposerViewModel.Factory): MavericksAssistedViewModelFactory<*, *> | ||
@MavericksViewModelKey(RoomDetailViewModel::class) | ||
fun roomDetailViewModelFactory(factory: RoomDetailViewModel.Factory): MavericksAssistedViewModelFactory<*, *> |
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.
This is not replaced by MessageComposerViewModel
?
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 not :( when a view model is not in this module it's because it needs a workaround https://github.com/vector-im/element-android/blob/develop/vector/src/main/java/im/vector/app/features/home/room/detail/RoomDetailViewModel.kt#L161
Usually we can do...
companion object : MavericksViewModelFactory<*ViewModel, *ViewState> by hiltMavericksViewModelFactory()
which is where the MavericksViewModelModule
comes in and injects for us
buttttt the voice helpers are marked as ActivityScoped
which hilt doesn't let us inject into the ViewModels
as they're some form of singleton scoped~, now that the voice helpers have moved to the MessageComposerViewModel
we can add the RoomDetailViewModel
to the normal way of providing the ViewModels
The workaround involves injecting the assisted factory into the fragment and then using that field to provide the ViewModel
@JvmStatic
override fun create(viewModelContext: ViewModelContext, state: *ViewState): *ViewModel {
val fragment: *Fragment = (viewModelContext as FragmentViewModelContext).fragment()
return fragment.factory.create(state)
}
} | ||
|
||
companion object : MavericksViewModelFactory<TextComposerViewModel, TextComposerViewState> by hiltMavericksViewModelFactory() | ||
/** | ||
* Can't use the hiltMaverick here because some dependencies are injected here and in fragment but they don't share the graph. |
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.
hiltMaverick
? :p
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.
😂 I'll update and put a few more specifics
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.
@@ -103,7 +103,7 @@ class VoiceMessageRecorderView @JvmOverloads constructor( | |||
voiceMessageViews.renderVisibilityChanged(parentChanged, visibility) | |||
} | |||
|
|||
fun display(recordingState: RecordingUiState) { | |||
fun render(recordingState: RecordingUiState) { |
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.
👍
277abe1
to
c0f8984
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.
Thanks, let's merge this one
Moves all the voice recording logic to the
MessageComposer
(previously calledTextComposer
) from theRoomDetailViewModel
, this will unlock simplifying the draft saving for voice messages (and potentially push more logic to the SDK)