Skip to content

Commit

Permalink
Improve handling of partially bi-directional text.
Browse files Browse the repository at this point in the history
  • Loading branch information
@greyson-signal
greyson-signal committed Jul 30, 2020
1 parent e504ffa commit 8ed7fc8
Show file tree
Hide file tree
Showing 7 changed files with 108 additions and 24 deletions.
5 changes: 3 additions & 2 deletions ...rc/main/java/org/thoughtcrime/securesms/database/model/GroupsV2UpdateMessageProducer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import org.thoughtcrime.securesms.R;
import org.thoughtcrime.securesms.groups.GV2AccessLevelUtil;
import org.thoughtcrime.securesms.util.ExpirationUtil;
import org.thoughtcrime.securesms.util.StringUtil;
import org.whispersystems.libsignal.util.guava.Optional;
import org.whispersystems.signalservice.api.groupsv2.DecryptedGroupUtil;
import org.whispersystems.signalservice.api.util.UuidUtil;
Expand Down Expand Up @@ -396,7 +397,7 @@ private void describeNewTitle(@NonNull DecryptedGroupChange change, @NonNull Lis
boolean editorIsYou = change.getEditor().equals(selfUuidBytes);

if (change.hasNewTitle()) {
String newTitle = change.getNewTitle().getValue();
String newTitle = StringUtil.isolateBidi(change.getNewTitle().getValue());
if (editorIsYou) {
updates.add(updateDescription(context.getString(R.string.MessageRecord_you_changed_the_group_name_to_s, newTitle)));
} else {
Expand All @@ -407,7 +408,7 @@ private void describeNewTitle(@NonNull DecryptedGroupChange change, @NonNull Lis

private void describeUnknownEditorNewTitle(@NonNull DecryptedGroupChange change, @NonNull List<UpdateDescription> updates) {
if (change.hasNewTitle()) {
updates.add(updateDescription(context.getString(R.string.MessageRecord_the_group_name_has_changed_to_s, change.getNewTitle().getValue())));
updates.add(updateDescription(context.getString(R.string.MessageRecord_the_group_name_has_changed_to_s, StringUtil.isolateBidi(change.getNewTitle().getValue()))));
}
}

Expand Down
5 changes: 3 additions & 2 deletions app/src/main/java/org/thoughtcrime/securesms/database/model/MessageRecord.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import org.thoughtcrime.securesms.util.Base64;
import org.thoughtcrime.securesms.util.ExpirationUtil;
import org.thoughtcrime.securesms.util.GroupUtil;
import org.thoughtcrime.securesms.util.StringUtil;
import org.whispersystems.libsignal.util.guava.Function;
import org.whispersystems.signalservice.api.util.UuidUtil;

Expand Down Expand Up @@ -196,8 +197,8 @@ public SpannableString getDisplayBody(@NonNull Context context) {

if (profileChangeDetails.hasProfileNameChange()) {
String displayName = getIndividualRecipient().getDisplayName(context);
String newName = ProfileName.fromSerialized(profileChangeDetails.getProfileNameChange().getNew()).toString();
String previousName = ProfileName.fromSerialized(profileChangeDetails.getProfileNameChange().getPrevious()).toString();
String newName = StringUtil.isolateBidi(ProfileName.fromSerialized(profileChangeDetails.getProfileNameChange().getNew()).toString());
String previousName = StringUtil.isolateBidi(ProfileName.fromSerialized(profileChangeDetails.getProfileNameChange().getPrevious()).toString());

if (getIndividualRecipient().isSystemContact()) {
return context.getString(R.string.MessageRecord_changed_their_profile_name_from_to, displayName, previousName, newName);
Expand Down
4 changes: 2 additions & 2 deletions app/src/main/java/org/thoughtcrime/securesms/messagerequests/MessageRequestsBottomView.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ public void setRecipient(@NonNull Recipient recipient) {
if (recipient.isGroup()) {
question.setText(R.string.MessageRequestBottomView_unblock_this_group_and_share_your_name_and_photo_with_its_members);
} else {
String name = recipient.getProfileName().isEmpty() ? recipient.getDisplayName(getContext()) : recipient.getProfileName().getGivenName();
String name = recipient.getShortDisplayName(getContext());
question.setText(HtmlCompat.fromHtml(getContext().getString(R.string.MessageRequestBottomView_do_you_want_to_let_s_message_you_wont_receive_any_messages_until_you_unblock_them, HtmlUtil.bold(name)), 0));
}
setActiveInactiveGroups(blockedButtons, normalButtons);
Expand All @@ -77,7 +77,7 @@ public void setRecipient(@NonNull Recipient recipient) {
question.setText(R.string.MessageRequestBottomView_do_you_want_to_join_this_group_they_wont_know_youve_seen_their_messages_until_you_accept);
}
} else {
String name = recipient.getProfileName().isEmpty() ? recipient.getDisplayName(getContext()) : recipient.getProfileName().getGivenName();
String name = recipient.getShortDisplayName(getContext());
question.setText(HtmlCompat.fromHtml(getContext().getString(R.string.MessageRequestBottomView_do_you_want_to_let_s_message_you_they_wont_know_youve_seen_their_messages_until_you_accept, HtmlUtil.bold(name)), 0));
}
setActiveInactiveGroups(normalButtons, blockedButtons);
Expand Down
25 changes: 14 additions & 11 deletions app/src/main/java/org/thoughtcrime/securesms/recipients/Recipient.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,19 +26,18 @@
import org.thoughtcrime.securesms.database.DatabaseFactory;
import org.thoughtcrime.securesms.database.IdentityDatabase.VerifiedStatus;
import org.thoughtcrime.securesms.database.RecipientDatabase;
import org.thoughtcrime.securesms.database.RecipientDatabase.RecipientIdResult;
import org.thoughtcrime.securesms.database.RecipientDatabase.RegisteredState;
import org.thoughtcrime.securesms.database.RecipientDatabase.UnidentifiedAccessMode;
import org.thoughtcrime.securesms.database.RecipientDatabase.VibrateState;
import org.thoughtcrime.securesms.dependencies.ApplicationDependencies;
import org.thoughtcrime.securesms.groups.GroupId;
import org.thoughtcrime.securesms.jobs.DirectoryRefreshJob;
import org.thoughtcrime.securesms.logging.Log;
import org.thoughtcrime.securesms.notifications.NotificationChannels;
import org.thoughtcrime.securesms.phonenumbers.NumberUtil;
import org.thoughtcrime.securesms.phonenumbers.PhoneNumberFormatter;
import org.thoughtcrime.securesms.profiles.ProfileName;
import org.thoughtcrime.securesms.util.FeatureFlags;
import org.thoughtcrime.securesms.util.StringUtil;
import org.thoughtcrime.securesms.util.Util;
import org.thoughtcrime.securesms.util.concurrent.SignalExecutors;
import org.whispersystems.libsignal.util.guava.Optional;
Expand Down Expand Up @@ -400,18 +399,22 @@ public boolean hasAUserSetDisplayName(@NonNull Context context) {
}

public @NonNull String getDisplayName(@NonNull Context context) {
return Util.getFirstNonEmpty(getName(context),
getProfileName().toString(),
getDisplayUsername(),
e164,
email,
context.getString(R.string.Recipient_unknown));
String name = Util.getFirstNonEmpty(getName(context),
getProfileName().toString(),
getDisplayUsername(),
e164,
email,
context.getString(R.string.Recipient_unknown));

return StringUtil.isolateBidi(name);
}

public @NonNull String getShortDisplayName(@NonNull Context context) {
return Util.getFirstNonEmpty(getName(context),
getProfileName().getGivenName(),
getDisplayName(context));
String name = Util.getFirstNonEmpty(getName(context),
getProfileName().getGivenName(),
getDisplayName(context));

return StringUtil.isolateBidi(name);
}

public @NonNull MaterialColor getColor() {
Expand Down
2 changes: 1 addition & 1 deletion app/src/main/java/org/thoughtcrime/securesms/util/GroupUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ public String toString(@NonNull Recipient sender) {
return description.toString();
}

String title = groupContext.getName();
String title = StringUtil.isolateBidi(groupContext.getName());

if (members != null && members.size() > 0) {
description.append("\n");
Expand Down
77 changes: 77 additions & 0 deletions app/src/main/java/org/thoughtcrime/securesms/util/StringUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@

import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
import androidx.core.text.BidiFormatter;

import com.google.android.collect.Sets;

import java.nio.charset.StandardCharsets;
import java.util.Collections;
import java.util.Set;

public final class StringUtil {
Expand All @@ -14,6 +16,27 @@ public final class StringUtil {
'\u200F', // right-to-left mark
'\u2007'); // figure space

private static final class Bidi {
/** Override text direction */
private static final Set<Integer> OVERRIDES = Sets.newHashSet("\u202a".codePointAt(0), /* LRE */
"\u202b".codePointAt(0), /* RLE */
"\u202d".codePointAt(0), /* LRO */
"\u202e".codePointAt(0) /* RLO */);

/** Set direction and isolate surrounding text */
private static final Set<Integer> ISOLATES = Sets.newHashSet("\u2066".codePointAt(0), /* LRI */
"\u2067".codePointAt(0), /* RLI */
"\u2068".codePointAt(0) /* FSI */);
/** Closes things in {@link #OVERRIDES} */
private static final int PDF = "\u202c".codePointAt(0);

/** Closes things in {@link #ISOLATES} */
private static final int PDI = "\u2069".codePointAt(0);

/** Auto-detecting isolate */
private static final int FSI = "\u2068".codePointAt(0);
}

private StringUtil() {
}

Expand Down Expand Up @@ -99,4 +122,58 @@ public static boolean isVisuallyEmpty(char c) {
public static @NonNull String codePointToString(int codePoint) {
return new String(Character.toChars(codePoint));
}

/**
* Isolates bi-directional text from influencing surrounding text. You should use this whenever
* you're injecting user-generated text into a larger string.
*
* You'd think we'd be able to trust {@link BidiFormatter}, but unfortunately it just misses some
* corner cases, so here we are.
*
* The general idea is just to balance out the opening and closing codepoints, and then wrap the
* whole thing in FSI/PDI to isolate it.
*
* For more details, see:
* https://www.w3.org/International/questions/qa-bidi-unicode-controls
*/
public static @NonNull String isolateBidi(@NonNull String text) {
int overrideCount = 0;
int overrideCloseCount = 0;
int isolateCount = 0;
int isolateCloseCount = 0;

for (int i = 0, len = text.codePointCount(0, text.length()); i < len; i++) {
int codePoint = text.codePointAt(i);

if (Bidi.OVERRIDES.contains(codePoint)) {
overrideCount++;
} else if (codePoint == Bidi.PDF) {
overrideCloseCount++;
} else if (Bidi.ISOLATES.contains(codePoint)) {
isolateCount++;
} else if (codePoint == Bidi.PDI) {
isolateCloseCount++;
}
}

StringBuilder suffix = new StringBuilder();

while (overrideCount > overrideCloseCount) {
suffix.appendCodePoint(Bidi.PDF);
overrideCloseCount++;
}

while (isolateCount > isolateCloseCount) {
suffix.appendCodePoint(Bidi.FSI);
isolateCloseCount++;
}

StringBuilder out = new StringBuilder();

return out.appendCodePoint(Bidi.FSI)
.append(text)
.append(suffix)
.appendCodePoint(Bidi.PDI)
.toString();
}
}
14 changes: 8 additions & 6 deletions ...est/java/org/thoughtcrime/securesms/database/model/GroupsV2UpdateMessageProducerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import org.signal.storageservice.protos.groups.local.DecryptedString;
import org.signal.storageservice.protos.groups.local.DecryptedTimer;
import org.thoughtcrime.securesms.testutil.MainThreadUtil;
import org.thoughtcrime.securesms.util.StringUtil;
import org.thoughtcrime.securesms.util.Util;
import org.whispersystems.signalservice.api.util.UuidUtil;

Expand All @@ -43,6 +44,7 @@
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;
import static org.thoughtcrime.securesms.util.StringUtil.isolateBidi;

@RunWith(RobolectricTestRunner.class)
@Config(manifest = Config.NONE, application = Application.class)
Expand Down Expand Up @@ -603,7 +605,7 @@ public void member_changes_title() {
.title("New title")
.build();

assertThat(describeChange(change), is(singletonList("Alice changed the group name to \"New title\".")));
assertThat(describeChange(change), is(singletonList("Alice changed the group name to \"" + isolateBidi("New title") + "\".")));
}

@Test
Expand All @@ -612,7 +614,7 @@ public void you_change_title() {
.title("Title 2")
.build();

assertThat(describeChange(change), is(singletonList("You changed the group name to \"Title 2\".")));
assertThat(describeChange(change), is(singletonList("You changed the group name to \"" + isolateBidi("Title 2") + "\".")));
}

@Test
Expand All @@ -621,7 +623,7 @@ public void unknown_changed_title() {
.title("Title 3")
.build();

assertThat(describeChange(change), is(singletonList("The group name has changed to \"Title 3\".")));
assertThat(describeChange(change), is(singletonList("The group name has changed to \"" + isolateBidi("Title 3") + "\".")));
}

// Avatar change
Expand Down Expand Up @@ -762,7 +764,7 @@ public void multiple_changes() {

assertThat(describeChange(change), is(Arrays.asList(
"Alice added Bob.",
"Alice changed the group name to \"Title\".",
"Alice changed the group name to \"" + isolateBidi("Title") + "\".",
"Alice set the disappearing message timer to 5 minutes.",
"Alice changed who can edit group membership to \"All members\".")));
}
Expand Down Expand Up @@ -803,7 +805,7 @@ public void multiple_changes_by_unknown() {

assertThat(describeChange(change), is(Arrays.asList(
"Bob joined the group.",
"The group name has changed to \"Title 2\".",
"The group name has changed to \"" + isolateBidi("Title 2") + "\".",
"The group avatar has been changed.",
"The disappearing message timer has been set to 10 minutes.",
"Who can edit group membership has been changed to \"All members\".")));
Expand All @@ -821,7 +823,7 @@ public void multiple_changes_join_and_leave_by_unknown() {
assertThat(describeChange(change), is(Arrays.asList(
"Alice joined the group.",
"Alice is now an admin.",
"The group name has changed to \"Updated title\".",
"The group name has changed to \"" + isolateBidi("Updated title") + "\".",
"Alice is no longer in the group.")));
}

Expand Down

0 comments on commit 8ed7fc8

Please sign in to comment.