Editing profile: No change warning on leave. (#3972)

### Objective
* Prevent data loss when the user inadvertently hits back or wants to
leave the profile edition with unsaved changes.

### Description
* To limit the number of changes to the existing codebase, I merely
re-used the same method used by `save()` in the ViewModel to decide
whether to make a network request or simply return the profile as-is.
* ~A bit of code juggling around in the ViewModel and I was able to use
the logic for all the encoding of each profile field (Which is what the
ViewModel caches in memory).~ Thanks @Lakoja for improving this in the
VM.
* A couple of internal data classes used as helpers to move all the
fields around (now that they are no longer used in one single place)
were introduced.

### Potential Optimizations
* ~The profile encoding is done twice (once for checking, and then again
if the user has to actually save it). I'd say this is a negligible price
to pay, since the alternative would be to create a different set of
comparisons and/or keeping another profile in memory for the purpose of
comparison.~

### Visual Improvement
* I believe the Dialog is difficult to see, but it's being displayed
with Tusky's theme. Perhaps there's a better style to apply in this
case? (or maybe the edit profile activity shouldn't have the same
background color as dialogs?!)


### Issue
* #3486
This commit is contained in:
Levi Bard 2023-08-28 14:13:24 +02:00 committed by GitHub
commit 10a6b1616a
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 153 additions and 74 deletions

View File

@ -25,7 +25,9 @@ import android.view.Menu
import android.view.MenuItem import android.view.MenuItem
import android.view.View import android.view.View
import android.widget.ImageView import android.widget.ImageView
import androidx.activity.OnBackPressedCallback
import androidx.activity.viewModels import androidx.activity.viewModels
import androidx.appcompat.app.AlertDialog
import androidx.core.view.isVisible import androidx.core.view.isVisible
import androidx.lifecycle.LiveData import androidx.lifecycle.LiveData
import androidx.lifecycle.lifecycleScope import androidx.lifecycle.lifecycleScope
@ -46,9 +48,11 @@ import com.keylesspalace.tusky.di.ViewModelFactory
import com.keylesspalace.tusky.util.Error import com.keylesspalace.tusky.util.Error
import com.keylesspalace.tusky.util.Loading import com.keylesspalace.tusky.util.Loading
import com.keylesspalace.tusky.util.Success import com.keylesspalace.tusky.util.Success
import com.keylesspalace.tusky.util.await
import com.keylesspalace.tusky.util.show import com.keylesspalace.tusky.util.show
import com.keylesspalace.tusky.util.viewBinding import com.keylesspalace.tusky.util.viewBinding
import com.keylesspalace.tusky.viewmodel.EditProfileViewModel import com.keylesspalace.tusky.viewmodel.EditProfileViewModel
import com.keylesspalace.tusky.viewmodel.ProfileDataInUi
import com.mikepenz.iconics.IconicsDrawable import com.mikepenz.iconics.IconicsDrawable
import com.mikepenz.iconics.typeface.library.googlematerial.GoogleMaterial import com.mikepenz.iconics.typeface.library.googlematerial.GoogleMaterial
import com.mikepenz.iconics.utils.colorInt import com.mikepenz.iconics.utils.colorInt
@ -96,6 +100,14 @@ class EditProfileActivity : BaseActivity(), Injectable {
} }
} }
private val currentProfileData
get() = ProfileDataInUi(
displayName = binding.displayNameEditText.text.toString(),
note = binding.noteEditText.text.toString(),
locked = binding.lockedCheckBox.isChecked,
fields = accountFieldEditAdapter.getFieldData()
)
override fun onCreate(savedInstanceState: Bundle?) { override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState) super.onCreate(savedInstanceState)
@ -200,17 +212,26 @@ class EditProfileActivity : BaseActivity(), Injectable {
} }
} }
} }
val onBackCallback = object : OnBackPressedCallback(enabled = true) {
override fun handleOnBackPressed() = checkForUnsavedChanges()
}
onBackPressedDispatcher.addCallback(this, onBackCallback)
}
fun checkForUnsavedChanges() {
if (viewModel.hasUnsavedChanges(currentProfileData)) {
showUnsavedChangesDialog()
} else {
finish()
}
} }
override fun onStop() { override fun onStop() {
super.onStop() super.onStop()
if (!isFinishing) { if (!isFinishing) {
viewModel.updateProfile( viewModel.updateProfile(currentProfileData)
binding.displayNameEditText.text.toString(),
binding.noteEditText.text.toString(),
binding.lockedCheckBox.isChecked,
accountFieldEditAdapter.getFieldData()
)
} }
} }
@ -287,14 +308,7 @@ class EditProfileActivity : BaseActivity(), Injectable {
return super.onOptionsItemSelected(item) return super.onOptionsItemSelected(item)
} }
private fun save() { private fun save() = viewModel.save(currentProfileData)
viewModel.save(
binding.displayNameEditText.text.toString(),
binding.noteEditText.text.toString(),
binding.lockedCheckBox.isChecked,
accountFieldEditAdapter.getFieldData()
)
}
private fun onSaveFailure(msg: String?) { private fun onSaveFailure(msg: String?) {
val errorMsg = msg ?: getString(R.string.error_media_upload_sending) val errorMsg = msg ?: getString(R.string.error_media_upload_sending)
@ -306,4 +320,16 @@ class EditProfileActivity : BaseActivity(), Injectable {
Log.w("EditProfileActivity", "failed to pick media", throwable) Log.w("EditProfileActivity", "failed to pick media", throwable)
Snackbar.make(binding.avatarButton, R.string.error_media_upload_sending, Snackbar.LENGTH_LONG).show() Snackbar.make(binding.avatarButton, R.string.error_media_upload_sending, Snackbar.LENGTH_LONG).show()
} }
private fun showUnsavedChangesDialog() = lifecycleScope.launch {
when (launchSaveDialog()) {
AlertDialog.BUTTON_POSITIVE -> save()
else -> finish()
}
}
private suspend fun launchSaveDialog() = AlertDialog.Builder(this)
.setMessage(getString(R.string.dialog_save_profile_changes_message))
.create()
.await(R.string.action_save, R.string.action_discard)
} }

View File

@ -42,7 +42,6 @@ import kotlinx.coroutines.flow.shareIn
import kotlinx.coroutines.launch import kotlinx.coroutines.launch
import okhttp3.MediaType.Companion.toMediaTypeOrNull import okhttp3.MediaType.Companion.toMediaTypeOrNull
import okhttp3.MultipartBody import okhttp3.MultipartBody
import okhttp3.RequestBody
import okhttp3.RequestBody.Companion.asRequestBody import okhttp3.RequestBody.Companion.asRequestBody
import okhttp3.RequestBody.Companion.toRequestBody import okhttp3.RequestBody.Companion.toRequestBody
import java.io.File import java.io.File
@ -51,6 +50,13 @@ import javax.inject.Inject
private const val HEADER_FILE_NAME = "header.png" private const val HEADER_FILE_NAME = "header.png"
private const val AVATAR_FILE_NAME = "avatar.png" private const val AVATAR_FILE_NAME = "avatar.png"
internal data class ProfileDataInUi(
val displayName: String,
val note: String,
val locked: Boolean,
val fields: List<StringField>
)
class EditProfileViewModel @Inject constructor( class EditProfileViewModel @Inject constructor(
private val mastodonApi: MastodonApi, private val mastodonApi: MastodonApi,
private val eventHub: EventHub, private val eventHub: EventHub,
@ -66,7 +72,7 @@ class EditProfileViewModel @Inject constructor(
val instanceData: Flow<InstanceInfo> = instanceInfoRepo::getInstanceInfo.asFlow() val instanceData: Flow<InstanceInfo> = instanceInfoRepo::getInstanceInfo.asFlow()
.shareIn(viewModelScope, SharingStarted.Eagerly, replay = 1) .shareIn(viewModelScope, SharingStarted.Eagerly, replay = 1)
private var oldProfileData: Account? = null private var apiProfileAccount: Account? = null
fun obtainProfile() = viewModelScope.launch { fun obtainProfile() = viewModelScope.launch {
if (profileData.value == null || profileData.value is Error) { if (profileData.value == null || profileData.value is Error) {
@ -74,7 +80,7 @@ class EditProfileViewModel @Inject constructor(
mastodonApi.accountVerifyCredentials().fold( mastodonApi.accountVerifyCredentials().fold(
{ profile -> { profile ->
oldProfileData = profile apiProfileAccount = profile
profileData.postValue(Success(profile)) profileData.postValue(Success(profile))
}, },
{ {
@ -96,68 +102,49 @@ class EditProfileViewModel @Inject constructor(
headerData.value = getHeaderUri() headerData.value = getHeaderUri()
} }
fun save(newDisplayName: String, newNote: String, newLocked: Boolean, newFields: List<StringField>) { internal fun save(newProfileData: ProfileDataInUi) {
if (saveData.value is Loading || profileData.value !is Success) { if (saveData.value is Loading || profileData.value !is Success) {
return return
} }
saveData.value = Loading() saveData.value = Loading()
val displayName = if (oldProfileData?.displayName == newDisplayName) { val diff = getProfileDiff(apiProfileAccount, newProfileData)
null if (!diff.hasChanges()) {
} else { // if nothing has changed, there is no need to make an api call
newDisplayName.toRequestBody(MultipartBody.FORM) saveData.value = Success()
}
val note = if (oldProfileData?.source?.note == newNote) {
null
} else {
newNote.toRequestBody(MultipartBody.FORM)
}
val locked = if (oldProfileData?.locked == newLocked) {
null
} else {
newLocked.toString().toRequestBody(MultipartBody.FORM)
}
val avatar = if (avatarData.value != null) {
val avatarBody = getCacheFileForName(AVATAR_FILE_NAME).asRequestBody("image/png".toMediaTypeOrNull())
MultipartBody.Part.createFormData("avatar", randomAlphanumericString(12), avatarBody)
} else {
null
}
val header = if (headerData.value != null) {
val headerBody = getCacheFileForName(HEADER_FILE_NAME).asRequestBody("image/png".toMediaTypeOrNull())
MultipartBody.Part.createFormData("header", randomAlphanumericString(12), headerBody)
} else {
null
}
// when one field changed, all have to be sent or they unchanged ones would get overridden
val fieldsUnchanged = oldProfileData?.source?.fields == newFields
val field1 = calculateFieldToUpdate(newFields.getOrNull(0), fieldsUnchanged)
val field2 = calculateFieldToUpdate(newFields.getOrNull(1), fieldsUnchanged)
val field3 = calculateFieldToUpdate(newFields.getOrNull(2), fieldsUnchanged)
val field4 = calculateFieldToUpdate(newFields.getOrNull(3), fieldsUnchanged)
if (displayName == null && note == null && locked == null && avatar == null && header == null &&
field1 == null && field2 == null && field3 == null && field4 == null
) {
/** if nothing has changed, there is no need to make a network request */
saveData.postValue(Success())
return return
} }
viewModelScope.launch { viewModelScope.launch {
var avatarFileBody: MultipartBody.Part? = null
diff.avatarFile?.let {
avatarFileBody = MultipartBody.Part.createFormData("avatar", randomAlphanumericString(12), it.asRequestBody("image/png".toMediaTypeOrNull()))
}
var headerFileBody: MultipartBody.Part? = null
diff.headerFile?.let {
headerFileBody = MultipartBody.Part.createFormData("header", randomAlphanumericString(12), it.asRequestBody("image/png".toMediaTypeOrNull()))
}
mastodonApi.accountUpdateCredentials( mastodonApi.accountUpdateCredentials(
displayName, note, locked, avatar, header, diff.displayName?.toRequestBody(MultipartBody.FORM),
field1?.first, field1?.second, field2?.first, field2?.second, field3?.first, field3?.second, field4?.first, field4?.second diff.note?.toRequestBody(MultipartBody.FORM),
diff.locked?.toString()?.toRequestBody(MultipartBody.FORM),
avatarFileBody,
headerFileBody,
diff.field1?.first?.toRequestBody(MultipartBody.FORM),
diff.field1?.second?.toRequestBody(MultipartBody.FORM),
diff.field2?.first?.toRequestBody(MultipartBody.FORM),
diff.field2?.second?.toRequestBody(MultipartBody.FORM),
diff.field3?.first?.toRequestBody(MultipartBody.FORM),
diff.field3?.second?.toRequestBody(MultipartBody.FORM),
diff.field4?.first?.toRequestBody(MultipartBody.FORM),
diff.field4?.second?.toRequestBody(MultipartBody.FORM)
).fold( ).fold(
{ newProfileData -> { newAccountData ->
saveData.postValue(Success()) saveData.postValue(Success())
eventHub.dispatch(ProfileEditedEvent(newProfileData)) eventHub.dispatch(ProfileEditedEvent(newAccountData))
}, },
{ throwable -> { throwable ->
saveData.postValue(Error(errorMessage = throwable.getServerErrorMessage())) saveData.postValue(Error(errorMessage = throwable.getServerErrorMessage()))
@ -167,30 +154,95 @@ class EditProfileViewModel @Inject constructor(
} }
// cache activity state for rotation change // cache activity state for rotation change
fun updateProfile(newDisplayName: String, newNote: String, newLocked: Boolean, newFields: List<StringField>) { internal fun updateProfile(newProfileData: ProfileDataInUi) {
if (profileData.value is Success) { if (profileData.value is Success) {
val newProfileSource = profileData.value?.data?.source?.copy(note = newNote, fields = newFields) val newProfileSource = profileData.value?.data?.source?.copy(note = newProfileData.note, fields = newProfileData.fields)
val newProfile = profileData.value?.data?.copy( val newProfile = profileData.value?.data?.copy(
displayName = newDisplayName, displayName = newProfileData.displayName,
locked = newLocked, locked = newProfileData.locked,
source = newProfileSource source = newProfileSource
) )
profileData.postValue(Success(newProfile)) profileData.value = Success(newProfile)
} }
} }
private fun calculateFieldToUpdate(newField: StringField?, fieldsUnchanged: Boolean): Pair<RequestBody, RequestBody>? { internal fun hasUnsavedChanges(newProfileData: ProfileDataInUi): Boolean {
val diff = getProfileDiff(apiProfileAccount, newProfileData)
return diff.hasChanges()
}
private fun getProfileDiff(oldProfileAccount: Account?, newProfileData: ProfileDataInUi): DiffProfileData {
val displayName = if (oldProfileAccount?.displayName == newProfileData.displayName) {
null
} else {
newProfileData.displayName
}
val note = if (oldProfileAccount?.source?.note == newProfileData.note) {
null
} else {
newProfileData.note
}
val locked = if (oldProfileAccount?.locked == newProfileData.locked) {
null
} else {
newProfileData.locked
}
val avatarFile = if (avatarData.value != null) {
getCacheFileForName(AVATAR_FILE_NAME)
} else {
null
}
val headerFile = if (headerData.value != null) {
getCacheFileForName(HEADER_FILE_NAME)
} else {
null
}
// when one field changed, all have to be sent or they unchanged ones would get overridden
val allFieldsUnchanged = oldProfileAccount?.source?.fields == newProfileData.fields
val field1 = calculateFieldToUpdate(newProfileData.fields.getOrNull(0), allFieldsUnchanged)
val field2 = calculateFieldToUpdate(newProfileData.fields.getOrNull(1), allFieldsUnchanged)
val field3 = calculateFieldToUpdate(newProfileData.fields.getOrNull(2), allFieldsUnchanged)
val field4 = calculateFieldToUpdate(newProfileData.fields.getOrNull(3), allFieldsUnchanged)
return DiffProfileData(
displayName, note, locked, field1, field2, field3, field4, headerFile, avatarFile
)
}
private fun calculateFieldToUpdate(newField: StringField?, fieldsUnchanged: Boolean): Pair<String, String>? {
if (fieldsUnchanged || newField == null) { if (fieldsUnchanged || newField == null) {
return null return null
} }
return Pair( return Pair(
newField.name.toRequestBody(MultipartBody.FORM), newField.name,
newField.value.toRequestBody(MultipartBody.FORM) newField.value
) )
} }
private fun getCacheFileForName(filename: String): File { private fun getCacheFileForName(filename: String): File {
return File(application.cacheDir, filename) return File(application.cacheDir, filename)
} }
private data class DiffProfileData(
val displayName: String?,
val note: String?,
val locked: Boolean?,
val field1: Pair<String, String>?,
val field2: Pair<String, String>?,
val field3: Pair<String, String>?,
val field4: Pair<String, String>?,
val headerFile: File?,
val avatarFile: File?
) {
fun hasChanges() = displayName != null || note != null || locked != null ||
avatarFile != null || headerFile != null || field1 != null || field2 != null ||
field3 != null || field4 != null
}
} }

View File

@ -819,4 +819,5 @@
<string name="error_media_playback">Playback failed: %s</string> <string name="error_media_playback">Playback failed: %s</string>
<string name="dialog_delete_filter_text">Delete filter \'%1$s\'?"</string> <string name="dialog_delete_filter_text">Delete filter \'%1$s\'?"</string>
<string name="dialog_delete_filter_positive_action">Delete</string> <string name="dialog_delete_filter_positive_action">Delete</string>
<string name="dialog_save_profile_changes_message">Do you want to save your profile changes?</string>
</resources> </resources>