-
Notifications
You must be signed in to change notification settings - Fork 338
Add alert to encrypted rooms with visible history (Android). #5709
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
base: develop
Are you sure you want to change the base?
Add alert to encrypted rooms with visible history (Android). #5709
Conversation
|
Thank you for your contribution! Here are a few things to check in the PR to ensure it's reviewed as quickly as possible:
|
appconfig/src/main/kotlin/io/element/android/appconfig/LearnMoreConfig.kt
Outdated
Show resolved
Hide resolved
| import kotlinx.coroutines.flow.Flow | ||
| import kotlinx.coroutines.flow.map | ||
|
|
||
| interface HistoryVisibleAcknowledgementRepository { |
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'm not sure if this is the correct naming scheme to use?
...otlin/io/element/android/features/messages/impl/crypto/historyvisible/HistoryVisibleState.kt
Outdated
Show resolved
Hide resolved
- Adds a dismissable alert that is displayed whenever the user opens a room with `history_visibility` != `joined`. When cleared, this is recorded in the app's data store. - When opening a room with `history_visibility` = `joined`, this flag is cleared.` Issue: element-hq/element-meta#2875
6c4104a to
46fba48
Compare
jmartinesp
left a comment
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.
Sorry for the long delay in reviewing this. I added a question, because I'm not sure I understand the logic behind one of the changes.
...element/android/features/messages/impl/crypto/historyvisible/HistoryVisibleStatePresenter.kt
Show resolved
Hide resolved
|
Could you record new screenshots following the instructions here? I've tried doing it myself but I think it'll fail since the action won't have permissions to add code to your fork. Thanks! |
| class DefaultHistoryVisibleAcknowledgementRepository( | ||
| preferenceDataStoreFactory: PreferenceDataStoreFactory, | ||
| ) : HistoryVisibleAcknowledgementRepository { | ||
| val store = preferenceDataStoreFactory.create("elementx_historyvisible") |
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.
To properly support multi-account, I believe that we would need to have a store per account. You can inject the sessionId in the constructor and use it to create a dedicated store.
Also when the session is deleted (log out), I guess we should do some cleanup? We can handle this part maybe since the code is not ideal so far regarding 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.
an other option is to have a single store, but include the sessionId in the preference keys.
Content
history_visibility!=joined. When cleared, this is recorded in the app's data store.history_visibility=joined, this flag is cleared.`Motivation and context
This is part of ongoing work to implement production-ready encrypted room history.
Issue: element-hq/element-meta#2875
Screenshots / GIFs
Tests
invitedhistory visibility, the new banner should be visible immediately upon opening the room.Tested devices
Checklist