-
Notifications
You must be signed in to change notification settings - Fork 749
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
Deferred DMs - Add and enable the feature by default in the labs settings #7180
Conversation
d74ae75
to
fc997e1
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.
LGTM, 2 remarks though.
@@ -124,7 +124,7 @@ class CreateDirectRoomViewModel @AssistedInject constructor( | |||
} | |||
|
|||
val result = runCatchingToAsync { | |||
if (vectorFeatures.shouldStartDmOnFirstMessage()) { | |||
if (vectorPreferences.isDeferredDmEnabled()) { |
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.
Just a remark as I am seeing this now: can you check that analytics will be sent when the room will be created?
If the feature is disabled, it's sent in the else
block.
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.
Good remark, done fa8b56b
I did not find a better place to add it
@@ -37,6 +37,7 @@ | |||
<bool name="settings_ignored_users_visible">true</bool> | |||
|
|||
<!-- Level 1: Labs --> | |||
<bool name="settings_labs_deferred_dm_default">true</bool> |
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.
See the doc at the top of this file, you should add another key with visible
suffix:
<bool name="settings_labs_deferred_dm_visible">true</bool>
and use it in the labs pref file.
Maybe for this very temporary setting you could take this remark as optional.
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.
Done dd92bb7 but the "doc" at the beginning of the file does not tell that these fields are "mandatory", it's more a description of the naming convention.
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.
OK, thanks 👐
1ee1a3e
to
fa8b56b
Compare
SonarCloud Quality Gate failed. |
Type of change
Content
Add the deferred DMs feature in the labs settings, enabled by default
Motivation and context
Release #5525
Screenshots / GIFs
Tests
Tested devices
Checklist