Mob 3604 devs want to implement business logic for the engagement launcher approach #1094

DavDo · 2024-09-26T22:17:08Z

Jira issue:
https://glia.atlassian.net/browse/MOB-3604

What was solved?

Remove all the configurations in favor of ConfigurationManager
Get rid of configurations passed through Activity extras
Move extra keys to internal class
Make activities internal
Minor refactoring

Release notes:

Feature
Ignore
Release notes (Is it clear from the description here?)
Migration guide (If changes are needed for integrator already using the SDK - what needs to be communicated? Add underneath please)

Additional info:

Is the feature sufficiently tested? All tests fixed? Necessary unit, acceptance, snapshots added? Check that at least new public classes & methods are covered with unit tests
Did you add logging beneficial for troubleshooting of customer issues?
Did you add new logging? We would like the logging between platforms to be similar. Refer to Logging from Android SDKs → Things to consider for newly added logs in Confluence for more information.

Screenshots:

gugalo

You did great work but could youplease separate your changes to smaller PRs. It is very hard to review such big PRs

gugalo · 2024-09-30T09:04:18Z

widgetssdk/src/main/java/com/glia/widgets/GliaWidgets.java

-    /**
-     * Use with {@link android.os.Bundle} to pass in a {@link UiTheme} as a navigation argument when
-     * navigating to {@link com.glia.widgets.chat.ChatActivity}
-     *
-     * @deprecated While UiTheme can still be used for UI customization,
-     * we strongly encourage adopting remote configurations {@link GliaWidgetsConfig.Builder#setUiJsonRemoteConfig(String)}.
-     * The remote configurations approach is more versatile and better suited for future development.
-     */
-    @Deprecated
-    public static final String UI_THEME = "ui_theme";
-    /**
-     * Use with {@link android.os.Bundle} to pass in the name of your company as a navigation
-     * argument when navigating to {@link com.glia.widgets.chat.ChatActivity}
-     *
-     * @deprecated Use {@link com.glia.widgets.GliaWidgetsConfig.Builder#companyName}
-     * or customize the strings from GliaHub
-     */
-    @Deprecated
-    public static final String COMPANY_NAME = "company_name";
-
-    /**
-     * Use with {@link android.os.Bundle} to pass in the ID of the queue you wish to enroll in
-     * as a navigation argument when navigating to {@link com.glia.widgets.chat.ChatActivity}
-     * or {@link com.glia.widgets.call.CallActivity}
-     *
-     * @deprecated Use {QUEUE_IDS} instead.
-     */
-    @Deprecated
-    public static final String QUEUE_ID = "queue_id";
-
-    /**
-     * Use with {@link android.os.Bundle} to pass in an {@link java.util.ArrayList} of {@link String} of the queues IDs
-     * you wish to enroll in as a navigation argument when navigating to {@link com.glia.widgets.chat.ChatActivity}
-     * or {@link com.glia.widgets.call.CallActivity}
-     */
-    public static final String QUEUE_IDS = "queue_ids";
-    /**
-     * Use with {@link android.os.Bundle} to pass in a context url as a navigation
-     * argument when navigating to {@link com.glia.widgets.chat.ChatActivity}
-     *
-     * @deprecated Use {@link com.glia.widgets.GliaWidgets#CONTEXT_ASSET_ID}
-     */
-    @Deprecated
-    public static final String CONTEXT_URL = "context_url";
-    /**
-     * Use with {@link android.os.Bundle} to pass in a context asset ID as a navigation
-     * argument when navigating to {@link com.glia.widgets.chat.ChatActivity}
-     */
-    public static final String CONTEXT_ASSET_ID = "context_asset_id";
-    /**
-     * It's recommended to use {@link GliaWidgetsConfig.Builder#setUseOverlay(boolean)} ()} instead of this constant directly.
-     * Use with {@link android.os.Bundle}  to pass in a boolean which represents if you would like to use the chat head bubble
-     * as an overlay outside your application for navigating to {@link com.glia.widgets.chat.ChatActivity}.
-     * If set to true then the SDK will ask for overlay permissions and try to always show the navigation bubble outside
-     * the application. However, it will be shown only if the user has accepted the permissions.
-     * If false, then overlay permission is not requested and the navigation bubble is shown when the application is active.
-     * Default value is true.
-     *
-     * @deprecated Use {@link com.glia.widgets.GliaWidgetsConfig#isEnableBubbleOutsideApp() and
-     * {@link GliaWidgetsConfig#isEnableBubbleInsideApp()}
-     */
-    @Deprecated
-    public static final String USE_OVERLAY = "use_overlay";


Remove of depricated properties should be a separate commit and even separate PR

I removed all the properties that are useless after these PR changes because they’re mainly used for intent extras that we closed from integrators, so there aren’t only the deprecated ones.
This is part of the breaking changes we will introduce in version 3.0.0. Do you think we need to somehow separate deprecated ones?

gugalo · 2024-09-30T09:05:35Z

widgetssdk/src/main/java/com/glia/widgets/GliaWidgets.java

@@ -208,6 +115,7 @@ private static void setupQueueIds(@Nullable List<String> queueIds) {
     *
     * <p>This method is no-op for other non-Glia triggered results.</p>
     */
+    //TODO check the possibilities to remove this method


Is this going to be done in this PR or should we create a separate jira ticket for this?

gugalo · 2024-09-30T09:08:14Z

widgetssdk/src/main/java/com/glia/widgets/UiTheme.kt

-    /**
-     * Text to be shown on the top of the app bar in the chat
-     */
-    val appBarTitle: String? = null,


This should also be a separate PR. Also, the release notes should be mentioned in the PR section as breaking changes. Same for the removed deprecated properties so that we could communicate this in the 3.0.0 release and migration guides

This property internally was not in use, so had no effect for a while, that is why I removed it.
Regarding release notes and migration guides, relying on the fact that we made activities internal and introduced EngagementLauncher instead should fully cover the migration part.

gugalo · 2024-09-30T09:09:33Z

widgetssdk/src/main/java/com/glia/widgets/call/CallActivity.kt

@@ -40,9 +42,8 @@ import kotlin.properties.Delegates
 * startActivity(intent);
 </pre> *
 */
-class CallActivity : FadeTransitionActivity() {
+internal class CallActivity : FadeTransitionActivity() {


👍 Finally! I tried to do this twice only to find there are still public methods/classes that require this to be public. Very cool that it is internal at last!

gugalo · 2024-09-30T09:22:39Z

widgetssdk/src/main/java/com/glia/widgets/chat/ChatActivity.kt

+        val chatType = intent.getIntExtra(ExtraKeys.CHAT_TYPE, -1).takeIf { it != -1 }
+            ?.let { ChatType.values()[it] } ?: ChatType.LIVE_CHAT


Could you extract this code to some to some IntentExtentions class with a method name fun Intent.getChatType and also extract any calls to intent.putExtra(CHAT_TYPE, ChatType.SECURE_MESSAGING as Parcelable) to the same class with a name intent.putChatType().

I know that technically your solution is completely safe even for any future changes to enums but I really would still prefer to have such logic in one place.

gugalo · 2024-09-30T09:24:24Z

widgetssdk/src/main/java/com/glia/widgets/call/CallActivity.kt

+            val mediaType = intent.getIntExtra(ExtraKeys.MEDIA_TYPE, -1)
+                .takeIf { it != -1 }?.let { Engagement.MediaType.values()[it] }


Commented similar logic below

app/src/main/java/com/glia/exampleapp/Application.java

app/src/main/java/com/glia/exampleapp/MainFragment.kt

app/src/main/java/com/glia/exampleapp/Utils.java

andrews-moc · 2024-09-30T12:48:01Z

widgetssdk/src/main/java/com/glia/widgets/GliaWidgets.java

@@ -226,6 +134,7 @@ public static void onRequestPermissionsResult(int requestCode, String[] permissi
     *
     * <p>This method is no-op for other non-Glia triggered results.</p>
     */
+    //TODO check the possibilities to remove this method


The same as Alex asked for another TODO, is this going to be done in this PR or should we create a separate Jira ticket for this?

I will remove TODOs, I think it makes sense to have separate tickets for these two functions.
Do you think it makes sense to include this in this project?
This should be unused since we introduced ActivityWatchers for screen sharing and permissions, but unfortunately, we forgot to deprecate it.

I think a separate ticket is better. It doesn't seem to be essential for our current projects.

I will mark these methods as deprecated since we're not using them anymore.

widgetssdk/src/main/java/com/glia/widgets/UiTheme.kt

widgetssdk/src/main/java/com/glia/widgets/callvisualizer/EndScreenSharingView.kt

widgetssdk/src/main/java/com/glia/widgets/chat/adapter/holder/OperatorStatusViewHolder.kt

widgetssdk/src/main/java/com/glia/widgets/view/head/ChatHeadLayout.kt

andrews-moc · 2024-09-30T14:49:27Z

@DavDo You've done a good deal of work! 💪 Agree with @gugalo, it would be much easier to review and spot issues if a huge PR is split into smaller parts.

DavDo · 2024-09-30T14:56:29Z

Thank you guys for your review.
I will try to address comments and split this PR into separate smaller parts, but since there are a lot of connections, I'm afraid that there will still be one that is comparatively big.

andrews-moc · 2024-09-30T15:03:37Z

Taking into account the work done already, it makes sense to proceed with this PR, isn't it? Otherwise we would have to review everything once again. Not sure how to do better.

DavDo · 2024-09-30T15:06:47Z

Taking into account the work done already, it makes sense to proceed with this PR, isn't it? Otherwise we would have to review everything once again. Not sure how to do better.

I'd also prefer to proceed with this PR, but I'm also ready to split it if you really want it.

- Remove all the configurations in favor of ConfigurationManager - Get rid of configurations passed through Activity extras - Move extra keys to internal class - Make activities internal - Minor refactoring

DavDo requested review from gugalo, andrews-moc and AndriiHorishniiMOC September 26, 2024 22:17

DavDo requested a review from a team as a code owner September 26, 2024 22:17

gugalo requested changes Sep 30, 2024

View reviewed changes