-
Notifications
You must be signed in to change notification settings - Fork 101
Introduce MessageViewModel
+ Show original translated message
#815
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
base: develop
Are you sure you want to change the base?
Conversation
@EnvironmentObject var channelViewModel: ChatChannelViewModel | ||
@EnvironmentObject var messageViewModel: MessageViewModel |
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 have a stashed commit that uses @EnvinronmentValues
instead, like we do with the @Environment(\.channelTranslationLanguage)
. Ofc, there are pros and cons here:
@EnvironmentObject:
🟢 No Optionals
🔴 Unsafe, can cause crashes easily if a customer is using views directly and do not provide the env object
@Environment:
🟢 Safer
🔴 More boilerplate code, to fall back to previous logic when the view model is optional
Either way, I think going for the @Environment
is better IMO. It is safer, we can use it in child views safely.
SDK Size
|
Generated by 🚫 Danger |
} | ||
}, | ||
label: { | ||
Text(messageViewModel.originalTextShown ? "Show Translation" : "Show Original") |
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.
Still requires adding L10n translation. And the UI is not fully done yet.
.applyDefaultSize() | ||
.environmentObject(ChatChannelViewModel(channelController: channelController)) |
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.
Add to update a lot of tests, and there is still more tests to update because of the @EnvironmentObject
. So another advantage of @Environment
is that it won't be required to update the tests since it will fallback to the previous implementation.
import StreamChat | ||
|
||
/// The view model that contains the logic for displaying a message in the message list view. | ||
open class MessageViewModel: ObservableObject { |
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.
Another advantage of @Environment
is that we can now remove the ObservableObject
from the MessageViewModel
. This is safer since the view model will be just dummy logic, and not contain internal state.
open class MessageViewModel: ObservableObject { | |
open class MessageViewModel { |
public struct LinkDetectionTextView: View { | ||
@Environment(\.layoutDirection) var layoutDirection | ||
@Environment(\.channelTranslationLanguage) var translationLanguage | ||
|
||
@EnvironmentObject private var viewModel: MessageViewModel |
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 one is a good example that @EnvironmentObject is not a good idea here, since LinkDetectionTextView
is likely to be reused as a standalone by other customers.
.onAppear { | ||
displayedText = attributedString(for: message) | ||
} | ||
.onChange(of: message, perform: { updated in | ||
displayedText = attributedString(for: updated) | ||
}) |
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 will probably need some help here CC @martinmitrevski @laevandus. This was overriding the view model content always, and was causing some trouble to me. Why is this needed?
var displayText: AttributedString { | ||
let text = viewModel.textContent |
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.
Having just this was enough to make it work, and I tried markdown etc and did not break anything for me.
if let displayedText { | ||
Text(displayedText) | ||
} else { | ||
Text(message.adjustedText) | ||
} |
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.
In case of LinkDetectionTextView
, I'm considering passing a new property, maybe originalText: String?
instead of the whole view model, so that it requires less changes. Still need to check if it would work.
🔗 Issue Links
https://linear.app/stream/issue/IOS-794/show-original-translation
https://linear.app/stream/issue/IOS-823/customizing-message-view-display-logic
🎯 Goal
TODO:
@EnvironmentObject
to@Environment
(Already have a WIP in stash)📝 Summary
Goal
The main goal of this PR is to introduce the new
MessageViewModel
which was required to properly implement the original translated message feature without breaking changes or too many hacks. Besides this, a lot of customers want to override the logic of the Message View, especially when to hide or show some views based on business logic.Requirement
One thing that was important to do was to have a way to share state from the
ChatChannelViewModel
to the newMessageViewModel
, since theMessageViewModel
should not have any internal state to not cause unnecessary re-renders. Besides that, since the message view is inside a LazyStack, the state would be overidden whenever the view is recreated. So it is important to provide the message view model to each message view whenever it is created.🛠 Implementation
Solution
The most common practice is to have item view models as the data of the
ListViewModel
. So instead ofmessages: [ChatMessage]
as the data, it would bemessages: [MessageViewModel]
. But because of ourLazyCachedMapCollection
, and to avoid breaking changes, we can't really do this.The final solution is to create a factory method in the
ChatChannelViewModel
, calledmakeMessageViewModel(message:)
. This approach allows us to pass data from the channel view model to the message view model, ensuring the view model is always updated whenever the view is re-created. The view model is then passed to theMessageContainerView
as an environment object. (Can't be @ObservedObject, otherwise it would be breaking)In order to provide a custom
MessageViewModel
, customers will need to subclass theMessageViewModel
and override the functionChatChannelViewModel.makeMessageViewModel()
.🧪 Manual Testing Notes
TODO
☑️ Contributor Checklist
docs-content
repo