From e7f568e1628fbdd83bc92b0089803684262cb8ce Mon Sep 17 00:00:00 2001 From: Alan Evans Date: Fri, 24 Jan 2020 10:42:58 -0500 Subject: [PATCH] Trimming profile names to fit byte budget and remove leading/trailing spaces. --- .../securesms/profiles/ProfileName.java | 25 ++++-- .../profiles/edit/EditProfileFragment.java | 80 +++++++++++-------- .../profiles/edit/EditProfileViewModel.java | 6 +- .../res/layout/profile_create_fragment.xml | 4 +- app/src/main/res/values/integers.xml | 2 - .../securesms/profiles/ProfileNameTest.java | 19 ++++- 6 files changed, 84 insertions(+), 52 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/profiles/ProfileName.java b/app/src/main/java/org/thoughtcrime/securesms/profiles/ProfileName.java index e3248e7cb8c..5dc2e39d464 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/profiles/ProfileName.java +++ b/app/src/main/java/org/thoughtcrime/securesms/profiles/ProfileName.java @@ -5,6 +5,7 @@ import androidx.annotation.NonNull; import androidx.annotation.Nullable; +import androidx.annotation.VisibleForTesting; import com.annimon.stream.Stream; @@ -24,8 +25,8 @@ public final class ProfileName implements Parcelable { private final String joinedName; private ProfileName(@Nullable String givenName, @Nullable String familyName) { - this.givenName = sanitize(givenName); - this.familyName = sanitize(familyName); + this.givenName = givenName == null ? "" : givenName; + this.familyName = familyName == null ? "" : familyName; this.joinedName = getJoinedName(this.givenName, this.familyName); } @@ -43,7 +44,8 @@ String getFamilyName() { return familyName; } - public boolean isProfileNameCJKV() { + @VisibleForTesting + boolean isProfileNameCJKV() { return isCJKV(givenName, familyName); } @@ -51,8 +53,12 @@ public boolean isEmpty() { return joinedName.isEmpty(); } + public boolean isGivenNameEmpty() { + return givenName.isEmpty(); + } + public @NonNull String serialize() { - if (isEmpty()) { + if (isGivenNameEmpty()) { return ""; } @@ -87,12 +93,19 @@ public boolean isEmpty() { * Creates a profile name, trimming chars until it fits the limits. */ public static @NonNull ProfileName fromParts(@Nullable String givenName, @Nullable String familyName) { - if (givenName == null || givenName.isEmpty()) return EMPTY; + givenName = givenName == null ? "" : givenName; + familyName = familyName == null ? "" : familyName; + + givenName = trimToFit(givenName .trim()); + familyName = trimToFit(familyName.trim()); return new ProfileName(givenName, familyName); } - private static @NonNull String sanitize(@Nullable String name) { + /** + * Trims a name string to fit into the byte length requirement. + */ + public static @NonNull String trimToFit(@Nullable String name) { if (name == null) return ""; // At least one byte per char, so shorten string to reduce loop diff --git a/app/src/main/java/org/thoughtcrime/securesms/profiles/edit/EditProfileFragment.java b/app/src/main/java/org/thoughtcrime/securesms/profiles/edit/EditProfileFragment.java index a2d048aad29..af595f2d633 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/profiles/edit/EditProfileFragment.java +++ b/app/src/main/java/org/thoughtcrime/securesms/profiles/edit/EditProfileFragment.java @@ -2,15 +2,13 @@ import android.Manifest; import android.animation.Animator; -import android.annotation.SuppressLint; import android.app.Activity; import android.content.Context; import android.content.Intent; import android.net.Uri; -import android.os.AsyncTask; import android.os.Build; import android.os.Bundle; -import android.text.Selection; +import android.text.Editable; import android.view.LayoutInflater; import android.view.View; import android.view.ViewAnimationUtils; @@ -26,7 +24,6 @@ import androidx.annotation.RequiresApi; import androidx.annotation.StringRes; import androidx.fragment.app.Fragment; -import androidx.lifecycle.Observer; import androidx.lifecycle.ViewModelProviders; import androidx.navigation.NavDirections; import androidx.navigation.Navigation; @@ -45,6 +42,7 @@ import org.thoughtcrime.securesms.util.BitmapDecodingException; import org.thoughtcrime.securesms.util.BitmapUtil; import org.thoughtcrime.securesms.util.FeatureFlags; +import org.thoughtcrime.securesms.util.concurrent.SimpleTask; import org.thoughtcrime.securesms.util.text.AfterTextChanged; import org.whispersystems.libsignal.util.guava.Optional; @@ -70,7 +68,7 @@ public class EditProfileFragment extends Fragment { private TextView username; private Intent nextIntent; - private File captureFile; + private File captureFile; private EditProfileViewModel viewModel; @@ -113,7 +111,7 @@ public View onCreateView(@NonNull LayoutInflater inflater, @Nullable ViewGroup c @Override public void onViewCreated(@NonNull View view, @Nullable Bundle savedInstanceState) { initializeResources(view); - initializeViewModel(getArguments().getBoolean(EXCLUDE_SYSTEM, false)); + initializeViewModel(requireArguments().getBoolean(EXCLUDE_SYSTEM, false)); initializeProfileName(); initializeProfileAvatar(); initializeUsername(); @@ -122,7 +120,7 @@ public void onViewCreated(@NonNull View view, @Nullable Bundle savedInstanceStat } @Override - public void onRequestPermissionsResult(int requestCode, @NonNull String permissions[], @NonNull int[] grantResults) { + public void onRequestPermissionsResult(int requestCode, @NonNull String[] permissions, @NonNull int[] grantResults) { Permissions.onRequestPermissionsResult(this, requestCode, permissions, grantResults); } @@ -151,9 +149,7 @@ public void onActivityResult(int requestCode, int resultCode, Intent data) { break; case AvatarSelection.REQUEST_CODE_CROP_IMAGE: if (resultCode == Activity.RESULT_OK) { - new AsyncTask() { - @Override - protected byte[] doInBackground(Void... params) { + SimpleTask.run(() -> { try { BitmapUtil.ScaleResult result = BitmapUtil.createScaledBytes(requireActivity(), AvatarSelection.getResultUri(data), new ProfileMediaConstraints()); return result.getBitmap(); @@ -161,14 +157,12 @@ protected byte[] doInBackground(Void... params) { Log.w(TAG, e); return null; } - } - - @Override - protected void onPostExecute(byte[] result) { - if (result != null) { - viewModel.setAvatar(result); + }, + (avatarBytes) -> { + if (avatarBytes != null) { + viewModel.setAvatar(avatarBytes); GlideApp.with(EditProfileFragment.this) - .load(result) + .load(avatarBytes) .skipMemoryCache(true) .diskCacheStrategy(DiskCacheStrategy.NONE) .circleCrop() @@ -177,7 +171,7 @@ protected void onPostExecute(byte[] result) { Toast.makeText(requireActivity(), R.string.CreateProfileActivity_error_setting_profile_photo, Toast.LENGTH_LONG).show(); } } - }.execute(); + ); } break; } @@ -191,6 +185,7 @@ private void initializeViewModel(boolean excludeSystem) { } private void initializeResources(@NonNull View view) { + Bundle arguments = requireArguments(); this.avatar = view.findViewById(R.id.avatar); this.givenName = view.findViewById(R.id.given_name); @@ -201,9 +196,9 @@ private void initializeResources(@NonNull View view) { this.username = view.findViewById(R.id.profile_overview_username); this.usernameEditButton = view.findViewById(R.id.profile_overview_username_edit_button); this.usernameLabel = view.findViewById(R.id.profile_overview_username_label); - this.nextIntent = getArguments().getParcelable(NEXT_INTENT); + this.nextIntent = arguments.getParcelable(NEXT_INTENT); - if (FeatureFlags.usernames() && getArguments().getBoolean(DISPLAY_USERNAME, false)) { + if (FeatureFlags.usernames() && arguments.getBoolean(DISPLAY_USERNAME, false)) { username.setVisibility(View.VISIBLE); usernameEditButton.setVisibility(View.VISIBLE); usernameLabel.setVisibility(View.VISIBLE); @@ -215,8 +210,14 @@ private void initializeResources(@NonNull View view) { .onAnyResult(this::startAvatarSelection) .execute()); - this.givenName.addTextChangedListener(new AfterTextChanged(s -> viewModel.setGivenName(s.toString()))); - this.familyName.addTextChangedListener(new AfterTextChanged(s -> viewModel.setFamilyName(s.toString()))); + this.givenName .addTextChangedListener(new AfterTextChanged(s -> { + trimInPlace(s); + viewModel.setGivenName(s.toString()); + })); + this.familyName.addTextChangedListener(new AfterTextChanged(s -> { + trimInPlace(s); + viewModel.setFamilyName(s.toString()); + })); this.finishButton.setOnClickListener(v -> { this.finishButton.setIndeterminateProgressMode(true); @@ -224,7 +225,7 @@ private void initializeResources(@NonNull View view) { handleUpload(); }); - this.finishButton.setText(getArguments().getInt(NEXT_BUTTON_TEXT, R.string.CreateProfileActivity_next)); + this.finishButton.setText(arguments.getInt(NEXT_BUTTON_TEXT, R.string.CreateProfileActivity_next)); this.usernameEditButton.setOnClickListener(v -> { NavDirections action = EditProfileFragmentDirections.actionEditUsername(); @@ -238,8 +239,10 @@ private void initializeProfileName() { updateFieldIfNeeded(givenName, profileName.getGivenName()); updateFieldIfNeeded(familyName, profileName.getFamilyName()); - finishButton.setEnabled(!profileName.isEmpty()); - finishButton.setAlpha(!profileName.isEmpty() ? 1f : 0.5f); + boolean validEntry = !profileName.isGivenNameEmpty(); + + finishButton.setEnabled(validEntry); + finishButton.setAlpha(validEntry ? 1f : 0.5f); preview.setText(profileName.toString()); }); @@ -260,9 +263,11 @@ private void initializeUsername() { viewModel.username().observe(this, this::onUsernameChanged); } - private void updateFieldIfNeeded(@NonNull EditText field, @NonNull String value) { - if (!field.getText().toString().equals(value)) { + private static void updateFieldIfNeeded(@NonNull EditText field, @NonNull String value) { + String fieldTrimmed = field.getText().toString().trim(); + String valueTrimmed = value.trim(); + if (!fieldTrimmed.equals(valueTrimmed)) { boolean setSelectionToEnd = field.getText().length() == 0; field.setText(value); @@ -273,13 +278,8 @@ private void updateFieldIfNeeded(@NonNull EditText field, @NonNull String value) } } - @SuppressLint("SetTextI18n") private void onUsernameChanged(@NonNull Optional username) { - if (username.isPresent()) { - this.username.setText("@" + username.get()); - } else { - this.username.setText(""); - } + this.username.setText(username.transform(s -> "@" + s).or("")); } private void startAvatarSelection() { @@ -287,10 +287,13 @@ private void startAvatarSelection() { } private void handleUpload() { - viewModel.submitProfile(uploadResult -> { if (uploadResult == EditProfileRepository.UploadResult.SUCCESS) { - if (captureFile != null) captureFile.delete(); + if (captureFile != null) { + if (!captureFile.delete()) { + Log.w(TAG, "Failed to delete capture file " + captureFile); + } + } if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) handleFinishedLollipop(); else handleFinishedLegacy(); } else { @@ -345,6 +348,13 @@ public void onAnimationRepeat(Animator animation) {} animation.start(); } + private static void trimInPlace(Editable s) { + int trimmedLength = ProfileName.trimToFit(s.toString()).length(); + if (s.length() > trimmedLength) { + s.delete(trimmedLength, s.length()); + } + } + public interface Controller { void onProfileNameUploadCompleted(); } diff --git a/app/src/main/java/org/thoughtcrime/securesms/profiles/edit/EditProfileViewModel.java b/app/src/main/java/org/thoughtcrime/securesms/profiles/edit/EditProfileViewModel.java index edbdefdb9c2..4a991d91da2 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/profiles/edit/EditProfileViewModel.java +++ b/app/src/main/java/org/thoughtcrime/securesms/profiles/edit/EditProfileViewModel.java @@ -20,7 +20,7 @@ class EditProfileViewModel extends ViewModel { pair -> ProfileName.fromParts(pair.first(), pair.second())); private final MutableLiveData internalAvatar = new MutableLiveData<>(); private final MutableLiveData> internalUsername = new MutableLiveData<>(); - private final EditProfileRepository repository; + private final EditProfileRepository repository; private EditProfileViewModel(@NonNull EditProfileRepository repository) { this.repository = repository; @@ -70,10 +70,6 @@ public void submitProfile(Consumer uploadRes repository.uploadProfile(profileName, internalAvatar.getValue(), uploadResultConsumer); } - private ProfileName currentProfileName() { - return internalProfileName.getValue(); - } - static class Factory implements ViewModelProvider.Factory { private final EditProfileRepository repository; diff --git a/app/src/main/res/layout/profile_create_fragment.xml b/app/src/main/res/layout/profile_create_fragment.xml index 07b47348318..16091603c24 100644 --- a/app/src/main/res/layout/profile_create_fragment.xml +++ b/app/src/main/res/layout/profile_create_fragment.xml @@ -120,9 +120,9 @@ android:layout_marginTop="13dp" android:layout_marginEnd="16dp" android:layout_weight="1" + android:autofillHints="given-name" android:hint="@string/CreateProfileActivity_first_name_required" android:inputType="textPersonName" - android:maxLength="@integer/profile_name_part_max_length" android:singleLine="true" /> diff --git a/app/src/main/res/values/integers.xml b/app/src/main/res/values/integers.xml index 640068bf82b..adb103844f4 100644 --- a/app/src/main/res/values/integers.xml +++ b/app/src/main/res/values/integers.xml @@ -4,6 +4,4 @@ 400 380 10 - - 26 \ No newline at end of file diff --git a/app/src/test/java/org/thoughtcrime/securesms/profiles/ProfileNameTest.java b/app/src/test/java/org/thoughtcrime/securesms/profiles/ProfileNameTest.java index 3e2e6c4b9ae..d2db50fbe4a 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/profiles/ProfileNameTest.java +++ b/app/src/test/java/org/thoughtcrime/securesms/profiles/ProfileNameTest.java @@ -10,7 +10,7 @@ public final class ProfileNameTest { -@Test + @Test public void givenEmpty_thenIExpectSaneDefaults() { // GIVEN ProfileName profileName = ProfileName.EMPTY; @@ -23,7 +23,7 @@ public void givenEmpty_thenIExpectSaneDefaults() { } @Test - public void givenNullProfileName_whenIFromDataString_thenIExpectSaneDefaults() { + public void givenNullProfileName_whenIFromSerialized_thenIExpectExactlyEmpty() { // GIVEN ProfileName profileName = ProfileName.fromSerialized(null); @@ -128,6 +128,7 @@ public void givenProfileNameWithEmptyGivenName_whenIToDataString_thenIExpectAnEm // THEN assertEquals("Blank String should be returned (For back compat)", "", data); + assertEquals("Family", name.getFamilyName()); } @Test @@ -161,4 +162,18 @@ public void fromParts_with_long_name_parts() { assertEquals("GivenSomeVeryLongNameSomeV", name.getGivenName()); assertEquals("FamilySomeVeryLongNameSome", name.getFamilyName()); } + + @Test + public void givenProfileNameWithGivenNameAndFamilyNameWithSpaces_whenIToDataString_thenIExpectTrimmedProfileName() { + // GIVEN + ProfileName name = ProfileName.fromParts(" Given ", " Family"); + + // WHEN + String data = name.serialize(); + + // THEN + assertEquals(data, "Given\0Family"); + assertEquals(name.getGivenName(), "Given"); + assertEquals(name.getFamilyName(), "Family"); + } } \ No newline at end of file