Use FEP-c16b: Formatting MFM functions #823

Open
ilja wants to merge 2 commits from ilja/akkoma:use_fep-c16b_formatting_mfm_functions into develop
Contributor

See https://akkoma.dev/AkkomaGang/akkoma/issues/381

We can't use the HTML content as-is. FEP-c16b was written to have a "scrubber friendly" way of representing MFM functions in HTML. Here we add support in the backend for the functions Foundkey supports.

This currently uses the parser from https://codeberg.org/ilja/mfm_parser/ but can be changed to the one under the Akkoma namespace once AkkomaGang/mfm-parser#2 is merged.

Caveats:

  • FEP-c16b also has a discovery mechanism, this is not implemented yet
    • Could be added here, or could be a new PR (federate the new htmlMfm term, and use the content when that term is provided on incoming federation). This PR is already a strict improvement over the current situation, so it makes sense to me to keep this PR smaller and do the discovery in a separate PR.

Broader scope where this PR is part of:

Extra:

I fixed the earmark version to 1.4.46 bc there's apparently a bug in 1.4.47 (which I first pulled in). See my comment #823 (comment)

See <https://akkoma.dev/AkkomaGang/akkoma/issues/381> We can't use the HTML content as-is. [FEP-c16b](https://codeberg.org/fediverse/fep/src/branch/main/fep/c16b/fep-c16b.md) was written to have a "scrubber friendly" way of representing MFM functions in HTML. Here we add support in the backend for the functions Foundkey supports. This currently uses the parser from https://codeberg.org/ilja/mfm_parser/ but can be changed to the one under the Akkoma namespace once https://akkoma.dev/AkkomaGang/mfm-parser/pulls/2 is merged. Caveats: - FEP-c16b also has a discovery mechanism, this is not implemented yet - Could be added here, or could be a new PR (federate the new `htmlMfm` term, and use the `content` when that term is provided on incoming federation). This PR is already a strict improvement over the current situation, so it makes sense to me to keep this PR smaller and do the discovery in a separate PR. Broader scope where this PR is part of: - FEP-c16b is in DRAFT and can be seen at <https://codeberg.org/fediverse/fep/src/branch/main/fep/c16b/fep-c16b.md> - The MfmParser needs big adaptions. See <https://akkoma.dev/AkkomaGang/mfm-parser/pulls/2> - Akkoma front-end needs changes, see <https://akkoma.dev/AkkomaGang/akkoma-fe/pulls/410> Extra: I fixed the earmark version to 1.4.46 bc there's apparently a bug in 1.4.47 (which I first pulled in). See my comment https://akkoma.dev/AkkomaGang/akkoma/pulls/823#issuecomment-12826
ilja added 1 commit 2024-08-04 09:50:46 +00:00
Use FEP-c16b: formatting mfm functions
Some checks are pending
ci/woodpecker/pr/build-amd64 Pipeline is pending
ci/woodpecker/pr/build-arm64 Pipeline is pending
ci/woodpecker/pr/docs Pipeline is pending
ci/woodpecker/pr/lint Pipeline is pending
ci/woodpecker/pr/test Pipeline is pending
5fd6771bc6
See <#381>

We can't use the HTML content as-is. FEP-c16b was written to have a "scrubber friendly" way of representing MFM functions in HTML.
Here we add support in the backend for the functions Foundkey supports.
Front-ends still need to add support to make sure the HTML representation is turned into a correct view. (I.e. by help of CSS and Javascript)

Some caveats at the moment of writing:
- The mfm_parser module is from an unmerged branch. That means that it may change in the future and that the branch may be removed in the future
- FEP-c16b isn't part of the FEP repository yet. An outline has been written, but not yet merged
- FEP-c16b also has a discovery meganism,this is not implemented in this commit
ilja force-pushed use_fep-c16b_formatting_mfm_functions from 5fd6771bc6 to f6422cb370 2024-08-10 18:23:23 +00:00 Compare
ilja changed title from WIP: Use FEP-c16b: Formatting MFM functions to Use FEP-c16b: Formatting MFM functions 2024-08-10 18:35:52 +00:00
Author
Contributor

I notice mix.lock also changed a bunch of stuff. I'm not used to changing dependencies, so I'm unsure if that's OK or what I should do with that?

I was even wondering why this is not ignored, since it's rebuild anyhow, but apparently the good practice is to keep it https://elixirforum.com/t/should-mix-lock-be-part-of-the-public-repository-and-package/45203

I notice mix.lock also changed a bunch of stuff. I'm not used to changing dependencies, so I'm unsure if that's OK or what I should do with that? I was even wondering why this is not ignored, since it's rebuild anyhow, but apparently the good practice is to keep it <https://elixirforum.com/t/should-mix-lock-be-part-of-the-public-repository-and-package/45203>
ilja changed title from Use FEP-c16b: Formatting MFM functions to WIP: Use FEP-c16b: Formatting MFM functions 2024-08-11 12:49:29 +00:00
Author
Contributor

EDIT: Yes, it worked by downgrading earmark back to 1.4.46. There's an open issue for it https://github.com/pragdave/earmark/issues/361

You can ignore the below text 🤐


I put it in WIP again bc I just realised I forgot to run the test. One is failing bc the MFM markup changed, I already fixed that one locally. But the other one seems a problem with Earmark... Maybe bc I drew in a newer version than what the CI previously had? I'll see if I can figure it out.

Afaict, Earmark removes everything before the comment (Formatter.markdown_to_html()). Then the comment (I assume correctly) gets removed later with Formatter.html_escape("text/html").

This is the failure

  1) test format_input/3 with markdown raw HTML (Pleroma.Web.CommonAPI.UtilsTest)
     test/pleroma/web/common_api/utils_test.exs:213
     Assertion with == failed
     code:  assert result == ~s"<a href=\"http://example.org/\">OwO</a>"
     left:  ""
     right: "<a href=\"http://example.org/\">OwO</a>"
     stacktrace:
       test/pleroma/web/common_api/utils_test.exs:216: (test)

Here is the code responsible for this

alias Pleroma.Formatter

  def format_input(text, "text/markdown", options) do
    text
    |> Formatter.mentions_escape(options)
    |> Formatter.markdown_to_html()
    |> Formatter.linkify(options)
    |> Formatter.html_escape("text/html")
  end
  def markdown_to_html(text, opts \\ %{}) do
    Earmark.as_html!(text, %Earmark.Options{compact_output: true} |> Map.merge(opts))
  end
**EDIT:** Yes, it worked by downgrading earmark back to 1.4.46. There's an open issue for it https://github.com/pragdave/earmark/issues/361 You can ignore the below text 🤐 *** I put it in WIP again bc I just realised I forgot to run the test. One is failing bc the MFM markup changed, I already fixed that one locally. But the other one seems a problem with Earmark... Maybe bc I drew in a newer version than what the CI previously had? I'll see if I can figure it out. Afaict, Earmark removes everything before the comment (`Formatter.markdown_to_html()`). Then the comment (I assume correctly) gets removed later with `Formatter.html_escape("text/html")`. This is the failure ``` 1) test format_input/3 with markdown raw HTML (Pleroma.Web.CommonAPI.UtilsTest) test/pleroma/web/common_api/utils_test.exs:213 Assertion with == failed code: assert result == ~s"<a href=\"http://example.org/\">OwO</a>" left: "" right: "<a href=\"http://example.org/\">OwO</a>" stacktrace: test/pleroma/web/common_api/utils_test.exs:216: (test) ``` Here is the code responsible for this ``` alias Pleroma.Formatter def format_input(text, "text/markdown", options) do text |> Formatter.mentions_escape(options) |> Formatter.markdown_to_html() |> Formatter.linkify(options) |> Formatter.html_escape("text/html") end ``` ``` def markdown_to_html(text, opts \\ %{}) do Earmark.as_html!(text, %Earmark.Options{compact_output: true} |> Map.merge(opts)) end ```
ilja added 1 commit 2024-08-11 13:02:15 +00:00
Fix tests
Some checks are pending
ci/woodpecker/pr/build-amd64 Pipeline is pending
ci/woodpecker/pr/build-arm64 Pipeline is pending
ci/woodpecker/pr/docs Pipeline is pending
ci/woodpecker/pr/lint Pipeline is pending
ci/woodpecker/pr/test Pipeline is pending
90adb3cff5
There was one test who used MFM and now failed due to the new representation. This is now adapted so it doesn't fail any more.

There was another test failing, but I don't see how this could have been affected by the MFM changes...
But I did draw in newer dependencies, so I thought maybe a newer EARMARK dependency was now failing, and indeed.
By explicitly asking for 1.4.46 (according to mix.lock the version it was before), it now works again.

This is what was failing. It seems that earmark 1.4.47 removed everything before the comment, which it should not do.

  1) test format_input/3 with markdown raw HTML (Pleroma.Web.CommonAPI.UtilsTest)
     test/pleroma/web/common_api/utils_test.exs:213
     Assertion with == failed
     code:  assert result == ~s"<a href=\"http://example.org/\">OwO</a>"
     left:  ""
     right: "<a href=\"http://example.org/\">OwO</a>"
     stacktrace:
       test/pleroma/web/common_api/utils_test.exs:216: (test)
ilja changed title from WIP: Use FEP-c16b: Formatting MFM functions to Use FEP-c16b: Formatting MFM functions 2024-08-11 13:05:57 +00:00

I notice mix.lock also changed a bunch of stuff. I'm not used to changing dependencies, so I'm unsure if that's OK or what I should do with that?

yeah that's fine that's just mix updating stuff it's fine

this looks very cool, i'll test it out a bit~

>I notice mix.lock also changed a bunch of stuff. I'm not used to changing dependencies, so I'm unsure if that's OK or what I should do with that? yeah that's fine that's just mix updating stuff it's fine this looks very cool, i'll test it out a bit~
Some checks are pending
ci/woodpecker/pr/build-amd64 Pipeline is pending
ci/woodpecker/pr/build-arm64 Pipeline is pending
ci/woodpecker/pr/docs Pipeline is pending
ci/woodpecker/pr/lint Pipeline is pending
ci/woodpecker/pr/test Pipeline is pending
This pull request has changes conflicting with the target branch.
  • mix.lock
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u use_fep-c16b_formatting_mfm_functions:ilja-use_fep-c16b_formatting_mfm_functions
git checkout ilja-use_fep-c16b_formatting_mfm_functions
Sign in to join this conversation.
No description provided.