Skip to content
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

Toggle IP address visibility (PSG-860) #7546

Merged
merged 17 commits into from
Nov 22, 2022
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/7546.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
[Device Manager] Toggle IP address visibility
2 changes: 2 additions & 0 deletions library/ui-strings/src/main/res/values/strings.xml
Original file line number Diff line number Diff line change
Expand Up @@ -3351,6 +3351,8 @@
<item quantity="one">Sign out of %1$d session</item>
<item quantity="other">Sign out of %1$d sessions</item>
</plurals>
<string name="device_manager_other_sessions_show_ip_address">Show IP address</string>
<string name="device_manager_other_sessions_hide_ip_address">Hide IP address</string>
<string name="device_manager_session_overview_signout">Sign out of this session</string>
<string name="device_manager_session_details_title">Session details</string>
<string name="device_manager_session_details_description">Application, device, and activity information.</string>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,9 @@ class VectorPreferences @Inject constructor(
private const val SETTINGS_SECURITY_USE_GRACE_PERIOD_FLAG = "SETTINGS_SECURITY_USE_GRACE_PERIOD_FLAG"
const val SETTINGS_SECURITY_USE_COMPLETE_NOTIFICATIONS_FLAG = "SETTINGS_SECURITY_USE_COMPLETE_NOTIFICATIONS_FLAG"

// New Session Manager
const val SETTINGS_SESSION_MANAGER_SHOW_IP_ADDRESS = "SETTINGS_SESSION_MANAGER_SHOW_IP_ADDRESS"

// other
const val SETTINGS_MEDIA_SAVING_PERIOD_KEY = "SETTINGS_MEDIA_SAVING_PERIOD_KEY"
private const val SETTINGS_MEDIA_SAVING_PERIOD_SELECTED_KEY = "SETTINGS_MEDIA_SAVING_PERIOD_SELECTED_KEY"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,4 +29,5 @@ sealed class DevicesAction : VectorViewModelAction {
object VerifyCurrentSession : DevicesAction()
data class MarkAsManuallyVerified(val cryptoDeviceInfo: CryptoDeviceInfo) : DevicesAction()
object MultiSignoutOtherSessions : DevicesAction()
object ToggleIpAddressVisibility : DevicesAction()
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,19 @@

package im.vector.app.features.settings.devices.v2

import android.content.SharedPreferences
import androidx.core.content.edit
import com.airbnb.mvrx.MavericksViewModelFactory
import com.airbnb.mvrx.Success
import dagger.assisted.Assisted
import dagger.assisted.AssistedFactory
import dagger.assisted.AssistedInject
import im.vector.app.core.di.ActiveSessionHolder
import im.vector.app.core.di.DefaultPreferences
import im.vector.app.core.di.MavericksAssistedViewModelFactory
import im.vector.app.core.di.hiltMavericksViewModelFactory
import im.vector.app.features.auth.PendingAuthHandler
import im.vector.app.features.settings.VectorPreferences
import im.vector.app.features.settings.devices.v2.filter.DeviceManagerFilterType
import im.vector.app.features.settings.devices.v2.signout.InterceptSignoutFlowResponseUseCase
import im.vector.app.features.settings.devices.v2.signout.SignoutSessionsReAuthNeeded
Expand All @@ -49,6 +53,8 @@ class DevicesViewModel @AssistedInject constructor(
private val interceptSignoutFlowResponseUseCase: InterceptSignoutFlowResponseUseCase,
private val pendingAuthHandler: PendingAuthHandler,
refreshDevicesUseCase: RefreshDevicesUseCase,
@DefaultPreferences
private val sharedPreferences: SharedPreferences,
) : VectorSessionsListViewModel<DevicesViewState, DevicesAction, DevicesViewEvent>(initialState, activeSessionHolder, refreshDevicesUseCase) {

@AssistedFactory
Expand All @@ -63,6 +69,14 @@ class DevicesViewModel @AssistedInject constructor(
observeDevices()
refreshDevicesOnCryptoDevicesChange()
refreshDeviceList()
refreshIpAddressVisibility()
}

private fun refreshIpAddressVisibility() {
val shouldShowIpAddress = sharedPreferences.getBoolean(VectorPreferences.SETTINGS_SESSION_MANAGER_SHOW_IP_ADDRESS, false)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should either define the default value for this preference somewhere in VectorPreferences or create a method to retrieve this setting so that it can be reused accross the screens.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, done.

setState {
copy(isShowingIpAddress = shouldShowIpAddress)
}
}

private fun observeCurrentSessionCrossSigningInfo() {
Expand Down Expand Up @@ -112,6 +126,17 @@ class DevicesViewModel @AssistedInject constructor(
is DevicesAction.VerifyCurrentSession -> handleVerifyCurrentSessionAction()
is DevicesAction.MarkAsManuallyVerified -> handleMarkAsManuallyVerifiedAction()
DevicesAction.MultiSignoutOtherSessions -> handleMultiSignoutOtherSessions()
DevicesAction.ToggleIpAddressVisibility -> handleToggleIpAddressVisibility()
}
}

private fun handleToggleIpAddressVisibility() = withState { state ->
val isShowingIpAddress = state.isShowingIpAddress
setState {
copy(isShowingIpAddress = !isShowingIpAddress)
}
sharedPreferences.edit {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to encapsulate the domain layer code inside a UseCase. I would see 2 options in prefered order:

  • if we have a way to observe settings in the app, we should have a usecase to observe the ip address visiblity setting and another one to toggle the setting.
  • if we don't have a way to observe settings, we should have a usecase to set the setting to a certain value.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created a use case to set the value.

putBoolean(VectorPreferences.SETTINGS_SESSION_MANAGER_SHOW_IP_ADDRESS, !isShowingIpAddress)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,4 +27,5 @@ data class DevicesViewState(
val unverifiedSessionsCount: Int = 0,
val inactiveSessionsCount: Int = 0,
val isLoading: Boolean = false,
val isShowingIpAddress: Boolean = false,
) : MavericksState
Original file line number Diff line number Diff line change
Expand Up @@ -146,11 +146,19 @@ class VectorSettingsDevicesFragment :
confirmMultiSignoutOtherSessions()
true
}
R.id.otherSessionsHeaderToggleIpAddress -> {
handleToggleIpAddressVisibility()
true
}
else -> false
}
}
}

private fun handleToggleIpAddressVisibility() {
viewModel.handle(DevicesAction.ToggleIpAddressVisibility)
}

private fun confirmMultiSignoutOtherSessions() {
activity?.let {
buildConfirmSignoutDialogUseCase.execute(it, this::multiSignoutOtherSessions)
Expand Down Expand Up @@ -240,7 +248,7 @@ class VectorSettingsDevicesFragment :

renderSecurityRecommendations(state.inactiveSessionsCount, state.unverifiedSessionsCount, isCurrentSessionVerified)
renderCurrentDevice(currentDeviceInfo)
renderOtherSessionsView(otherDevices)
renderOtherSessionsView(otherDevices, state.isShowingIpAddress)
} else {
hideSecurityRecommendations()
hideCurrentSessionView()
Expand Down Expand Up @@ -297,7 +305,7 @@ class VectorSettingsDevicesFragment :
hideInactiveSessionsRecommendation()
}

private fun renderOtherSessionsView(otherDevices: List<DeviceFullInfo>?) {
private fun renderOtherSessionsView(otherDevices: List<DeviceFullInfo>?, isShowingIpAddress: Boolean) {
if (otherDevices.isNullOrEmpty()) {
hideOtherSessionsView()
} else {
Expand All @@ -308,12 +316,18 @@ class VectorSettingsDevicesFragment :
multiSignoutItem.title = stringProvider.getQuantityString(R.plurals.device_manager_other_sessions_multi_signout_all, nbDevices, nbDevices)
multiSignoutItem.setTextColor(color)
views.deviceListOtherSessions.isVisible = true
val devices = if (isShowingIpAddress) otherDevices else otherDevices.map { it.copy(deviceInfo = it.deviceInfo.copy(lastSeenIp = null)) }
views.deviceListOtherSessions.render(
devices = otherDevices.take(NUMBER_OF_OTHER_DEVICES_TO_RENDER),
totalNumberOfDevices = otherDevices.size,
showViewAll = otherDevices.size > NUMBER_OF_OTHER_DEVICES_TO_RENDER
devices = devices.take(NUMBER_OF_OTHER_DEVICES_TO_RENDER),
totalNumberOfDevices = devices.size,
showViewAll = devices.size > NUMBER_OF_OTHER_DEVICES_TO_RENDER
)
}
views.deviceListHeaderOtherSessions.menu.findItem(R.id.otherSessionsHeaderToggleIpAddress).title = if (isShowingIpAddress) {
stringProvider.getString(R.string.device_manager_other_sessions_hide_ip_address)
} else {
stringProvider.getString(R.string.device_manager_other_sessions_show_ip_address)
}
}
}

private fun hideOtherSessionsView() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import im.vector.app.core.epoxy.ClickListener
import im.vector.app.core.epoxy.VectorEpoxyHolder
import im.vector.app.core.epoxy.VectorEpoxyModel
import im.vector.app.core.epoxy.onClick
import im.vector.app.core.extensions.setTextOrHide
import im.vector.app.core.resources.ColorProvider
import im.vector.app.core.resources.DrawableProvider
import im.vector.app.core.resources.StringProvider
Expand Down Expand Up @@ -69,6 +70,9 @@ abstract class OtherSessionItem : VectorEpoxyModel<OtherSessionItem.Holder>(R.la
@EpoxyAttribute
var selected: Boolean = false

@EpoxyAttribute
var ipAddress: String? = null

@EpoxyAttribute(EpoxyAttribute.Option.DoNotHash)
var clickListener: ClickListener? = null

Expand Down Expand Up @@ -100,6 +104,7 @@ abstract class OtherSessionItem : VectorEpoxyModel<OtherSessionItem.Holder>(R.la
holder.otherSessionDescriptionTextView.setTextColor(it)
}
holder.otherSessionDescriptionTextView.setCompoundDrawablesWithIntrinsicBounds(sessionDescriptionDrawable, null, null, null)
holder.otherSessionIpAddressTextView.setTextOrHide(ipAddress)
holder.otherSessionItemBackgroundView.isSelected = selected
}

Expand All @@ -108,6 +113,7 @@ abstract class OtherSessionItem : VectorEpoxyModel<OtherSessionItem.Holder>(R.la
val otherSessionVerificationStatusImageView by bind<ShieldImageView>(R.id.otherSessionVerificationStatusImageView)
val otherSessionNameTextView by bind<TextView>(R.id.otherSessionNameTextView)
val otherSessionDescriptionTextView by bind<TextView>(R.id.otherSessionDescriptionTextView)
val otherSessionIpAddressTextView by bind<TextView>(R.id.otherSessionIpAddressTextView)
val otherSessionItemBackgroundView by bind<View>(R.id.otherSessionItemBackground)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ class OtherSessionsController @Inject constructor(
sessionDescription(description)
sessionDescriptionDrawable(descriptionDrawable)
sessionDescriptionColor(descriptionColor)
ipAddress(device.deviceInfo.lastSeenIp)
stringProvider(host.stringProvider)
colorProvider(host.colorProvider)
drawableProvider(host.drawableProvider)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ class SessionInfoView @JvmOverloads constructor(
sessionInfoViewState.deviceFullInfo.isInactive,
sessionInfoViewState.deviceFullInfo.deviceInfo,
sessionInfoViewState.isLastSeenDetailsVisible,
sessionInfoViewState.isShowingIpAddress,
dateFormatter,
drawableProvider,
colorProvider,
Expand Down Expand Up @@ -157,6 +158,7 @@ class SessionInfoView @JvmOverloads constructor(
isInactive: Boolean,
deviceInfo: DeviceInfo,
isLastSeenDetailsVisible: Boolean,
isShowingIpAddress: Boolean,
dateFormatter: VectorDateFormatter,
drawableProvider: DrawableProvider,
colorProvider: ColorProvider,
Expand Down Expand Up @@ -187,6 +189,7 @@ class SessionInfoView @JvmOverloads constructor(
views.sessionInfoLastActivityTextView.isGone = true
}
views.sessionInfoLastIPAddressTextView.setTextOrHide(deviceInfo.lastSeenIp?.takeIf { isLastSeenDetailsVisible })
Copy link
Contributor

@mnaturel mnaturel Nov 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could write instead:
views.sessionInfoLastIPAddressTextView.setTextOrHide(deviceInfo.lastSeenIp?.takeIf { isLastSeenDetailsVisible && isShowingIpAddress })

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, done.

views.sessionInfoLastIPAddressTextView.isVisible = isShowingIpAddress
}

private fun renderDetailsButton(isDetailsButtonVisible: Boolean) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,5 @@ data class SessionInfoViewState(
val isDetailsButtonVisible: Boolean = true,
val isLearnMoreLinkVisible: Boolean = false,
val isLastSeenDetailsVisible: Boolean = false,
val isShowingIpAddress: Boolean = false,
)
Original file line number Diff line number Diff line change
Expand Up @@ -33,4 +33,5 @@ sealed class OtherSessionsAction : VectorViewModelAction {
object SelectAll : OtherSessionsAction()
object DeselectAll : OtherSessionsAction()
object MultiSignout : OtherSessionsAction()
object ToggleIpAddressVisibility : OtherSessionsAction()
}
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,12 @@ class OtherSessionsFragment :
menu.findItem(R.id.otherSessionsSelectAll).isVisible = isSelectModeEnabled
menu.findItem(R.id.otherSessionsDeselectAll).isVisible = isSelectModeEnabled
menu.findItem(R.id.otherSessionsSelect).isVisible = !isSelectModeEnabled && state.devices()?.isNotEmpty().orFalse()
menu.findItem(R.id.otherSessionsToggleIpAddress).isVisible = !isSelectModeEnabled
menu.findItem(R.id.otherSessionsToggleIpAddress).title = if (state.isShowingIpAddress) {
getString(R.string.device_manager_other_sessions_hide_ip_address)
} else {
getString(R.string.device_manager_other_sessions_show_ip_address)
}
updateMultiSignoutMenuItem(menu, state)
}
}
Expand Down Expand Up @@ -130,10 +136,18 @@ class OtherSessionsFragment :
confirmMultiSignout()
true
}
R.id.otherSessionsToggleIpAddress -> {
toggleIpAddressVisibility()
true
}
else -> false
}
}

private fun toggleIpAddressVisibility() {
viewModel.handle(OtherSessionsAction.ToggleIpAddressVisibility)
}

private fun confirmMultiSignout() {
activity?.let {
buildConfirmSignoutDialogUseCase.execute(it, this::multiSignout)
Expand Down Expand Up @@ -213,7 +227,7 @@ class OtherSessionsFragment :
updateLoading(state.isLoading)
if (state.devices is Success) {
val devices = state.devices.invoke()
renderDevices(devices, state.currentFilter)
renderDevices(devices, state.currentFilter, state.isShowingIpAddress)
updateToolbar(devices, state.isSelectModeEnabled)
}
}
Expand All @@ -237,7 +251,7 @@ class OtherSessionsFragment :
toolbar?.title = title
}

private fun renderDevices(devices: List<DeviceFullInfo>, currentFilter: DeviceManagerFilterType) {
private fun renderDevices(devices: List<DeviceFullInfo>, currentFilter: DeviceManagerFilterType, isShowingIpAddress: Boolean) {
views.otherSessionsFilterBadgeImageView.isVisible = currentFilter != DeviceManagerFilterType.ALL_SESSIONS
views.otherSessionsSecurityRecommendationView.isVisible = currentFilter != DeviceManagerFilterType.ALL_SESSIONS
views.deviceListHeaderOtherSessions.isVisible = currentFilter == DeviceManagerFilterType.ALL_SESSIONS
Expand Down Expand Up @@ -299,7 +313,8 @@ class OtherSessionsFragment :
} else {
views.deviceListOtherSessions.isVisible = true
views.otherSessionsNotFoundLayout.isVisible = false
views.deviceListOtherSessions.render(devices = devices, totalNumberOfDevices = devices.size, showViewAll = false)
val mappedDevices = if (isShowingIpAddress) devices else devices.map { it.copy(deviceInfo = it.deviceInfo.copy(lastSeenIp = null)) }
views.deviceListOtherSessions.render(devices = mappedDevices, totalNumberOfDevices = mappedDevices.size, showViewAll = false)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,19 @@

package im.vector.app.features.settings.devices.v2.othersessions

import android.content.SharedPreferences
import androidx.core.content.edit
import com.airbnb.mvrx.MavericksViewModelFactory
import com.airbnb.mvrx.Success
import dagger.assisted.Assisted
import dagger.assisted.AssistedFactory
import dagger.assisted.AssistedInject
import im.vector.app.core.di.ActiveSessionHolder
import im.vector.app.core.di.DefaultPreferences
import im.vector.app.core.di.MavericksAssistedViewModelFactory
import im.vector.app.core.di.hiltMavericksViewModelFactory
import im.vector.app.features.auth.PendingAuthHandler
import im.vector.app.features.settings.VectorPreferences
import im.vector.app.features.settings.devices.v2.GetDeviceFullInfoListUseCase
import im.vector.app.features.settings.devices.v2.RefreshDevicesUseCase
import im.vector.app.features.settings.devices.v2.VectorSessionsListViewModel
Expand All @@ -42,7 +46,9 @@ class OtherSessionsViewModel @AssistedInject constructor(
private val getDeviceFullInfoListUseCase: GetDeviceFullInfoListUseCase,
private val signoutSessionsUseCase: SignoutSessionsUseCase,
private val pendingAuthHandler: PendingAuthHandler,
refreshDevicesUseCase: RefreshDevicesUseCase
refreshDevicesUseCase: RefreshDevicesUseCase,
@DefaultPreferences
private val sharedPreferences: SharedPreferences,
) : VectorSessionsListViewModel<OtherSessionsViewState, OtherSessionsAction, OtherSessionsViewEvents>(
initialState, activeSessionHolder, refreshDevicesUseCase
) {
Expand All @@ -58,6 +64,14 @@ class OtherSessionsViewModel @AssistedInject constructor(

init {
observeDevices(initialState.currentFilter)
refreshIpAddressVisibility()
}

private fun refreshIpAddressVisibility() {
val shouldShowIpAddress = sharedPreferences.getBoolean(VectorPreferences.SETTINGS_SESSION_MANAGER_SHOW_IP_ADDRESS, false)
setState {
copy(isShowingIpAddress = shouldShowIpAddress)
}
}

private fun observeDevices(currentFilter: DeviceManagerFilterType) {
Expand Down Expand Up @@ -85,6 +99,17 @@ class OtherSessionsViewModel @AssistedInject constructor(
OtherSessionsAction.DeselectAll -> handleDeselectAll()
OtherSessionsAction.SelectAll -> handleSelectAll()
OtherSessionsAction.MultiSignout -> handleMultiSignout()
OtherSessionsAction.ToggleIpAddressVisibility -> handleToggleIpAddressVisibility()
}
}

private fun handleToggleIpAddressVisibility() = withState { state ->
val isShowingIpAddress = state.isShowingIpAddress
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same remark for adding a reusable usecase.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored.

setState {
copy(isShowingIpAddress = !isShowingIpAddress)
}
sharedPreferences.edit {
putBoolean(VectorPreferences.SETTINGS_SESSION_MANAGER_SHOW_IP_ADDRESS, !isShowingIpAddress)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ data class OtherSessionsViewState(
val excludeCurrentDevice: Boolean = false,
val isSelectModeEnabled: Boolean = false,
val isLoading: Boolean = false,
val isShowingIpAddress: Boolean = false,
) : MavericksState {

constructor(args: OtherSessionsArgs) : this(excludeCurrentDevice = args.excludeCurrentDevice)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,4 +29,5 @@ sealed class SessionOverviewAction : VectorViewModelAction {
val deviceId: String,
val enabled: Boolean,
) : SessionOverviewAction()
object ToggleIpAddressVisibility : SessionOverviewAction()
}
Loading