Threads Beta opt-in mechanism #5692

ariskotsomitopoulos · 2022-04-04T16:55:53Z

Threads Beta Opt-In

If threads are disabled in Labs:

Hide the thread button in the room header. Keep the “Reply in thread” action in the bottom sheet, which shows a BETA badge until the stable version is released. On tapping on the “Reply in thread” action a modal is shown to enable threads in Labs. Ideally, this would be a one-step action (refer to “Beta opt-in — Modal (direct)”).

github-actions · 2022-04-04T17:26:50Z

Unit Test Results

110 files ±0 110 suites ±0 1m 18s ⏱️ +9s
195 tests ±0 195 ✔️ ±0 0 💤 ±0 0 ❌ ±0
650 runs ±0 650 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit 598892d. ± Comparison against base commit 3b8ffcf.

♻️ This comment has been updated with latest results.

ouchadam · 2022-04-05T09:43:24Z

vector/src/main/java/im/vector/app/features/home/room/detail/TimelineFragment.kt

@@ -2374,6 +2385,29 @@ class TimelineFragment @Inject constructor(
        }
    }

+    /**


we could avoid this comment by including the context in the function name displayThreadsBetaOptInDialog

The truth is thats a better name !

ouchadam · 2022-04-05T09:47:31Z

vector/src/main/res/layout/item_bottom_sheet_action.xml

@@ -82,4 +82,27 @@
        tools:ignore="MissingPrefix"
        tools:visibility="visible" />

+
+    <TextView


would be great to get a screenshot of this view 🙏

Will add to PR's description!

ouchadam · 2022-04-05T09:48:16Z

vector/src/main/res/layout/item_bottom_sheet_action.xml

+        android:visibility="gone"
+        tools:visibility="visible"
+        android:text="@string/beta_title_bottom_sheet_action"
+        android:textColor="@color/palette_white"


to double check, should this use the theme colours? I'm guessing we're not using them because the background colour is hardcoded blue

Yeap, we are not using them, its the same for both themes!

ouchadam · 2022-04-05T09:49:43Z

vector/src/main/java/im/vector/app/features/home/room/threads/ThreadsManager.kt

+     * Generates and return an Html spanned string to be rendered especially in dialogs
+     */
+    fun getBetaEnableThreadsMessage(): Spanned {
+        val href = "<a href='$learnMoreUrl'>Learn more</a>.<br><br>"


if we want to allow translations here we'll need to move this Learn more to the resources

thinking a little more, assuming the learnMoreUrl doesn't need to be dynamic, it could be in the string resource (we've done something similar for the EMS copy in the onboarding)

The easiest way to implement that functionality was to include <a href='$learnMoreUrl'>Learn more</a>.<br><br> in a string resources but I didn't want to do that. Thats why I decoupled the html part with the rest.

Yes I agree learn more can be also a string resource, will update it accordingly

I have not seen any http links in any of our string resources so I tried to avoid it. And if we do that we should mark it as not translatable to avoid trying to translate them. Where exactly we do that in the onboarding?

We already have several instance of "learn more" in the string, maybe create a generic one, and we can do some cleanup during the next welbate sync.

for 2., I've just realised it hasn't been pushed yet whilst we figure out the exact url 🤦 the benefit of including the url in the translatable strings is that we could also change the domain based on locale eg DE could use the .de domain myurl.de

although this only works if the url is static 🤔

not a blocker for the PR ^^^ there's no convention for having urls in code vs resources at the moment (from what I can tell at least!)

@bmarty I also observed it will do a clean up there.

@ouchadam I would suggest to create a separate string resource file for non translatable strings. And when we have dynamic URLs with multiple domains for sure we can add them in our translatable strings

If we want to translate the URL, this is fine, but please let the formatting out of the string resource.
Also I think the URL can be translated, but out of Weblate, since this is quite technical.

example of url in the resources: https://github.com./vector-im/element-android/blob/main/vector-config/src/main/res/values/config.xml#L7

ouchadam · 2022-04-05T09:53:19Z

vector/src/main/java/im/vector/app/features/home/room/detail/TimelineFragment.kt

+                        .setCancelable(true)
+                        .setNegativeButton(R.string.action_not_now) { _, _ -> }
+                        .setPositiveButton(R.string.action_try_it_out) { _, _ ->
+                            timelineViewModel.onCleared() // We should first clear our viewModel


do we need to clear the viewmodel if we're restarting the app? I'm wondering if we'll get it for free~

If we don't there is a crash in the live events observers

This is only happening if we RR the app from the Timeline, while the observers are active

ah that is strange, I expected restarting the app to also call timelineViewModel.onCleared(), perhaps there's a existing issue in the screen with the way the listeners are being set/released (if they're tied to the viewmodel lifecycle rather than the fragment)

not a blocker for this PR, just something to bear in mind for the future

Yes, definitely its something wrong here, manually clearing the viewModel it's a workaround

I agree this is quite strange to call onCleared manually (there is no other case like this in the codebase). There is probably another bug for your unexpected behavior.

ouchadam

tiny comment about the Learn more copy, otherwise LGTM! 💯

bmarty · 2022-04-05T10:18:28Z

vector/src/main/java/im/vector/app/core/epoxy/bottomsheet/BottomSheetActionItem.kt

@@ -106,12 +109,14 @@ abstract class BottomSheetActionItem : VectorEpoxyModel<BottomSheetActionItem.Ho
        } else {
            holder.text.setCompoundDrawablesWithIntrinsicBounds(null, null, null, null)
        }
+        holder.betaLabel.isVisible =  showBetaLabel


extra space! (ktlint should complain about it now)

bmarty · 2022-04-05T10:23:02Z

vector/src/main/java/im/vector/app/features/home/room/detail/TimelineFragment.kt

+     * Display a dialog that will let the user to enable threads
+     */
+
+    private fun displayThreadsBetaNotice() =


I do not like using = when the fun should not return anything. Can you use { instead?

Here it will return a TextView? which is highly unexpected.

I agree, and I prefer that than return Unit manually

bmarty · 2022-04-05T10:24:52Z

vector/src/main/java/im/vector/app/features/home/room/detail/TimelineFragment.kt

+                        .setCancelable(true)
+                        .setNegativeButton(R.string.action_not_now) { _, _ -> }
+                        .setPositiveButton(R.string.action_try_it_out) { _, _ ->
+                            timelineViewModel.onCleared() // We should first clear our viewModel


I agree this is quite strange to call onCleared manually (there is no other case like this in the codebase). There is probably another bug for your unexpected behavior.

bmarty · 2022-04-05T10:26:31Z

...main/java/im/vector/app/features/home/room/detail/timeline/action/MessageActionsViewModel.kt

@@ -450,7 +450,8 @@ class MessageActionsViewModel @AssistedInject constructor(
    private fun canReplyInThread(event: TimelineEvent,
                                 messageContent: MessageContent?,
                                 actionPermissions: ActionPermissions): Boolean {
-        if (!vectorPreferences.areThreadMessagesEnabled()) return false
+/* We let reply in thread visible even if threads are not enabled, with an enhanced flow to attract users */
+//        if (!vectorPreferences.areThreadMessagesEnabled()) return false


I would prefer to add some indentation here and for the comment above.

bmarty · 2022-04-05T10:28:18Z

vector/src/main/java/im/vector/app/features/home/room/threads/ThreadsManager.kt

+) {
+
+    companion object {
+        const val learnMoreUrl = "https://element.io/help#threads"


Probably something to move to the vector-config module, maybe in a new urls.xml resource file

Yes, thats what we discussed with adam

bmarty · 2022-04-05T10:29:10Z

vector/src/main/res/layout/item_bottom_sheet_action.xml

+        android:layout_marginEnd="1dp"
+        android:paddingBottom="3dp"
+        android:visibility="gone"
+        tools:visibility="visible"


please rearrange attributes

Yes, also maybe we can force ktlintformat to do that if possible

ktlint only works on Kotlin files AFAIK, not XML resources.
But if you found a solution for this, it could be very nice.

bmarty · 2022-04-05T10:30:12Z

vector/src/main/res/values/strings.xml

@@ -734,6 +735,8 @@
    <string name="search_thread_from_a_thread">From a Thread</string>
    <string name="threads_notice_migration_title">Threads Approaching Beta 🎉</string>
    <string name="threads_notice_migration_message">We’re getting closer to releasing a public Beta for Threads.\n\nAs we prepare for it, we need to make some changes: threads created before this point will be displayed as regular replies.\n\nThis will be a one-off transition as Threads are now part of the Matrix specification.</string>
+    <string name="threads_beta_enable_notice_title">Threads Beta</string>
+    <string name="threads_beta_enable_notice_message">Threads help keep your conversations on-topic and easy to track. %sEnabling threads will refresh the app. This may take longer for some accounts.</string>


Maybe add a comment above to explain with what %s will be replaced.

Will do yeap

bmarty

timelineViewModel.onCleared() is still annoying me :/

bmarty · 2022-04-05T15:32:17Z

vector-config/src/main/res/values/urls.xml

+<resources>
+    <!-- This file contains url values-->
+
+    <string name="threads_learn_more_url" translatable="false">https://element.io/help#threads</string>


This anchor threads does not exist (yet), and today this page does not mention thread the user experience will be quite bad. But @ariskotsomitopoulos told me that the webpage will be ready today or tomorrow.
So OK to merge the PR.

Yeap, it should be deployed before the release as we discussed. All platforms will release that url

Seems to be live now 👍

vector/src/main/res/values/strings_login_v2.xml

Fix typo

bmarty

We should find another way to avoid calling onCleared()

bmarty · 2022-04-05T16:10:37Z

vector/src/main/res/values/strings.xml

@@ -2137,7 +2142,6 @@
    <string name="direct_room_profile_encrypted_subtitle">Messages here are end-to-end encrypted.\n\nYour messages are secured with locks and only you and the recipient have the unique keys to unlock them.</string>
    <string name="room_profile_section_security">Security</string>
    <string name="room_profile_section_restore_security">Restore Encryption</string>
-    <string name="room_profile_section_security_learn_more">Learn more</string>


This is not the proper way to remove string. Please refer to this: https://github.com./vector-im/element-android/blob/main/CONTRIBUTING.md#removing-existing-strings

I will revert that and do that on a separate commit

This reverts commit 2ac3306.

…d objects

ariskotsomitopoulos · 2022-04-05T17:43:51Z

Update: We achieved to avoid manually calling viewModel onCleared, with an alternative approach. Thanks @ouchadam for that!

bmarty · 2022-04-05T17:46:11Z

vector/src/main/res/values/strings_login_v2.xml

@@ -43,7 +43,7 @@
    <string name="login_set_msisdn_title_2">Associate a phone number</string>
    <string name="login_set_msisdn_notice_2">Associate a phone number to optionally allow people you know to discover you.</string>
    <string name="login_set_msisdn_mandatory_notice_2">The server %s requires you to associate a phone number to create an account.</string>
-    <!-- %s will be replaced by the value of action_learn_more ("Learn more" in English)-->
+    <!-- %S will be replaced by the value of login_server_modular_learn_more ("Learn more" in English)-->


I think you have been a bit fast with the "replace all" command :/

I never use that command, now its reverted as is in develop, in develop is with capital %S, what do you mean?

bmarty · 2022-04-05T17:47:50Z

vector/src/main/java/im/vector/app/features/home/room/threads/ThreadsManager.kt

+class ThreadsManager @Inject constructor(
+        private val vectorPreferences: VectorPreferences,
+        private val lightweightSettingsStorage: LightweightSettingsStorage,
+        private val context: Context


could be a stringProvider here

Nice point, will update asap

bmarty

Thanks for the update! Happy to merge it now.

ariskotsomitopoulos added 2 commits April 4, 2022 19:51

Implement Beta opt-in mechanism to inform users about threads.

031c1b4

Add changelog

9e48b02

ariskotsomitopoulos requested review from a team and mnaturel and removed request for a team April 4, 2022 16:57

ouchadam added the Z-NextRelease For issues and PRs which should be included in the NextRelease. label Apr 5, 2022

ouchadam reviewed Apr 5, 2022

View reviewed changes

Rename displayThreadsBetaNotice method

2bb6e77

ouchadam approved these changes Apr 5, 2022

View reviewed changes

bmarty requested changes Apr 5, 2022

View reviewed changes

ariskotsomitopoulos added 6 commits April 5, 2022 13:36

Add learn more to string resources

7662abc

Remove duplicate learn more string resources

2ac3306

Remove extra space

e3ac5ad

Improve comment format

35a526a

Format code

6567628

Add urls.xml in vector-config

eecaf96

ariskotsomitopoulos requested a review from bmarty April 5, 2022 11:13

bmarty approved these changes Apr 5, 2022

View reviewed changes

Update vector/src/main/res/values/strings_login_v2.xml

c76dc7b

Fix typo

bmarty requested changes Apr 5, 2022

View reviewed changes

ariskotsomitopoulos added 3 commits April 5, 2022 19:39

Revert comment change

72d4c76

Revert "Remove duplicate learn more string resources"

5ee2579

This reverts commit 2ac3306.

Add string resource comment

59d87df

ariskotsomitopoulos requested a review from bmarty April 5, 2022 16:45

Add isValid check to RealmChangeListener to prevent crashes on delete…

37dc77c

…d objects

bmarty requested changes Apr 5, 2022

View reviewed changes

bmarty reviewed Apr 5, 2022

View reviewed changes

Injecting stringProvider instead of context in ThreadsManager class

598892d

ariskotsomitopoulos requested a review from bmarty April 5, 2022 17:58

bmarty approved these changes Apr 5, 2022

View reviewed changes

bmarty merged commit 0f14652 into develop Apr 5, 2022

bmarty deleted the feature/aris/threads_beta_infrom_users_on_reply branch April 5, 2022 18:40

@@ @@ -2374,6 +2385,29 @@ class TimelineFragment @Inject constructor( @@
                       }
                   }
+                  /**

Threads Beta opt-in mechanism #5692

Threads Beta opt-in mechanism #5692

Conversation

ariskotsomitopoulos commented Apr 4, 2022 • edited Loading

Threads Beta Opt-In

github-actions bot commented Apr 4, 2022 • edited Loading

Unit Test Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariskotsomitopoulos Apr 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ouchadam Apr 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ouchadam Apr 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ouchadam left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariskotsomitopoulos commented Apr 5, 2022

Choose a reason for hiding this comment

ariskotsomitopoulos Apr 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

ariskotsomitopoulos commented Apr 4, 2022 •

edited

Loading

github-actions bot commented Apr 4, 2022 •

edited

Loading

ariskotsomitopoulos Apr 5, 2022 •

edited

Loading

ouchadam Apr 5, 2022 •

edited

Loading

ouchadam Apr 5, 2022 •

edited

Loading

ouchadam left a comment •

edited

Loading

bmarty left a comment •

edited

Loading

ariskotsomitopoulos Apr 5, 2022 •

edited

Loading