SES-4753 : Manage Members #1665
Conversation
| @@ -0,0 +1,16 @@ | |||
| <vector xmlns:android="http://schemas.android.com/apk/res/android" | |||
| android:width="50dp" | |||
There was a problem hiding this comment.
Remember to set the icon's size to 24 when you import it to match our other icons.
You can still change it after the fact by updating to
android:width="24dp"
android:height="24dp"
But you can leave the viewport to 50
This icon should probably be allowed to be mirrored for RTL too with:
android:autoMirrored="true"
Which is also true for the other icon you added
| */ | ||
|
|
||
| @Composable | ||
| fun SearchBarWithCancel( |
There was a problem hiding this comment.
We already have a search bar component: SearchBar. Any reason to not just use that? The cancel can be added where needed in a row with it.
There was a problem hiding this comment.
I was thinking it would be easier to have this reusable component that handles the visibility of the Cancel .
There was a problem hiding this comment.
In theory composables should be stateless and pass the responsibility all the way up to the VM.
It's a bit painful at times, so if it feels over complicated you could leave it here, but I guess the question is still: Why not use out SearchBar?
Is it not compatible with this?
|
|
||
| // Output: List of only NON-ADMINS | ||
| @OptIn(FlowPreview::class) | ||
| val nonAdminMembers: StateFlow<List<GroupMemberState>> = combine( |
There was a problem hiding this comment.
Why recreate the members logic here? You already have members so you can map it directly:
val nonAdminMembers: StateFlow<List<GroupMemberState>> = members
.map { list -> list.filter { !it.showAsAdmin } }
.stateIn(viewModelScope, SharingStarted.Lazily, emptyList())
```
| import org.session.libsignal.utilities.Log | ||
|
|
||
| class InviteContactsJob(val groupSessionId: String, val memberSessionIds: Array<String>) : Job { | ||
| class InviteContactsJob(val groupSessionId: String, val memberSessionIds: Array<String>, val isReinvite : Boolean) : Job { |
There was a problem hiding this comment.
It might not be safe adding a property directly into an existing job. A job is persisted into db and when it's deserialized, funny thing could happen but i'm not very sure what the behavior here will be. You can try this by inviting someone to the group, using code prior to this PR, then quickly kill the app before the invite completes. Then you install the code from this PR and see whether you'll have trouble reintroduce the job. It could also be fine though, just need some testing
| } | ||
| ) { | ||
| // Look up current member configs once | ||
| val membersCfg = configFactory.withGroupConfigs(groupId) { it.groupMembers } |
There was a problem hiding this comment.
It's not safe to use the member config object outside of the withGroupConfigs function. The function is awkward to use to prevent this from happening. This is because the config objects are not thread safe, the withGroupConfigs holds a mutex to allow you to access it safely. You'll need to do things quickly with the group members inside the block.
There was a problem hiding this comment.
I see. Will update to this:
val invites: List<MemberInvite> = configFactory.withGroupConfigs(groupId) { cfg ->
selectedMembers.value.map { member ->
val shareHistory =
cfg.groupMembers.getOrNull(member.accountId.hexString)?.supplement == true
MemberInvite(id = member.accountId, shareHistory = shareHistory)
}
}
| navigateToInviteContact: (Set<String>) -> Unit, | ||
| onBack: () -> Unit, | ||
| ) { | ||
| ManageMembers( |
There was a problem hiding this comment.
This is using the old way of listing each actions one by one. You can see why we decided to switch for the VM Commands system as this quickly gets out of hand the more functionality you have.
It would have been a good occasion to switch it to commands as you created new screens.
It's ok for this one as it's already done, but it's good to keep in mind for new screens as it makes it a lot more manageable.
There was a problem hiding this comment.
Ah, I can clean this up since I have the sendCommand parameter already setup.
| removeMembersData: ManageGroupMembersViewModel.RemoveMembersState, | ||
| removeSearchState: (clearSelection : Boolean) -> Unit | ||
| ) { | ||
| val optionsList: List<ManageGroupMembersViewModel.OptionsItem> = listOf( |
There was a problem hiding this comment.
Wouldn't you want the VM to create this? This way we could have more buttonsin the future, also mixed in with logic, so the VM could handle choosing which buttons are displayed.
| } | ||
| } | ||
|
|
||
| //todo : Delete after implementing collapsing bottom |
There was a problem hiding this comment.
You have a few todo to remove certain elements, and it looks like they can be removed
| private val mutableError = MutableStateFlow<String?>(null) | ||
| val error: StateFlow<String?> get() = mutableError | ||
|
|
||
| private val _mutableOngoingAction = MutableStateFlow<String?>(null) |
There was a problem hiding this comment.
If you end up with a lot of different state for the screen, they could be solidified under one UiState object.
I'll leave it for you to decide out to organise it.
3 participants
This PR updates the Manage Members screen:
TO DOs:
manageMember.mov