Skip to content

Conversation

@mzorz
Copy link
Contributor

@mzorz mzorz commented Aug 3, 2025

Fix

  • replaced markdown library com.commonsware.cwac:anddown which was not 16kb-page-size compatible, with org.jetbrains:markdown:0.7.3 to handle markdown to html conversions
  • converted NoteMarkdownFragment to Kotlin

Test

  1. Edit an entry, verify the markdown option is checked in the three dots menu
  2. add a markdown title with ###, checkboxes, a quote, etc
  3. exist to save
  4. open the entry on another platform (i.e. web or desktop) and see the titles being rendered in bold

Review

cc @ParaskP7

Release

…t 16kb-page-size compatible, with org.jetbrains:markdown:0.7.3 to handle markdown to html conversions

- converted NoteMarkdownFragment to Kotlin
@mzorz mzorz added this to the Future milestone Aug 3, 2025
@mzorz mzorz added the [Type] Debt Technical debt. label Aug 3, 2025
@dangermattic
Copy link
Collaborator

dangermattic commented Aug 3, 2025

2 Warnings
⚠️ This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
⚠️ Class SimplenoteMarkdownFlavorDescriptor is missing tests, but unit-tests-exemption label was set to ignore this.

Generated by 🚫 Danger

@mzorz mzorz requested a review from ParaskP7 August 3, 2025 14:43
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Aug 3, 2025

📲 You can test the changes from this Pull Request in Simplenote Android by scanning the QR code below to install the corresponding build.

App Name Simplenote Android
Build TypeDebug
Commit1f2f06f
Direct Downloadsimplenote-android-prototype-build-pr1738-1f2f06f-01987b72-3f80-4003-958a-c4e855573564.apk

@ParaskP7 ParaskP7 requested a review from Copilot August 4, 2025 07:45
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR replaces an outdated markdown library with JetBrains' markdown parser and converts a Java fragment to Kotlin. The main purpose is to resolve compatibility issues with 16kb-page-size configurations by migrating from com.commonsware.cwac:anddown to org.jetbrains:markdown:0.7.3.

  • Replaced the anddown markdown library with JetBrains markdown library for HTML conversion
  • Converted NoteMarkdownFragment from Java to Kotlin
  • Updated the markdown parsing implementation to use the new library's API

Reviewed Changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.

File Description
NoteMarkdownFragment.kt New Kotlin implementation replacing the Java version with updated markdown parsing
NoteMarkdownFragment.java Removed the original Java file
build.gradle Updated dependency from anddown to JetBrains markdown library

Copy link
Contributor

@ParaskP7 ParaskP7 left a comment

Choose a reason for hiding this comment

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

👋 @mzorz !

I have reviewed and tested this PR as per the instructions, everything works as expected (ish), nicely done replacing com.commonsware.cwac:anddown! 🙌 x 🙌 ^ 🙌


I have left one warning (⚠️) and a couple of future suggestions (💡) for you. I am NOT going to approve this PR just yet, mainly because of this one warning (⚠️), but other than that this all LGTM.

import org.intellij.markdown.parser.MarkdownParser
import java.lang.ref.SoftReference

class NoteMarkdownFragment : Fragment(), Bucket.Listener<Note> {
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Suggestion (💡): I always recommend to have a Java-to-Kotlin conversion commit first, before making any additional manual change to a file. Otherwise, it can become quite tricky to review. For example, in this case I had to make sure to review both the conversation and the getMarkdownFormattedContent(...) change, all in one commit, risking missing bits.
  2. Suggestion (💡): This is just future suggestion for such Java-to-Kotlin conversions, that using an extra commit for .java > .kt renames helps with preserving the files history and makes it easier to diff the changes, for example:

Before committing the change, you could ☑️ this Extra commit for .java > .kt renames on that IDE commit dialog:

image

And then, instead of having a change like this below, where the Java file is deleted, and another Kotlin file is created:

image

You now get this instead:

image

This above is possible because prior to that an automated Rename .java to .kt is commit for you:

image

Really hope that this will become handy next time you'll converted a Java file! 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh I use the command line for git stuff. I'll remember to do the rename, commit, then convert, commit 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

followed the advice and converted the utils file on this other PR in 3543434 and f42e53e 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

This is awesome @mzorz , I did saw you doing that trick on this other PR and worked fantastically, much much better, thanks so much! 🙇 ❤️


val flavour = CommonMarkFlavourDescriptor()
val parsedTree = MarkdownParser(flavour).buildMarkdownTreeFromString(sourceContent)
var parsedMarkdown = HtmlGenerator(sourceContent, parsedTree, flavour).generateHtml()
Copy link
Contributor

Choose a reason for hiding this comment

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

Warning (⚠️): I asked my friend the AI to compare the previous com.commonsware.cwac:anddown Java implementation to this new org.jetbrains:markdown Kotlin implementation, it comparing these 2 versions:

java

If this Java code below, which is using the 'com.commonsware.cwac:anddown' library:

String parsedMarkdown = new AndDown().markdownToHtml(
sourceContent,
AndDown.HOEDOWN_EXT_STRIKETHROUGH | AndDown.HOEDOWN_EXT_FENCED_CODE |
AndDown.HOEDOWN_EXT_QUOTE | AndDown.HOEDOWN_EXT_TABLES,
AndDown.HOEDOWN_HTML_ESCAPE
);

kotlin

val flavour = CommonMarkFlavourDescriptor()
val parsedTree = MarkdownParser(flavour).buildMarkdownTreeFromString(sourceContent)
var parsedMarkdown = HtmlGenerator(sourceContent, parsedTree, flavour).generateHtml()

It came back with this table below:

Feature AndDown (Hoedown) JetBrains Markdown (CommonMarkFlavourDescriptor)
Strikethrough Yes (with HOEDOWN_EXT_STRIKETHROUGH) No (CommonMark); Yes with GFMFlavourDescriptor
Fenced code blocks Yes (with HOEDOWN_EXT_FENCED_CODE) Yes (supported by default)
Block quotes Yes (with HOEDOWN_EXT_QUOTE) Yes (supported by default)
Tables Yes (with HOEDOWN_EXT_TABLES) No (CommonMark); Yes with GFMFlavourDescriptor
Task lists No (unless using custom extensions) No (CommonMark); Yes with GFMFlavourDescriptor

I then tested Strikethrough, Tables and Task lists and actually it was right, see below:

EDIT
EDIT

PREVIEW (before) -> val flavour = CommonMarkFlavourDescriptor()

PREVIEW (Before)

PREVIEW (after) -> val flavour = GFMFlavourDescriptor()

PREVIEW (After)

This too me looks like AI is right and we should use the GFMFlavourDescriptor in favor of CommonMarkFlavourDescriptor. Yes, for some reason the Strikethrough doesn't look too good (I don't see the actual strikethrough), same with the task lists, but it is nevertheless better than before, right? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great catch, I thought I had GFMFlavourDescriptor in one of my branches but no!

I did observe the prod app was not particularly good in the Preview tab so I didn't check one by one, see for example how an empty checkbox looks like on preview on prod:

image

Will update and perhaps try something about that sneaky strikethrough 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Great catch, I thought I had GFMFlavourDescriptor in one of my branches but no!

Never when you need it, right! 🤣

I did observe the prod app was not particularly good in the Preview tab so I didn't check one by one, see for example how an empty checkbox looks like on preview on prod:

Yea, I know... 🤷

Will update and perhaps try something about that sneaky strikethrough 👍

Thanks so much @mzorz ! 🙇 ❤️

@ParaskP7
Copy link
Contributor

ParaskP7 commented Aug 5, 2025

Ah, and btw @mzorz, unrelated to this PR, but as I was testing this PR, I found another "bug" related to the screen scrolling behavior as well as the keyboard being on top of the content, without a means to move the content up, above the keyboard, see below:

image

Notice that I can't scroll the content up to show this 3rd [ ] Test Three in that task list of mine, nor the content scrolls automatically when the keyboard is called-up. 🤔

@mzorz
Copy link
Contributor Author

mzorz commented Aug 5, 2025

Great testing there @ParaskP7 ! do you have a video for the screen scrolling behavior? I can't reproduce (testing both this version and the prod version)

@ParaskP7
Copy link
Contributor

ParaskP7 commented Aug 5, 2025

Here's a recording for you @mzorz :

screen-20250805-181337.mp4

Does it help? 🙏

@mzorz
Copy link
Contributor Author

mzorz commented Aug 5, 2025

Wow that recording helps. I can't reproduce it here though 🤔 - could you paste the markdown here?

@mzorz
Copy link
Contributor Author

mzorz commented Aug 5, 2025

@ParaskP7 good news, updated the code here 7e2623d and now it supports markdown strikethrough correctly alongside all the other good bits the GFMFlavourDescriptor comes with.

Turns out the GFMFlavourDescriptor converted strikethrough tokens to the following, for example in converting the string Not sure why we are ~~using~~ this:

Not sure why we are <span class="user-del">using</span> this

which then was not being parsed correctly by the Android WebView since it would need some css treatment. So I checked the original code which produced this code:

Not sure why we are <del>using</del> this

The deleted element is supported off the shelf since its basic html.

Had to do some digging into how the GFMFlavourDescriptor worked, and was able to override that mapping to produce <del> tags instead. It works now! see screenshot below:

image

@mzorz
Copy link
Contributor Author

mzorz commented Aug 5, 2025

Here's a recording for you @mzorz :

Will look into this 👍

@mzorz
Copy link
Contributor Author

mzorz commented Aug 5, 2025

as I was testing this PR, I found another "bug" related to the screen scrolling behavior as well as the keyboard being on top of the content, without a means to move the content up, above the keyboard, see below:

✅ this issue where content couldn't be scrolled and the virtual keyboard was hiding it was fixed in dd575ff in #1737

@ParaskP7
Copy link
Contributor

ParaskP7 commented Aug 6, 2025

✅ this #1738 (comment) where content couldn't be scrolled and the virtual keyboard was hiding it was fixed in dd575ff in #1737

As per my other comment, nicely done @mzorz , I just tested there and it works as expected, thank YOU! 🙇 ❤️ 🚀

@ParaskP7
Copy link
Contributor

ParaskP7 commented Aug 6, 2025

@ParaskP7 good news, updated the code here 7e2623d and now it supports markdown strikethrough correctly alongside all the other good bits the GFMFlavourDescriptor comes with.

Awesome, I am checking SimplenoteMarkdownFlavorDescriptor as we speak and testing it, everything works as expected, nicely done extending GFMFlavourDescriptor @mzorz ! 🥇

Turns out the GFMFlavourDescriptor converted strikethrough tokens to the following, for example in converting the string
Not sure why we are ~~using~~ this:

Not sure why we are <span class="user-del">using</span> this

which then was not being parsed correctly by the Android WebView since it would need some css treatment. So I checked the original code which produced this code:

Not sure why we are <del>using</del> this

The deleted element is supported off the shelf since its basic html.

Hmmm, thanks for the explanation on all that, not sure why too... 🤷

Had to do some digging into how the GFMFlavourDescriptor worked, and was able to override that mapping to produce tags instead. It works now! see screenshot below:

Indeed it does, tested! 🎉

@ParaskP7 ParaskP7 self-requested a review August 6, 2025 07:47
@mzorz
Copy link
Contributor Author

mzorz commented Aug 6, 2025

Thanks a ton @ParaskP7 ! 🙌 I'll wait until the base PR is reviewed before merging, to help ease the review there 👍

Base automatically changed from mzorz/update-targetsdk-35 to trunk August 7, 2025 17:01
@mzorz mzorz merged commit 9e4c864 into trunk Aug 7, 2025
15 checks passed
@mzorz mzorz deleted the mzorz/remove-cwac-anddown-library branch August 7, 2025 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants