SES2397 - Fix display name change fail feedback (#1544)

* Added check to not update display name if offline or should config sync fail

* Addressed PR feedback

* WIP

* Addressed PR feedback

* Adjusted phrasing of log statement

---------

Co-authored-by: alansley <aclansley@gmail.com>

app/src/main/java/org/thoughtcrime/securesms/components/ProfilePictureView.kt

@@ -17,6 +17,7 @@ import org.session.libsession.messaging.contacts.Contact
 import org.session.libsession.utilities.Address
 import org.session.libsession.utilities.GroupUtil
 import org.session.libsession.utilities.recipients.Recipient
+import org.session.libsignal.utilities.Log
 import org.thoughtcrime.securesms.dependencies.DatabaseComponent
 import org.thoughtcrime.securesms.mms.GlideApp
 import org.thoughtcrime.securesms.mms.GlideRequests
@@ -24,6 +25,8 @@ import org.thoughtcrime.securesms.mms.GlideRequests
 class ProfilePictureView @JvmOverloads constructor(
     context: Context, attrs: AttributeSet? = null
 ) : RelativeLayout(context, attrs) {
+    private val TAG = "ProfilePictureView"
+
     private val binding = ViewProfilePictureBinding.inflate(LayoutInflater.from(context), this)
     private val glide: GlideRequests = GlideApp.with(this)
     var publicKey: String? = null
@@ -85,7 +88,7 @@ class ProfilePictureView @JvmOverloads constructor(
     }
 
     fun update() {
-        val publicKey = publicKey ?: return
+        val publicKey = publicKey ?: return Log.w(TAG, "Could not find public key to update profile picture")
         val additionalPublicKey = additionalPublicKey
         if (additionalPublicKey != null) {
             setProfilePictureIfNeeded(binding.doubleModeImageView1, publicKey, displayName)

app/src/main/java/org/thoughtcrime/securesms/preferences/SettingsActivity.kt

@@ -56,6 +56,7 @@ import org.thoughtcrime.securesms.showSessionDialog
 import org.thoughtcrime.securesms.util.BitmapDecodingException
 import org.thoughtcrime.securesms.util.BitmapUtil
 import org.thoughtcrime.securesms.util.ConfigurationMessageUtilities
+import org.thoughtcrime.securesms.util.NetworkUtils
 import org.thoughtcrime.securesms.util.disableClipping
 import org.thoughtcrime.securesms.util.push
 import org.thoughtcrime.securesms.util.show
@@ -179,7 +180,7 @@ class SettingsActivity : PassphraseRequiredActionBarActivity() {
                     try {
                         val profilePictureToBeUploaded = BitmapUtil.createScaledBytes(this@SettingsActivity, AvatarSelection.getResultUri(data), ProfileMediaConstraints()).bitmap
                         Handler(Looper.getMainLooper()).post {
-                            updateProfile(true, profilePictureToBeUploaded)
+                            updateProfilePicture(profilePictureToBeUploaded)
                         }
                     } catch (e: BitmapDecodingException) {
                         e.printStackTrace()
@@ -228,56 +229,59 @@ class SettingsActivity : PassphraseRequiredActionBarActivity() {
         }
     }
 
-    private fun updateProfile(
+    private fun updateDisplayName(displayName: String): Boolean {
-        isUpdatingProfilePicture: Boolean,
-        profilePicture: ByteArray? = null,
-        displayName: String? = null
-    ) {
         binding.loader.isVisible = true
 
-        if (displayName != null) {
+        // We'll assume we fail & flip the flag on success
-            TextSecurePreferences.setProfileName(this, displayName)
+        var updateWasSuccessful = false
-            configFactory.user?.setName(displayName)
-        }
 
-        // Bail if we're not updating the profile picture in any way
+        val haveNetworkConnection = NetworkUtils.haveValidNetworkConnection(this@SettingsActivity);
-        if (!isUpdatingProfilePicture) return
+        if (!haveNetworkConnection) {
-
+            Log.w(TAG, "Cannot update display name - no network connection.")
-        val encodedProfileKey = ProfileKeyUtil.generateEncodedProfileKey(this)
-
-        val uploadProfilePicturePromise: Promise<*, Exception>
-        var removingProfilePic = false
-
-        // Adding a new profile picture?
-        if (profilePicture != null) {
-            uploadProfilePicturePromise = ProfilePictureUtilities.upload(profilePicture, encodedProfileKey, this)
         } else {
-            // If not then we must be removing the existing one.
+            // if we have a network connection then attempt to update the display name
-            // Note: To get a promise that will resolve / sync correctly we overwrite the existing profile picture with
+            TextSecurePreferences.setProfileName(this, displayName)
-            // a 0 byte image.
+            val user = configFactory.user
-            removingProfilePic = true
+            if (user == null) {
-            val emptyByteArray = ByteArray(0)
+                Log.w(TAG, "Cannot update display name - missing user details from configFactory.")
-            uploadProfilePicturePromise = ProfilePictureUtilities.upload(emptyByteArray, encodedProfileKey, this)
+            } else {
+                user.setName(displayName)
+                binding.btnGroupNameDisplay.text = displayName
+                updateWasSuccessful = true
+            }
         }
 
-        // If the upload picture promise succeeded then we hit this successUi block
+        // Inform the user if we failed to update the display name
-        uploadProfilePicturePromise.successUi {
+        if (!updateWasSuccessful) {
+            Toast.makeText(this@SettingsActivity, R.string.profileErrorUpdate, Toast.LENGTH_LONG).show()
+        }
 
-            // If we successfully removed the profile picture on the network then we can clear the
+        binding.loader.isVisible = false
-            // local data - otherwise it's weird to fail the online section but it _looks_ like it
+        return updateWasSuccessful
-            // worked because we cleared the local image (also it denies them the chance to retry
+    }
-            // removal if we do it locally, and may result in them having a visible profile picture
+
-            // everywhere EXCEPT on their own device!).
+    // Helper method used by updateProfilePicture and removeProfilePicture to sync it online
-            if (removingProfilePic) {
+    private fun syncProfilePicture(profilePicture: ByteArray, onFail: () -> Unit) {
+        binding.loader.isVisible = true
+
+        // Grab the profile key and kick of the promise to update the profile picture
+        val encodedProfileKey = ProfileKeyUtil.generateEncodedProfileKey(this)
+        val updateProfilePicturePromise = ProfilePictureUtilities.upload(profilePicture, encodedProfileKey, this)
+
+        // If the online portion of the update succeeded then update the local state
+        updateProfilePicturePromise.successUi {
+
+            // When removing the profile picture the supplied ByteArray is empty so we'll clear the local data
+            if (profilePicture.isEmpty()) {
                 MessagingModuleConfiguration.shared.storage.clearUserPic()
             }
 
             val userConfig = configFactory.user
             AvatarHelper.setAvatar(this, Address.fromSerialized(TextSecurePreferences.getLocalNumber(this)!!), profilePicture)
-            TextSecurePreferences.setProfileAvatarId(this, profilePicture?.let { SecureRandom().nextInt() } ?: 0 )
+            TextSecurePreferences.setProfileAvatarId(this, profilePicture.let { SecureRandom().nextInt() } )
             ProfileKeyUtil.setEncodedProfileKey(this, encodedProfileKey)
 
-            // new config
+            // Attempt to grab the details we require to update the profile picture
             val url = TextSecurePreferences.getProfilePictureURL(this)
             val profileKey = ProfileKeyUtil.getProfileKey(this)
 
@@ -291,30 +295,52 @@ class SettingsActivity : PassphraseRequiredActionBarActivity() {
             }
 
             ConfigurationMessageUtilities.forceSyncConfigurationNowIfNeeded(this@SettingsActivity)
+
+            // Update our visuals
+            binding.profilePictureView.recycle()
+            binding.profilePictureView.update()
         }
 
-        // Or if the promise failed to upload the new profile picture then we hit this failUi block
+        // If the sync failed then inform the user
-        uploadProfilePicturePromise.failUi {
+        updateProfilePicturePromise.failUi { onFail() }
-            if (removingProfilePic) {
+
-                Log.e(TAG, "Failed to remove profile picture")
+        // Finally, remove the loader animation after we've waited for the attempt to succeed or fail
-                Toast.makeText(this@SettingsActivity, R.string.profileDisplayPictureRemoveError, Toast.LENGTH_LONG).show()
+        updateProfilePicturePromise.alwaysUi { binding.loader.isVisible = false }
-            } else {
+    }
-                Log.e(TAG, "Failed to upload profile picture")
+
-                Toast.makeText(this@SettingsActivity, R.string.profileErrorUpdate, Toast.LENGTH_LONG).show()
+    private fun updateProfilePicture(profilePicture: ByteArray) {
-            }
+
+        val haveNetworkConnection = NetworkUtils.haveValidNetworkConnection(this@SettingsActivity);
+        if (!haveNetworkConnection) {
+            Log.w(TAG, "Cannot update profile picture - no network connection.")
+            Toast.makeText(this@SettingsActivity, R.string.profileErrorUpdate, Toast.LENGTH_LONG).show()
+            return
         }
 
-        // Finally, regardless of whether the promise succeeded or failed, we always hit this `alwaysUi` block
+        val onFail: () -> Unit = {
-        uploadProfilePicturePromise.alwaysUi {
+            Log.e(TAG, "Sync failed when uploading profile picture.")
-            if (displayName != null) {
+            Toast.makeText(this@SettingsActivity, R.string.profileErrorUpdate, Toast.LENGTH_LONG).show()
-                binding.btnGroupNameDisplay.text = displayName
-            }
-            if (isUpdatingProfilePicture) {
-                binding.profilePictureView.recycle() // Clear the cached image before updating
-                binding.profilePictureView.update()
-            }
-            binding.loader.isVisible = false
         }
+
+        syncProfilePicture(profilePicture, onFail)
+    }
+
+    private fun removeProfilePicture() {
+
+        val haveNetworkConnection = NetworkUtils.haveValidNetworkConnection(this@SettingsActivity);
+        if (!haveNetworkConnection) {
+            Log.w(TAG, "Cannot remove profile picture - no network connection.")
+            Toast.makeText(this@SettingsActivity, R.string.profileDisplayPictureRemoveError, Toast.LENGTH_LONG).show()
+            return
+        }
+
+        val onFail: () -> Unit = {
+            Log.e(TAG, "Sync failed when removing profile picture.")
+            Toast.makeText(this@SettingsActivity, R.string.profileDisplayPictureRemoveError, Toast.LENGTH_LONG).show()
+        }
+
+        val emptyProfilePicture = ByteArray(0)
+        syncProfilePicture(emptyProfilePicture, onFail)
     }
     // endregion
 
@@ -333,8 +359,7 @@ class SettingsActivity : PassphraseRequiredActionBarActivity() {
             Toast.makeText(this, R.string.activity_settings_display_name_too_long_error, Toast.LENGTH_SHORT).show()
             return false
         }
-        updateProfile(false, displayName = displayName)
+        return updateDisplayName(displayName)
-        return true
     }
 
     private fun showQRCode() {
@@ -348,7 +373,7 @@ class SettingsActivity : PassphraseRequiredActionBarActivity() {
             view(R.layout.dialog_change_avatar)
             button(R.string.activity_settings_upload) { startAvatarSelection() }
             if (TextSecurePreferences.getProfileAvatarId(context) != 0) {
-                button(R.string.activity_settings_remove) { removeAvatar() }
+                button(R.string.activity_settings_remove) { removeProfilePicture() }
             }
             cancelButton()
         }.apply {
@@ -366,10 +391,6 @@ class SettingsActivity : PassphraseRequiredActionBarActivity() {
             }
     }
 
-    private fun removeAvatar() {
-        updateProfile(true)
-    }
-
     private fun startAvatarSelection() {
         // Ask for an optional camera permission.
         Permissions.with(this)