Fix default encrypted for restricted #4083
Conversation
+ hide restricted rule if no current space selected
| } | ||
| ) | ||
| switchChecked(viewState.isEncrypted) | ||
| if (viewState.isEncrypted != null) { |
There was a problem hiding this comment.
NIT: Something I've noticed through the codebase is we tend to favour inverted if conditions, it might just be me but I find positive conditions much easier on the brain
if (viewState.isEncrypted == null) {
switchChecked(viewState.defaultEncrypted[viewState.roomJoinRules] ?: false)
} else {
switchChecked(viewState.isEncrypted)
}and whilst inverting the if ^^^ I also noticed we could simplify the checked state by chaining the fallbacks
val isSwitchChecked = viewState.isEncrypted ?: viewState.defaultEncrypted[viewState.roomJoinRules] ?: false
There was a problem hiding this comment.
Generally I agree to avoid if(!cond) {} else {}, but sometimes it also nice to have the most common case first. I'm not saying this is the case here, just wanted to add my two pence :)
There was a problem hiding this comment.
Yes I explicitly used the if/else like that thinking it will ease reading of the code (otherwise I would have gone for the chaining).
Looks like I failed :D.
| val allowed = if (state.supportsRestricted) { | ||
| // If restricted is supported and the user is in the context of a parent space | ||
| // then show restricted option. | ||
| val allowed = if (state.supportsRestricted && state.parentSpaceId != null) { |
There was a problem hiding this comment.
NIT: if we wanted to avoid using separate branches for appending to the list (and potentially be more kotlin idiomatic) we could use
listOfNotNull
val list = listOfNotNull(
INVITE,
PUBLIC,
RESTRICTED.takeIf { state.supportsRestricted && state.parentSpaceId != null }
)or buildList
val list = buildList {
add(INVITE)
add(PUBLIC)
if (state.supportsRestricted && state.parentSpaceId != null) {
add(RESTRICTED)
}
}There was a problem hiding this comment.
AS is telling me that it's experimental
There was a problem hiding this comment.
ah my bad, I totally forgot that buildList is still experimental
| if (state.isEncrypted) { | ||
| enableEncryption() | ||
| // we ignore the isEncrypted for public room as the switch is hidden in this case | ||
| if (state.roomJoinRules != RoomJoinRules.PUBLIC && state.isEncrypted != null) { |
There was a problem hiding this comment.
NIT: it might be clearer to convert this to a when and decouple isEncrypted creation from the enableEncryption()
val isEncrypted = when {
// public rooms use the default encrypted join rules as the switch is hidden
state.roomJoinRules == RoomJoinRules.PUBLIC -> state.defaultEncrypted[state.roomJoinRules] ?: false
else -> state.isEncrypted ?: false
}
if (isEncrypted) {
enableEncryption()
}what do you think?
There was a problem hiding this comment.
?: false can also get out the when branch. We also have a nice .orFalse extension.
@BillCarsonFr if you copy this code, please fix the typo in isEncrpyted 😛
There was a problem hiding this comment.
for some reason I can't type isEncrypted 🤦
There was a problem hiding this comment.
ah ah I definitly failed to make it clearer :D
|
awesome stuff! 💯 I'm attempting to use https://emmer.dev/blog/code-review-comment-prefixes/ to hopefully avoid confusion in my reviews 😅
would be handy to have screenshots for those of us who aren't familiar with the area (if possible) 🙏 |
| switchChecked(viewState.isEncrypted) | ||
| } else { | ||
| switchChecked(viewState.defaultEncrypted[viewState.roomJoinRules] ?: false) | ||
| } |
There was a problem hiding this comment.
In this case, I would have probably write:
switchChecked(
viewState.isEncrypted
?: viewState.defaultEncrypted[viewState.roomJoinRules]
?: false
)
| val aliasLocalPart: String? = null, | ||
| val isSubSpace: Boolean = false | ||
| val isSubSpace: Boolean = false, | ||
| val defaultEncrypted: Map<RoomJoinRules, Boolean> = emptyMap() |
There was a problem hiding this comment.
I'm sensitive to ordering declarations, why not adding this just after val isEncrypted, instead of appending it at the end?
BillCarsonFr
added
A-Spaces
Fixes #4045
Align default encrypted for restricted room as web is doing it, i.e use what is in well-known (default e2e).
Private and restricted encrypted switch is on if well-known e2e is by default.
=> Also if the user did manually change the encrypted toggled then always take that value and not the default.
Additional Fix:
When no space was selected, the option to set the room to restricted was visible, that would create a room restricted to nothing. So now if no space selected you'll only have a choice between Private or Public