-
Notifications
You must be signed in to change notification settings - Fork 1
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
Feature/mob 3434 entry widget poc #1064
base: feature/entry-widget-and-secure-conversations-v2
Are you sure you want to change the base?
Feature/mob 3434 entry widget poc #1064
Conversation
42a5f06
to
32c9911
Compare
I think we can also simplify and remove a lot of stuff related to |
Yes, I will just need to go through that stuff and refactor. |
@@ -326,6 +327,11 @@ public static CallVisualizer getCallVisualizer() { | |||
return Dependencies.getCallVisualizerManager(); | |||
} | |||
|
|||
public static Navigator getNavigator(@Nullable ArrayList<String> queues, | |||
@Nullable String contextAssetId) { // TODO: 01.08.2024 Wrap all params to some object? Ideally existing one, not new because there are to many config classes already. |
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.
contextAssetId
is visitor context id?
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, exactly.
348f6b2
to
5007a98
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.
Overall looks good. Left some questions regarding alternative method to start interaction using enum.
|
||
private val gliaWidgetsConfig = Dependencies.getSdkConfigurationManager().createWidgetsConfiguration() | ||
|
||
fun startToChat(context: Context) { |
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.
@andrews-moc @DavDo
While having dedicated method like startToChat
, startAudioCall
, startVideoCall
, startSecureConversations
is totally acceptable approach, an alternative way could be to have singe method someting like start(InteractionKind)
where InteractionKind
could enlist all possible kinds to start interaction.
In Swift it would be something like this:
enum InteractionKind {
case videoCall
case chat
case secureConversations
}
func start(interactionKind: InteractionKind, context: Context) {
...
Having dedicated enum for all possible interactions seems more appealing to me, because we avoid having several very similar public method declarations.
Does it make sense?
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.
Thank you for sharing this idea. It makes sense to me overall.
At the same time, I think that POC approach with separate functions has several advantages as well:
- it's more readable and closer the the way we think about it in real life
- anyway we would need separate functions, the difference is that with a single
start(InteractionKind)
method these methods would be private - although these functions have some common things, their implementation is different
I'm not against start(InteractionKind)
, let's hear other opinions.
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.
IMHO, this solution is not acceptable for this situation because I see a couple of issues.
- It is not enough flexible — in the case when some of the underlying methods (starting chat, video, or secure conversation) require additional parameters, we will have to deprecate this function and replace it with a new function/functions.
- It breaks the single responsibility principle - every
InteractionKind
type has its nature and will launch a completely different unit of our SDK, so it might be clearer if every single function was responsible only for starting one kind of interaction. - With SC V2 we will have additional functionality inside each of these functions. This is to choose the right screen based on an ongoing SC + unread messages combination, so even if we repeat some functionality (that actually can be moved to some internal function), it is better to have separate functions to reduce complexity.
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 would like to point out that the public access methods will be the main interaction point for integrators who will not be using the Entry Widget. So three defined functions seems less confusing even if I personally like the suggestion Igor proposed.
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 this case enum case will have associated required parameter so it's not a problem, at least in Swift. However if this is not possible in Kotlin, or will drastically differ, than it makes sense to use separate methods instead as it is now.
- Still enum with associated values allows singe entry point and we do not expand surface area for change/deprecation in future when all those methods will start to require additional parameters, so there would more to deprecate. I agree that it will grow complexity on the single method though.
- Agree totally.
I do not enforce the enum usage, especially because it may be problematic to have similar interface on both platforms. The goal is just to understand if it can bring some advantage. I'm fine with using separate methods.
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.
- It is also possible in Kotlin. I agree that this way we can avoid deprecation and make it more flexible.
Yes, I agree with @Pelkar. I like this kind of modern solution, but for public interfaces, I would prefer to have functions that are as straightforward as possible.
// Navigate to chat | ||
} | ||
|
||
fun startAudioCall(context: Context) { |
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.
@andrews-moc If I'm not mistaken, we can use application context from configuration properties, so most probably no need to explicitly pass it here.
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.
@DavDo I was not able to find a way to use it so far. Context is available for Glia core init only, as far as I see.
b6ee407
to
40ee60f
Compare
ed3b1d5
to
d26bc68
Compare
167d8de
to
94dee0f
Compare
- Bottom Sheet - Navigator - Start Call
- resolve merge conflicts
d26bc68
to
9b93d0d
Compare
206a8e8
to
76c6e8d
Compare
1e40530
to
c0e2124
Compare
8ae0b3e
to
c866c9d
Compare
Project plan (draft)
Simplified way of using Glia Widgets SDK
Please have a look and share your early feedback. 🙏
What was implemented in this POC?
EntryWidget(Navigator)
GliaWidgets.getNavigator(queues, contextAssetId)
Navigator
or explicitly passnull
, and createEntryWidget
using theNavigator
CallActivity
becomes internal,CallActivity
can now be started only throughNavigator
instead ofIntent
CallScreen
Screenshots: