Only treat _ as markdown if separated by white space #5685

grzesiek2010 · 2023-07-19T20:48:51Z

What has been done to verify that this works as intended?

I've improved automated tests.

Why is this the best possible solution? Were any other approaches considered?

This is what we discussed in the issue.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

We need to test styling with underscores _ https://docs.getodk.org/form-styling/#emphasis. Other types of styling should rather be intact.

Do we need any specific form for testing your changes? If so, please attach one.

No.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

Before submitting this PR, please make sure you have:

run ./gradlew checkAll and confirmed all checks still pass OR confirm CircleCI build passes and run ./gradlew connectedDebugAndroidTest locally.
verified that any code or assets from external sources are properly credited in comments and/or in the about file.
verified that any new UI elements use theme colors. UI Components Style guidelines

grzesiek2010 · 2023-07-19T21:17:17Z

collect_app/src/main/java/org/odk/collect/android/utilities/HtmlUtils.java

+        text = text.replaceAll("\\s_([^\\s][^_\n]*)_\\s", " <em>$1</em> ");
+        text = text.replaceAll("^_([^\\s][^_\n]*)_$", "<em>$1</em>");
+        text = text.replaceAll("^_([^\\s][^_\n]*)_\\s", "<em>$1</em> ");
+        text = text.replaceAll("\\s_([^\\s][^_\n]*)_$", " <em>$1</em>");


Maybe it's possible to have one regex for all those 4 cases I don't know I'm not a master of regex. But even if it's possible I think this way is better because it's clear and such a one regex for all cases would be complicated.

grzesiek2010 · 2023-07-19T21:21:17Z

collect_app/src/main/java/org/odk/collect/android/utilities/HtmlUtils.java

@@ -77,32 +77,40 @@ static String markdownToHtml(String text) {
                text, createSpan);

        //intermediary replacements keys for special characters, N/B: These symbols are not meant to be interpreted as markdown
-        text = text.replaceAll("(?s)\\\\#", "&#35;");


(?s) in regex is called a dot-all mode.
In the default mode (without (?s)), the dot (.) character in a regex pattern matches any character except a newline (\n). However, when (?s) is used, the dot (.) matches any character, including newline characters.
so it's about using dots but the lines I updated by removing (?s) didn't contain any.

lognaturel · 2023-08-24T21:46:03Z

collect_app/src/main/java/org/odk/collect/android/utilities/HtmlUtils.java

+        text = text.replaceAll("\\s_([^\\s][^_\n]*)_$", " <em>$1</em>");
+
+        // emphasis using asterisk
+        text = text.replaceAll("\\s\\*([^\\s][^\\*\n]*)\\*\\s", " <em>$1</em> ");


I think we actually want to keep * the same. There's no practical use of * surrounded by text that I can think of. This is why Github makes the space special behavior only for _:

un*frigging*believable: unfriggingbelievable
un_frigging_believable: un_frigging_believable

Ok I've reverted that change.

lognaturel · 2023-08-24T21:46:54Z

collect_app/src/test/java/org/odk/collect/android/utilities/HtmlUtilsTest.java

@@ -31,15 +31,13 @@ public void markDownToHtmlEscapesBackslash() {
                {"_A\\_B\\_C_", "<em>A_B_C</em>"},
                {"A_B\\_C", "A_B_C"},
                {"A\\_B_C", "A_B_C"},
-                {"A_B_C", "A<em>B</em>C"},


I think it would be helpful to keep the existing test cases with the new behavior so it's easier to see what changed. The new cases you added are helpful too so I suggest keeping both.

lognaturel

Sorry this took me forever to get to! Regex take a lot of concentration for me to read.

The behavior for _ looks like what I would expect. I think we should revert the change to *.

lognaturel

Sweet!!

srujner · 2023-09-06T11:47:44Z

Tested with Success!

Verified on device with Android 13

Verified cases:

Issue Only treat _ as markdown if separated by white space #5508 is no longer reproducing;
Checked different styling option in form e.g. white spaces before words, after words, in the middle of sentences;
Regression check on other Styling options;
Light and Dark mode;
Rotate the screen, minimize the app;

grzesiek2010 mentioned this pull request Jul 19, 2023

Only treat _ as markdown if separated by white space #5508

Closed

grzesiek2010 commented Jul 19, 2023

View reviewed changes

lognaturel reviewed Aug 24, 2023

View reviewed changes

lognaturel requested changes Aug 24, 2023

View reviewed changes

grzesiek2010 force-pushed the COLLECT-5508 branch from 970fc25 to 1d2278b Compare August 25, 2023 10:18

grzesiek2010 marked this pull request as ready for review August 25, 2023 10:39

grzesiek2010 requested a review from lognaturel August 25, 2023 10:40

lognaturel approved these changes Aug 31, 2023

View reviewed changes

lognaturel added the needs testing label Aug 31, 2023

grzesiek2010 added 4 commits September 1, 2023 09:21

Removed redundant DOTALL modifiers

f7a85db

Improve handling markdown italic

3005847

Improved tests

938955b

Fixed the all-widgets form

9da364e

grzesiek2010 force-pushed the COLLECT-5508 branch from 1d2278b to 9da364e Compare September 1, 2023 07:23

lognaturel changed the title ~~Improve handling markdown italic~~ Only treat _ as markdown if separated by white space Sep 1, 2023

srujner added behavior verified and removed needs testing labels Sep 6, 2023

grzesiek2010 merged commit 3cc9ea3 into getodk:master Sep 6, 2023

grzesiek2010 mentioned this pull request Apr 15, 2024

Improved interpreting text as markdown #6095

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only treat _ as markdown if separated by white space #5685

Only treat _ as markdown if separated by white space #5685

grzesiek2010 commented Jul 19, 2023 •

edited

Loading

grzesiek2010 Jul 19, 2023

grzesiek2010 Jul 19, 2023 •

edited

Loading

lognaturel Aug 24, 2023

grzesiek2010 Aug 25, 2023

lognaturel Aug 24, 2023

grzesiek2010 Aug 25, 2023

lognaturel left a comment

lognaturel left a comment

srujner commented Sep 6, 2023

Only treat _ as markdown if separated by white space #5685

Only treat _ as markdown if separated by white space #5685

Conversation

grzesiek2010 commented Jul 19, 2023 • edited Loading

What has been done to verify that this works as intended?

Why is this the best possible solution? Were any other approaches considered?

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Do we need any specific form for testing your changes? If so, please attach one.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

Before submitting this PR, please make sure you have:

grzesiek2010 Jul 19, 2023

Choose a reason for hiding this comment

grzesiek2010 Jul 19, 2023 • edited Loading

Choose a reason for hiding this comment

lognaturel Aug 24, 2023

Choose a reason for hiding this comment

grzesiek2010 Aug 25, 2023

Choose a reason for hiding this comment

lognaturel Aug 24, 2023

Choose a reason for hiding this comment

grzesiek2010 Aug 25, 2023

Choose a reason for hiding this comment

lognaturel left a comment

Choose a reason for hiding this comment

lognaturel left a comment

Choose a reason for hiding this comment

srujner commented Sep 6, 2023

grzesiek2010 commented Jul 19, 2023 •

edited

Loading

grzesiek2010 Jul 19, 2023 •

edited

Loading