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

Improve range split function and add unit tests #553

Open
wants to merge 7 commits into
base: @Skalakid/fix-emoji-formatting
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
2 changes: 2 additions & 0 deletions android/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,8 @@ dependencies {
implementation "com.facebook.react:react-android" // version substituted by RNGP
implementation "com.facebook.react:hermes-android" // version substituted by RNGP
implementation project(":react-native-reanimated")

testImplementation "junit:junit:4.13.2"
}

if (isNewArchitectureEnabled()) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package com.expensify.livemarkdown;

import static com.expensify.livemarkdown.RangeSplitter.splitRangesOnEmojis;

import androidx.annotation.NonNull;

import com.facebook.react.bridge.ReactContext;
Expand Down Expand Up @@ -32,38 +34,6 @@ public MarkdownParser(@NonNull ReactContext reactContext) {

private native String nativeParse(@NonNull String text, int parserId);

private void splitRangesOnEmojis(List<MarkdownRange> markdownRanges, String type) {
List<MarkdownRange> emojiRanges = new ArrayList<>();
for (MarkdownRange range : markdownRanges) {
if (range.getType().equals("emoji")) {
emojiRanges.add(range);
}
}

int i = 0;
int j = 0;
while (i < markdownRanges.size() && j < emojiRanges.size()) {
MarkdownRange currentRange = markdownRanges.get(i);
MarkdownRange emojiRange = emojiRanges.get(j);

if (!currentRange.getType().equals(type) || currentRange.getEnd() < emojiRange.getStart()) {
i += 1;
continue;
} else if (emojiRange.getStart() >= currentRange.getStart() && emojiRange.getEnd() <= currentRange.getEnd()) {
// Split range
MarkdownRange startRange = new MarkdownRange(currentRange.getType(), currentRange.getStart(), emojiRange.getStart() - currentRange.getStart(), currentRange.getDepth());
MarkdownRange endRange = new MarkdownRange(currentRange.getType(), emojiRange.getEnd(), currentRange.getEnd() - emojiRange.getEnd(), currentRange.getDepth());

markdownRanges.add(i + 1, startRange);
markdownRanges.add(i + 2, endRange);
markdownRanges.remove(i);
i = i + 1;
}
j += 1;
}
}


private List<MarkdownRange> parseRanges(String rangesJSON, String innerText) {
List<MarkdownRange> markdownRanges = new ArrayList<>();
try {
Expand All @@ -84,8 +54,8 @@ private List<MarkdownRange> parseRanges(String rangesJSON, String innerText) {
} catch (JSONException e) {
return Collections.emptyList();
}
splitRangesOnEmojis(markdownRanges, "italic");
splitRangesOnEmojis(markdownRanges, "strikethrough");
markdownRanges = splitRangesOnEmojis(markdownRanges, "italic");
markdownRanges = splitRangesOnEmojis(markdownRanges, "strikethrough");
return markdownRanges;
}
public synchronized List<MarkdownRange> parse(@NonNull String text, int parserId) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,32 @@ public int getLength() {
public int getDepth() {
return mDepth;
}

@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}

if (o instanceof MarkdownRange other) {
return this.mType.equals(other.mType)
&& this.mStart == other.mStart
&& this.mEnd == other.mEnd
&& this.mLength == other.mLength
&& this.mDepth == other.mDepth;
}
return false;
}

@NonNull
@Override
public String toString() {
return "MarkdownRange{" +
"type='" + mType + "'" +
", start=" + mStart +
", end=" + mEnd +
", length=" + mLength +
", depth=" + mDepth +
"}";
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
package com.expensify.livemarkdown;

import androidx.annotation.NonNull;

import java.util.ArrayList;
import java.util.List;

public class RangeSplitter {
public static ArrayList<MarkdownRange> splitRangesOnEmojis(@NonNull List<MarkdownRange> markdownRanges, @NonNull String type) {
ArrayList<MarkdownRange> emojiRanges = new ArrayList<>();
ArrayList<MarkdownRange> oldRanges = new ArrayList<>(markdownRanges);
ArrayList<MarkdownRange> newRanges = new ArrayList<>();
for (MarkdownRange range : oldRanges) {
if (range.getType().equals("emoji")) {
emojiRanges.add(range);
}
}

int i = 0;
int j = 0;
while (i < oldRanges.size()) {
MarkdownRange currentRange = oldRanges.get(i);
if (!currentRange.getType().equals(type)) {
newRanges.add(currentRange);
i += 1;
continue;
}

// Iterate through all emoji ranges before the end of the current range, splitting the current range at each intersection.
while (j < emojiRanges.size()) {
MarkdownRange emojiRange = emojiRanges.get(j);
if (emojiRange.getStart() > currentRange.getEnd()) {
break;
}

int currentStart = currentRange.getStart();
int currentEnd = currentRange.getEnd();
int emojiStart = emojiRange.getStart();
int emojiEnd = emojiRange.getEnd();
if (emojiStart >= currentStart && emojiEnd <= currentEnd) { // Intersection
MarkdownRange newRange = new MarkdownRange(currentRange.getType(), currentStart, emojiStart - currentStart, currentRange.getDepth());
currentRange = new MarkdownRange(currentRange.getType(), emojiEnd, currentEnd - emojiEnd, currentRange.getDepth());

newRanges.add(newRange);
}
j += 1;
}
newRanges.add(currentRange);
i += 1;
}
return newRanges;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
package com.expensify.livemarkdown;

import static com.expensify.livemarkdown.RangeSplitter.splitRangesOnEmojis;

import static org.junit.Assert.assertEquals;

import org.junit.Test;

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;

public class RangeSplitterTest {

@Test
public void testNoOverlap() {
List<MarkdownRange> markdownRanges = new ArrayList<>();
markdownRanges.add(new MarkdownRange("strikethrough", 0, 10, 1));
markdownRanges.add(new MarkdownRange("emoji", 12, 2, 1));

markdownRanges = splitRangesOnEmojis(markdownRanges, "strikethrough");

assertEquals(2, markdownRanges.size());
assertEquals(new MarkdownRange("strikethrough", 0, 10, 1), markdownRanges.get(0));
assertEquals(new MarkdownRange("emoji", 12, 2, 1), markdownRanges.get(1));
}

@Test
public void testOverlapDifferentType() {
List<MarkdownRange> markdownRanges = new ArrayList<>();
markdownRanges.add(new MarkdownRange("strikethrough", 0, 10, 1));
markdownRanges.add(new MarkdownRange("emoji", 3, 4, 1));

markdownRanges = splitRangesOnEmojis(markdownRanges, "italic");

assertEquals(2, markdownRanges.size());
assertEquals(new MarkdownRange("strikethrough", 0, 10, 1), markdownRanges.get(0));
assertEquals(new MarkdownRange("emoji", 3, 4, 1), markdownRanges.get(1));
}

@Test
public void testSingleOverlap() {
List<MarkdownRange> markdownRanges = new ArrayList<>();
markdownRanges.add(new MarkdownRange("strikethrough", 0, 10, 1));
markdownRanges.add(new MarkdownRange("emoji", 3, 4, 1)); // This range should split the strikethrough range

markdownRanges = splitRangesOnEmojis(markdownRanges, "strikethrough");

// Sort is needed because ranges may get mixed while splitting
Collections.sort(markdownRanges, (r1, r2) -> Integer.compare(r1.getStart(), r2.getStart()));
Comment on lines +49 to +50
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it sufficient to compare ranges only based on start?

Is splitRangesOnEmojis deterministic?

Do we really need to sort the ranges?

Copy link
Author

@289Adam289 289Adam289 Dec 12, 2024

Choose a reason for hiding this comment

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

Is it sufficient to compare ranges only based on start?

For the tests I wrote - I think so but in generality no.

Is splitRangesOnEmojis deterministic?

It is.

Do we really need to sort the ranges?

splitRangesOnEmojis does not guarantee any order of ranges. I've added sort so that tests work for different implementation of the method.


assertEquals(3, markdownRanges.size());
assertEquals(new MarkdownRange("strikethrough", 0, 3, 1), markdownRanges.get(0));
assertEquals(new MarkdownRange("emoji", 3, 4, 1), markdownRanges.get(1));
assertEquals(new MarkdownRange("strikethrough", 7, 3, 1), markdownRanges.get(2));
}

@Test
public void testMultipleOverlapsMultipleTypes() {
List<MarkdownRange> markdownRanges = new ArrayList<>();
markdownRanges.add(new MarkdownRange("italic", 0, 20, 1));
markdownRanges.add(new MarkdownRange("strikethrough", 2, 12, 1));
markdownRanges.add(new MarkdownRange("emoji", 3, 1, 1));
markdownRanges.add(new MarkdownRange("emoji", 8, 2, 1));
markdownRanges.add(new MarkdownRange("strikethrough", 22, 5, 1));

markdownRanges = splitRangesOnEmojis(markdownRanges, "strikethrough");

// Sort is needed because ranges may get mixed while splitting
Collections.sort(markdownRanges, (r1, r2) -> Integer.compare(r1.getStart(), r2.getStart()));

assertEquals(7, markdownRanges.size());
assertEquals(new MarkdownRange("italic", 0, 20, 1), markdownRanges.get(0));
assertEquals(new MarkdownRange("strikethrough", 2, 1, 1), markdownRanges.get(1));
assertEquals(new MarkdownRange("emoji", 3, 1, 1), markdownRanges.get(2));
assertEquals(new MarkdownRange("strikethrough", 4, 4, 1), markdownRanges.get(3));
assertEquals(new MarkdownRange("emoji", 8, 2, 1), markdownRanges.get(4));
assertEquals(new MarkdownRange("strikethrough", 10, 4, 1), markdownRanges.get(5));
assertEquals(new MarkdownRange("strikethrough", 22, 5, 1), markdownRanges.get(6));
}
}
Loading