-
Notifications
You must be signed in to change notification settings - Fork 99
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
Show translated text for participant messages #776
Conversation
cb445f7
to
0ca671b
Compare
var channelTranslationLanguages = [ChannelId: TranslationLanguage?]() | ||
|
||
// ChatMessage.adjustedText is used everywhere, but there is no easy way to get the | ||
// language used in the channel. Therefore, it is cached here. | ||
func translationLanguage(for cid: ChannelId) -> TranslationLanguage? { | ||
if let language = channelTranslationLanguages[cid] { | ||
return language | ||
} | ||
let language = InjectedValues[\.chatClient].channelController(for: cid).channel?.membership?.language | ||
channelTranslationLanguages[cid] = language | ||
return language | ||
} |
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.
Struggled to think about a good solution here. UIKit's implementation uses channel.membership.language
for figuring out which language to show if there are translations. ChatMessage.adjustText
has no direct access to get that information.
I am not really happy about how this looks like at the moment, but can't figure out a better way. Ideally, ChatMessage would have some sort of property which tells the language to use for translations. That would be must cleaner, but would involve adding a new property to ChatMessage. When ChatMessage is created, we could access channel's membership.language
and read the state from there. In summary, we would add ChatMessage.translationLanguage
or something similar. WDYT?
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.
We can have a discussion in the next meeting on Monday if needed.
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.
Adding a property to the message would simplify things, but then all SDKs would need to add it, so it kind of becomes complex.
The solution is also a bit hacky. Maybe we can simplify it by setting an active channel when the chat view appears in this caching utils, and clean it up when the channel is dismissed?
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 give it another try. This also needs to work in the channel list so I need to make sure it works there as well. I'll see if it would be better to try to explicitly handle it and not doing it in the adjustedText method where is no channel information available.
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 think I found a better way. It sure got too hacky with trying to do this in the adjustedText
.
0ca671b
to
3183a80
Compare
SDK Size
|
3183a80
to
b0dbffe
Compare
… preview message formatter to use channels translation language
private struct ChannelTranslationLanguageKey: EnvironmentKey { | ||
static let defaultValue: TranslationLanguage? = nil | ||
} | ||
|
||
extension EnvironmentValues { | ||
var channelTranslationLanguage: TranslationLanguage? { | ||
get { | ||
self[ChannelTranslationLanguageKey.self] | ||
} | ||
set { | ||
self[ChannelTranslationLanguageKey.self] = newValue | ||
} | ||
} | ||
} |
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.
Channel data is not available in the message's text view. Interfaces are public so no easy way to forward this information with function arguments. Next best way is to use environment key for this.
Note: MessageContainerView
is the last one to have access to channel.
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.
V5 is a good option to improve this.
func textContent(for translationLanguage: TranslationLanguage?) -> String? { | ||
guard let translationLanguage else { return nil } | ||
guard !isSentByCurrentUser, !isDeleted else { return nil } | ||
return translatedText(for: translationLanguage) | ||
} |
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.
Convenience since similar logic is used in multiple places
1b07ceb
to
f882035
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.
this also works, just check that it won't crash under circumstances where customers might replace views where we setup the environment.
@@ -300,6 +300,7 @@ public struct ComposerInputView<Factory: ViewFactory>: View, KeyboardReadable { | |||
isInComposer: true, | |||
scrolledId: .constant(nil) | |||
) | |||
.environment(\.channelTranslationLanguage, viewModel.channelController.channel?.membership?.language) |
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.
Customers might have their own message composer view implementation. Just to make sure, if we access it @Environment(\.channelTranslationLanguage) var translationLanguage
, but this line is not called, it won't crash, because it's optional, right?
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.
Yes, then it uses the the default value for the key which is nil
private struct ChannelTranslationLanguageKey: EnvironmentKey { | ||
static let defaultValue: TranslationLanguage? = nil | ||
} | ||
|
||
extension EnvironmentValues { | ||
var channelTranslationLanguage: TranslationLanguage? { | ||
get { | ||
self[ChannelTranslationLanguageKey.self] | ||
} | ||
set { | ||
self[ChannelTranslationLanguageKey.self] = newValue | ||
} | ||
} | ||
} |
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.
V5 is a good option to improve this.
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! ✅
# Conflicts: # CHANGELOG.md
|
🔗 Issue Link
Resolves: IOS-724
🎯 Goal
Support showing auto-translated text
🛠 Implementation
Note
Added debug view support since otherwise it is hard to manually test this.
When auto-translation is enabled, we get back translated text with the message payload.
ChatMessage.text
is always the original text.Added the same "Translated to XYZ" label to messages which are translated (like UIKit). Use channel's membership for accessing the preferred language (like UIKit).
🧪 Testing
es
en
Hola
, B sees it asHello
Hello
, A sees it asHola
🎨 Changes
translation_demo.mov
☑️ Checklist