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.

Sign up for GitHub

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

Improve range split function and add unit tests #553

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6fa4db9
Improve splitRangesOnEmojis complexity
289Adam289 Nov 25, 2024
3e83307
move splitRangesOnEmojis to separate file
289Adam289 Nov 27, 2024
8fe38f3
add tests
289Adam289 Nov 27, 2024
5383871
Merge branch '@Skalakid/fix-emoji-formatting' into @289Adam289/improv…
289Adam289 Dec 11, 2024
0d8327e
fix tests
289Adam289 Dec 11, 2024
cea5128
code formatting
289Adam289 Dec 11, 2024
b39723f
review suggestions
289Adam289 Dec 12, 2024
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
View file
Open in desktop
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'
289Adam289 marked this conversation as resolved.
Show resolved Hide resolved
}

if (isNewArchitectureEnabled()) {
Expand Down
34 changes: 2 additions & 32 deletions android/src/main/java/com/expensify/livemarkdown/MarkdownParser.java
View file
Open in desktop
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 Down
44 changes: 44 additions & 0 deletions android/src/main/java/com/expensify/livemarkdown/RangeSplitter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
package com.expensify.livemarkdown;

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

public class RangeSplitter {
public static void splitRangesOnEmojis(List<MarkdownRange> markdownRanges, String type) {
289Adam289 marked this conversation as resolved.
Show resolved Hide resolved
List<MarkdownRange> emojiRanges = new ArrayList<>();
List<MarkdownRange> oldRanges = new ArrayList<>(markdownRanges);
289Adam289 marked this conversation as resolved.
Show resolved Hide resolved
markdownRanges.clear();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I still don't understand why it's better to modify markdownRanges in-place rather than create a new list. Could you please explain what's the actual benefit from doing so?

Copy link
Author

Choose a reason for hiding this comment

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

I must admit I might have misunderstood you during our last conversation about this.

I don't really see any benefit. I rewrote the function to not modify ranges in place.

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)) {
markdownRanges.add(currentRange);
i += 1;
continue;
}

// Split range
289Adam289 marked this conversation as resolved.
Show resolved Hide resolved
while (j < emojiRanges.size()) {
MarkdownRange emojiRange = emojiRanges.get(j);
if (emojiRange.getStart() > currentRange.getEnd()) break;
289Adam289 marked this conversation as resolved.
Show resolved Hide resolved

if (emojiRange.getStart() >= currentRange.getStart() && emojiRange.getEnd() <= currentRange.getEnd()) {
MarkdownRange newRange = new MarkdownRange(currentRange.getType(), currentRange.getStart(), emojiRange.getStart() - currentRange.getStart(), currentRange.getDepth());
currentRange = new MarkdownRange(currentRange.getType(), emojiRange.getEnd(), currentRange.getEnd() - emojiRange.getEnd(), currentRange.getDepth());
289Adam289 marked this conversation as resolved.
Show resolved Hide resolved

markdownRanges.add(newRange);
}
j += 1;
}
markdownRanges.add(currentRange);
i += 1;
}
}
}
89 changes: 89 additions & 0 deletions android/src/test/java/com/expensify/livemarkdown/RangeSplitterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
package com.expensify.livemarkdown;

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


289Adam289 marked this conversation as resolved.
Show resolved Hide resolved
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 {

private void testRange(MarkdownRange range, int start, int end, String type) {
assertEquals(start, range.getStart());
assertEquals(end, range.getEnd());
assertEquals(type, range.getType());
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of implementing a custom testRange function (which btw should be named assertRangeStartEndType or something along these lines), let's just implement MarkdownRange#equals method and rewrite the assertions in the unit tests like:

assertEquals(new MarkdownRange("type", 0, 1, 0), markdownRanges.get(0));

This way we get more readable logs (with expected and actual value) if test fails rather than some generic assertion error in utility method.

Copy link
Author

Choose a reason for hiding this comment

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

To ensure more readable logs I've added also simple implementation of MarkdownRange#toString method.


@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));

splitRangesOnEmojis(markdownRanges, "strikethrough");

assertEquals(2, markdownRanges.size());

289Adam289 marked this conversation as resolved.
Show resolved Hide resolved
testRange(markdownRanges.get(0), 0, 10, "strikethrough");
testRange(markdownRanges.get(1), 12, 14, "emoji");
}

@Test
public void testOverlapWrongType() {
289Adam289 marked this conversation as resolved.
Show resolved Hide resolved
List<MarkdownRange> markdownRanges = new ArrayList<>();
markdownRanges.add(new MarkdownRange("strikethrough", 0, 10, 1));
markdownRanges.add(new MarkdownRange("emoji", 3, 4, 1));

splitRangesOnEmojis(markdownRanges, "italic");

assertEquals(2, markdownRanges.size());
testRange(markdownRanges.get(0), 0, 10, "strikethrough");
testRange(markdownRanges.get(1), 3, 7, "emoji");
}

@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 should split the strikethrough range
289Adam289 marked this conversation as resolved.
Show resolved Hide resolved

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 •
edited
Loading

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());
testRange(markdownRanges.get(0), 0, 3, "strikethrough");
testRange(markdownRanges.get(1), 3, 7, "emoji");
testRange(markdownRanges.get(2), 7, 10, "strikethrough");
}

@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));

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());
testRange(markdownRanges.get(0), 0, 20, "italic");
testRange(markdownRanges.get(1), 2, 3, "strikethrough");
testRange(markdownRanges.get(2), 3, 4, "emoji");
testRange(markdownRanges.get(3), 4, 8, "strikethrough");
testRange(markdownRanges.get(4), 8, 10, "emoji");
testRange(markdownRanges.get(5), 10, 14, "strikethrough");
testRange(markdownRanges.get(6), 22, 27, "strikethrough");
}
}
Loading