-
Notifications
You must be signed in to change notification settings - Fork 907
Nick color #4826
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
Nick color #4826
Changes from 27 commits
7aaebd4
889a9a1
0aee15f
0e400dc
0109cde
f7d8127
cc15f9b
db97046
da10364
6c26cfc
ea01677
1ec0956
1019ffe
40d48cc
79a3be3
bf919b8
454baf8
7ce9ece
1d3cc52
364457d
07d2a15
ddadefd
1cb91ca
a7b72ed
6d8b5db
96d5652
51c9c2f
608e01a
02a8fd2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Allow changing nick colors from the member detail screen |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| /* | ||
| * Copyright (c) 2021 New Vector Ltd | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package im.vector.app.features.home | ||
|
|
||
| import com.airbnb.mvrx.MavericksState | ||
| import com.airbnb.mvrx.MavericksViewModelFactory | ||
| import dagger.assisted.Assisted | ||
| import dagger.assisted.AssistedFactory | ||
| import dagger.assisted.AssistedInject | ||
| import im.vector.app.core.di.MavericksAssistedViewModelFactory | ||
| import im.vector.app.core.di.hiltMavericksViewModelFactory | ||
| import im.vector.app.core.platform.EmptyAction | ||
| import im.vector.app.core.platform.EmptyViewEvents | ||
| import im.vector.app.core.platform.VectorViewModel | ||
| import im.vector.app.features.home.room.detail.timeline.helper.MatrixItemColorProvider | ||
| import kotlinx.coroutines.flow.launchIn | ||
| import kotlinx.coroutines.flow.map | ||
| import kotlinx.coroutines.flow.onEach | ||
| import org.matrix.android.sdk.api.session.Session | ||
| import org.matrix.android.sdk.api.session.accountdata.UserAccountDataTypes | ||
| import org.matrix.android.sdk.api.session.events.model.toModel | ||
| import org.matrix.android.sdk.flow.flow | ||
| import org.matrix.android.sdk.flow.unwrap | ||
| import timber.log.Timber | ||
|
|
||
| data class DummyState( | ||
| val dummy: Boolean = false | ||
| ) : MavericksState | ||
|
|
||
| class UserColorAccountDataViewModel @AssistedInject constructor( | ||
| @Assisted initialState: DummyState, | ||
| private val session: Session, | ||
| private val matrixItemColorProvider: MatrixItemColorProvider | ||
| ) : VectorViewModel<DummyState, EmptyAction, EmptyViewEvents>(initialState) { | ||
|
|
||
| @AssistedFactory | ||
| interface Factory : MavericksAssistedViewModelFactory<UserColorAccountDataViewModel, DummyState> { | ||
| override fun create(initialState: DummyState): UserColorAccountDataViewModel | ||
| } | ||
|
|
||
| companion object : MavericksViewModelFactory<UserColorAccountDataViewModel, DummyState> by hiltMavericksViewModelFactory() | ||
|
|
||
| init { | ||
| observeAccountData() | ||
| } | ||
|
|
||
| private fun observeAccountData() { | ||
| session.flow() | ||
| .liveUserAccountData(UserAccountDataTypes.TYPE_OVERRIDE_COLORS) | ||
| .unwrap() | ||
| .map { it.content.toModel<Map<String, String>>() } | ||
| .onEach { userColorAccountDataContent -> | ||
| if (userColorAccountDataContent == null) { | ||
| Timber.w("Invalid account data im.vector.setting.override_colors") | ||
| } | ||
| matrixItemColorProvider.setOverrideColors(userColorAccountDataContent) | ||
| } | ||
| .launchIn(viewModelScope) | ||
| } | ||
|
|
||
| override fun handle(action: EmptyAction) { | ||
| // No op | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,12 +16,14 @@ | |
|
|
||
| package im.vector.app.features.home.room.detail.timeline.helper | ||
|
|
||
| import android.graphics.Color | ||
| import androidx.annotation.ColorInt | ||
| import androidx.annotation.ColorRes | ||
| import androidx.annotation.VisibleForTesting | ||
| import im.vector.app.R | ||
| import im.vector.app.core.resources.ColorProvider | ||
| import org.matrix.android.sdk.api.util.MatrixItem | ||
| import timber.log.Timber | ||
| import javax.inject.Inject | ||
| import javax.inject.Singleton | ||
| import kotlin.math.abs | ||
|
|
@@ -44,6 +46,42 @@ class MatrixItemColorProvider @Inject constructor( | |
| } | ||
| } | ||
|
|
||
| fun setOverrideColors(overrideColors: Map<String, String>?) { | ||
| cache.clear() | ||
| overrideColors?.forEach { | ||
| setOverrideColor(it.key, it.value) | ||
| } | ||
| } | ||
|
|
||
| fun setOverrideColor(id: String, colorSpec: String?): Boolean { | ||
| val color = parseUserColorSpec(colorSpec) | ||
| return if (color == null) { | ||
| cache.remove(id) | ||
| false | ||
| } else { | ||
| cache[id] = color | ||
| true | ||
| } | ||
| } | ||
|
|
||
| @ColorInt | ||
| private fun parseUserColorSpec(colorText: String?): Int? { | ||
| return if (colorText.isNullOrBlank()) { | ||
| null | ||
| } else { | ||
| try { | ||
| if (colorText.length == 1) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will break if/when the palette is extended to 16 or more colors. Maybe it would be better to just try to parse it as
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, but the plan is to change this dialog to do something more user friendly, so it's acceptable like that to me.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, cool, thanks.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. being able to set the colour by index is quite cryptic, I wouldn't have expected an input with a hex string hint to also allow a 0-9 index something a UI iteration could help to improve
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this is really a hidden feature. Sort of easter egg. |
||
| colorProvider.getColor(getUserColorByIndex(colorText.toInt())) | ||
| } else { | ||
| Color.parseColor(colorText) | ||
| } | ||
| } catch (e: Throwable) { | ||
| Timber.e(e, "Unable to parse color $colorText") | ||
| null | ||
| } | ||
| } | ||
| } | ||
|
|
||
| companion object { | ||
| @ColorRes | ||
| @VisibleForTesting | ||
|
|
@@ -52,7 +90,12 @@ class MatrixItemColorProvider @Inject constructor( | |
|
|
||
| userId?.toList()?.map { chr -> hash = (hash shl 5) - hash + chr.code } | ||
|
|
||
| return when (abs(hash) % 8) { | ||
| return getUserColorByIndex(abs(hash)) | ||
| } | ||
|
|
||
| @ColorRes | ||
| private fun getUserColorByIndex(index: Int): Int { | ||
| return when (index % 8) { | ||
| 1 -> R.color.element_name_02 | ||
| 2 -> R.color.element_name_03 | ||
| 3 -> R.color.element_name_04 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,6 +43,7 @@ import im.vector.app.core.extensions.setTextOrHide | |
| import im.vector.app.core.platform.StateView | ||
| import im.vector.app.core.platform.VectorBaseFragment | ||
| import im.vector.app.core.utils.startSharePlainTextIntent | ||
| import im.vector.app.databinding.DialogBaseEditTextBinding | ||
| import im.vector.app.databinding.DialogShareQrCodeBinding | ||
| import im.vector.app.databinding.FragmentMatrixProfileBinding | ||
| import im.vector.app.databinding.ViewStubRoomMemberProfileHeaderBinding | ||
|
|
@@ -51,6 +52,7 @@ import im.vector.app.features.displayname.getBestName | |
| import im.vector.app.features.home.AvatarRenderer | ||
| import im.vector.app.features.home.room.detail.RoomDetailPendingAction | ||
| import im.vector.app.features.home.room.detail.RoomDetailPendingActionStore | ||
| import im.vector.app.features.home.room.detail.timeline.helper.MatrixItemColorProvider | ||
| import im.vector.app.features.roommemberprofile.devices.DeviceListBottomSheet | ||
| import im.vector.app.features.roommemberprofile.powerlevel.EditPowerLevelDialogs | ||
| import kotlinx.parcelize.Parcelize | ||
|
|
@@ -68,7 +70,8 @@ data class RoomMemberProfileArgs( | |
| class RoomMemberProfileFragment @Inject constructor( | ||
| private val roomMemberProfileController: RoomMemberProfileController, | ||
| private val avatarRenderer: AvatarRenderer, | ||
| private val roomDetailPendingActionStore: RoomDetailPendingActionStore | ||
| private val roomDetailPendingActionStore: RoomDetailPendingActionStore, | ||
| private val matrixItemColorProvider: MatrixItemColorProvider | ||
| ) : VectorBaseFragment<FragmentMatrixProfileBinding>(), | ||
| RoomMemberProfileController.Callback { | ||
|
|
||
|
|
@@ -200,6 +203,7 @@ class RoomMemberProfileFragment @Inject constructor( | |
| headerViews.memberProfileIdView.text = userMatrixItem.id | ||
| val bestName = userMatrixItem.getBestName() | ||
| headerViews.memberProfileNameView.text = bestName | ||
| headerViews.memberProfileNameView.setTextColor(matrixItemColorProvider.getColor(userMatrixItem)) | ||
| views.matrixProfileToolbarTitleView.text = bestName | ||
| avatarRenderer.render(userMatrixItem, headerViews.memberProfileAvatarView) | ||
| avatarRenderer.render(userMatrixItem, views.matrixProfileToolbarAvatarImageView) | ||
|
|
@@ -321,6 +325,26 @@ class RoomMemberProfileFragment @Inject constructor( | |
| navigator.openBigImageViewer(requireActivity(), view, userMatrixItem) | ||
| } | ||
|
|
||
| override fun onOverrideColorClicked(): Unit = withState(viewModel) { state -> | ||
| val inflater = requireActivity().layoutInflater | ||
| val layout = inflater.inflate(R.layout.dialog_base_edit_text, null) | ||
| val views = DialogBaseEditTextBinding.bind(layout) | ||
| views.editText.setText(state.userColorOverride) | ||
| views.editText.hint = "#000000" | ||
|
|
||
| MaterialAlertDialogBuilder(requireContext()) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. allowing the user to provide a hex string is quite verbose, perhaps another iteration of this could use a colour wheel UI
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't include a full-fledged UI for this in my original PR because I didn't have the necessary expertise to do it quickly, and it wasn't clear that the PR / idea would be accepted at all (this is mostly a PoC and a working solution for us). I do agree that a proper color selector should be created for this (that can select a color from the current palette or select an arbitrary RGB color) |
||
| .setTitle(R.string.room_member_override_nick_color) | ||
| .setView(layout) | ||
| .setPositiveButton(R.string.ok) { _, _ -> | ||
| val newColor = views.editText.text.toString() | ||
| if (newColor != state.userColorOverride) { | ||
| viewModel.handle(RoomMemberProfileAction.SetUserColorOverride(newColor)) | ||
| } | ||
| } | ||
| .setNegativeButton(R.string.cancel, null) | ||
| .show() | ||
| } | ||
|
|
||
| override fun onEditPowerLevel(currentRole: Role) { | ||
| EditPowerLevelDialogs.showChoice(requireActivity(), R.string.power_level_edit_title, currentRole) { newPowerLevel -> | ||
| viewModel.handle(RoomMemberProfileAction.SetPowerLevel(currentRole.value, newPowerLevel, true)) | ||
|
|
||
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.
does this type need to be added to the matrix spec somewhere?
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 have another PR that implements the same functionality in element-desktop (matrix-org/matrix-react-sdk#5626). As far as I understand keys under
im.vector.settingare free-to-use by clients, so I don't think it's absolutely necessary to include it in the matrix spec (apart from maybe documenting its existence so that other clients can pick this up if they wish)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.
Yes, you are right, using domain prefixed type is fine. Nowadays we should maybe consider using
io.element.prefix.More details here: https://spec.matrix.org/latest/#namespacing