Add 'send private read receipts' option in advanced settings #2290

jmartinesp · 2024-01-25T08:57:35Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Adds a 'read receipt' option to advanced settings so the client can save the last read message but won't share it with the other users in the rooms.

Motivation and context

Closes #2204.

Tests

Go to Advanced settings, disable this option.
Go to any room with unread messages where you have another account in the members. This should automatically mark the room as read.
Get out of the room, check the room is read.
From the other account, check the read receipt for your user didn't change in the timeline.

Tested devices

Physical
Emulator
OS version(s): 14

Checklist

Changes have been tested on an Android device or Android emulator with API 23
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request includes a new file under ./changelog.d. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#changelog
Pull request includes screenshots or videos if containing UI changes
Pull request includes a sign off
You've made a self review of your PR

github-actions · 2024-01-25T09:03:17Z

📱 Scan the QR code below to install the build (arm64 only) for this PR.

If you can't scan the QR code you can install the build via this link: https://i.diawi.com/kyA66F

bmarty

LGTM, 2 remarks. Also I think the screenshots have to be recorded.

bmarty · 2024-01-25T09:34:13Z

...rc/main/kotlin/io/element/android/features/preferences/impl/advanced/AdvancedSettingsView.kt

+                Text(text = "Send read receipts")
+            },
+            supportingContent = {
+                Text(text = "On disabling, others won't see your read receipt.")


Suggested change

Text(text = "On disabling, others won't see your read receipt.")

Text(text = "On disabling, others won't see your read receipts.")

Also I think those 2 strings should be defined in Localazy.

bmarty · 2024-01-25T09:37:14Z

...es/api/src/main/kotlin/io/element/android/features/preferences/api/store/PreferencesStore.kt

@@ -28,6 +28,9 @@ interface PreferencesStore {
    suspend fun setCustomElementCallBaseUrl(string: String?)
    fun getCustomElementCallBaseUrlFlow(): Flow<String?>

+    suspend fun setPrivateReadReceiptsEnabled(enabled: Boolean)
+    fun isPrivateReadReceiptsEnabled(): Flow<Boolean>


It's a bit annoying me that this settings is stored in PreferencesStore, which should be used for global settings only. The setting isPrivateReadReceiptsEnabled should have a Session scope.

That said, IIRC, we do not have (yet) a store for session preferences, and we do not support multi sessions yet. But, I think logging out and logging in again will keep the setting, and it's probably an unexpected side effect from the user POV.

I'll see how difficult it is to create a session-scoped preference store.

codecov · 2024-01-25T10:28:03Z

Codecov Report

Attention: 17 lines in your changes are missing coverage. Please review.

Comparison is base (99683d4) 70.15% compared to head (0429c3d) 70.14%.
Report is 21 commits behind head on develop.

Files	Patch %	Lines
...ences/impl/store/DefaultSessionPreferencesStore.kt	0.00%	10 Missing ⚠️
...eatureflag/test/InMemorySessionPreferencesStore.kt	62.50%	3 Missing ⚠️
...nt/android/features/call/ui/ElementCallActivity.kt	0.00%	2 Missing ⚠️
.../preferences/impl/advanced/AdvancedSettingsView.kt	60.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2290      +/-   ##
===========================================
- Coverage    70.15%   70.14%   -0.01%     
===========================================
  Files         1335     1337       +2     
  Lines        32802    32838      +36     
  Branches      6809     6811       +2     
===========================================
+ Hits         23012    23035      +23     
- Misses        6511     6524      +13     
  Partials      3279     3279

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

- Turn `sendPrivateReadReceipts` into `sendPublicReadReceipts`, make it true by default. - Create `SessionPreferencesStore` that stores the settings for the current use separate from those of the app. - Rename `PreferencesStore` to `AppPreferencesStore` to split the preferences. - Add and fix tests.

bmarty

Thanks for the change. There are a few remarks to handle.

bmarty · 2024-01-25T19:32:28Z

features/preferences/impl/src/main/res/values/localazy.xml

@@ -6,6 +6,8 @@
  <string name="screen_advanced_settings_developer_mode">"Developer mode"</string>
  <string name="screen_advanced_settings_developer_mode_description">"Enable to have access to features and functionality for developers."</string>
  <string name="screen_advanced_settings_rich_text_editor_description">"Disable the rich text editor to type Markdown manually."</string>
+  <string name="screen_advanced_settings_send_read_receipts">"Read receipts"</string>
+  <string name="screen_advanced_settings_send_read_receipts_description">"If turned off, your read receipts won\'t be sent to anyone. You will still receive read receipts from other users."</string>


You will still receive read receipts from other users.

Except if they also disabled the public RR :)

I think that's kind of implicit, but please let's not bring back the discussion on the text used here, I already had enough of it 😅 .

Yes, sure, I was sort of kidding :).

It reminds me the old days where mobile phone users could pay an option to get incoming call number displayed, and others could pay an option to hide their phone number when they're doing call :)

bmarty · 2024-01-25T19:34:31Z

...kotlin/io/element/android/libraries/preferences/impl/store/DefaultSessionPreferencesStore.kt

+    @ApplicationContext context: Context,
+    currentSessionIdHolder: CurrentSessionIdHolder,
+) : SessionPreferencesStore {
+    private val privateReadReceiptsKey = booleanPreferencesKey("privateReadReceipts")


Maybe rename to

Suggested change

private val privateReadReceiptsKey = booleanPreferencesKey("privateReadReceipts")

private val publicReadReceiptsKey = booleanPreferencesKey("publicReadReceipts")

bmarty · 2024-01-25T19:36:50Z

...kotlin/io/element/android/libraries/preferences/impl/store/DefaultSessionPreferencesStore.kt

+    private val privateReadReceiptsKey = booleanPreferencesKey("privateReadReceipts")
+
+    private val store = PreferenceDataStoreFactory.create {
+        context.preferencesDataStoreFile("session_${currentSessionIdHolder.current}_preferences")


You have to use a hash of the user id here, because it will crash if the user name is too long. See #2237

bmarty · 2024-01-25T19:37:08Z

...kotlin/io/element/android/libraries/preferences/impl/store/DefaultSessionPreferencesStore.kt

+    override fun isSendPublicReadReceiptsEnabled(): Flow<Boolean> = get(privateReadReceiptsKey, true)
+
+    override suspend fun clear() {
+        store.edit { it.clear() }


The file may also be removed.

Any attempt to read/write the file creates and closes a FileInputStream or FileOutputStream and is capable of handling errors if they're not found.

sonarcloud · 2024-01-26T07:41:31Z

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

bmarty

Thanks for the update! You can merge the PR like this.

bmarty · 2024-01-26T08:12:40Z

...kotlin/io/element/android/libraries/preferences/impl/store/DefaultSessionPreferencesStore.kt

+    private val hashedUserId = currentSessionIdHolder.current.value.hash().take(16)
+
+    private val dataStoreFile = context.preferencesDataStoreFile("session_${hashedUserId}_preferences")
+    private val store = PreferenceDataStoreFactory.create { dataStoreFile }


Just a small question, with this API, one can create a data store at any location, right?

I'm tempted to think so. This actually creates a File(dataStoreDir, filename) with filename being the string you pass here, and then calls mkdirs() so it should work that way.

bmarty · 2024-01-26T08:15:01Z

...kotlin/io/element/android/libraries/preferences/impl/store/DefaultSessionPreferencesStore.kt

+
+    private fun <T> get(key: Preferences.Key<T>, default: T): Flow<T> {
+        return store.data.map { prefs -> prefs[key] ?: default }
+    }


2 nice functions! I am tempted to extract them and reuse them elsewhere. I'll draft a quick PR.

bmarty · 2024-01-26T08:15:35Z

...src/main/kotlin/io/element/android/features/preferences/api/store/SessionPreferencesStore.kt

+    suspend fun setSendPublicReadReceipts(enabled: Boolean)
+    fun isSendPublicReadReceiptsEnabled(): Flow<Boolean>
+
+    suspend fun clear()


Note: this is never called. Not a big deal but should be fixed later.

Yeah, I need to figure out the right way to clean up this data when the user is logged out. I'll do it in another PR.

jmartinesp requested a review from a team as a code owner January 25, 2024 08:57

jmartinesp requested review from bmarty and removed request for a team January 25, 2024 08:57

jmartinesp added the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Jan 25, 2024

github-actions bot removed the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Jan 25, 2024

bmarty reviewed Jan 25, 2024

View reviewed changes

jmartinesp and others added 2 commits January 25, 2024 11:14

Add 'send private read receipts' option in advanced settings

2106fb0

Update screenshots

7c90e47

jmartinesp force-pushed the feature/2204-send-private-read-receipts branch from 421f113 to 7c90e47 Compare January 25, 2024 10:14

jmartinesp added 2 commits January 25, 2024 16:19

Add localazy strings

f826e65

jmartinesp added the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Jan 25, 2024

github-actions bot removed the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Jan 25, 2024

Update screenshots

21e3b1a

jmartinesp added the Run-Maestro Starts a Maestro Cloud session to run integration tests label Jan 25, 2024

github-actions bot removed the Run-Maestro Starts a Maestro Cloud session to run integration tests label Jan 25, 2024

bmarty reviewed Jan 25, 2024

View reviewed changes

jmartinesp added 3 commits January 26, 2024 08:34

Rename key in DefaultSessionPreferences

9a14623

Use hashed id as key for the pref store file name

da6c11a

On clear, just delete the whole file.

0429c3d

Any attempt to read/write the file creates and closes a FileInputStream or FileOutputStream and is capable of handling errors if they're not found.

bmarty approved these changes Jan 26, 2024

View reviewed changes

jmartinesp added the Run-Maestro Starts a Maestro Cloud session to run integration tests label Jan 26, 2024

github-actions bot removed the Run-Maestro Starts a Maestro Cloud session to run integration tests label Jan 26, 2024

jmartinesp merged commit 7e58f71 into develop Jan 26, 2024
16 of 18 checks passed

jmartinesp deleted the feature/2204-send-private-read-receipts branch January 26, 2024 09:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 'send private read receipts' option in advanced settings #2290

Add 'send private read receipts' option in advanced settings #2290

jmartinesp commented Jan 25, 2024 •

edited

github-actions bot commented Jan 25, 2024 •

edited

bmarty left a comment •

edited

bmarty Jan 25, 2024

bmarty Jan 25, 2024 •

edited

jmartinesp Jan 25, 2024

codecov bot commented Jan 25, 2024 •

edited

bmarty left a comment

bmarty Jan 25, 2024

jmartinesp Jan 26, 2024

bmarty Jan 26, 2024

bmarty Jan 25, 2024

jmartinesp Jan 26, 2024

bmarty Jan 25, 2024

jmartinesp Jan 26, 2024

bmarty Jan 25, 2024

jmartinesp Jan 26, 2024

sonarcloud bot commented Jan 26, 2024

bmarty left a comment

bmarty Jan 26, 2024

jmartinesp Jan 26, 2024

bmarty Jan 26, 2024

bmarty Jan 26, 2024

jmartinesp Jan 26, 2024

	Text(text = "On disabling, others won't see your read receipt.")
	Text(text = "On disabling, others won't see your read receipts.")

	private val privateReadReceiptsKey = booleanPreferencesKey("privateReadReceipts")
	private val publicReadReceiptsKey = booleanPreferencesKey("publicReadReceipts")

Add 'send private read receipts' option in advanced settings #2290

Add 'send private read receipts' option in advanced settings #2290

Conversation

jmartinesp commented Jan 25, 2024 • edited

Type of change

Content

Motivation and context

Tests

Tested devices

Checklist

github-actions bot commented Jan 25, 2024 • edited

bmarty left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty Jan 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 25, 2024 • edited

Codecov Report

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

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

sonarcloud bot commented Jan 26, 2024

Quality Gate passed

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

Choose a reason for hiding this comment

jmartinesp commented Jan 25, 2024 •

edited

github-actions bot commented Jan 25, 2024 •

edited

bmarty left a comment •

edited

bmarty Jan 25, 2024 •

edited

codecov bot commented Jan 25, 2024 •

edited