AkkomaGang/akkoma
AkkomaGang/akkoma
15
65
Fork 92
Code Issues 177 Pull requests 25 Projects 2 Releases 14 Packages Wiki Activity

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
pull from: ilja/akkoma:use_fep-c16b_formatting_mfm_functions
merge into: AkkomaGang:develop
Conversation 3 Commits 2 Files changed 4 +70 -27
ilja commented 2024-08-04 09:50:45 +00:00
Contributor
Copy link

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 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
ilja commented 2024-08-10 18:44:06 +00:00
Author
Contributor
Copy link

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
ilja commented 2024-08-11 12:50:03 +00:00
Author
Contributor
Copy link

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
Details
ci/woodpecker/pr/build-arm64 Pipeline is pending
Details
ci/woodpecker/pr/docs Pipeline is pending
Details
ci/woodpecker/pr/lint Pipeline is pending
Details
ci/woodpecker/pr/test Pipeline is pending
Details
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
floatingghost commented 2024-08-20 10:35:23 +00:00
Owner
Copy link

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~
🎉 1
Some checks are pending
ci/woodpecker/pr/build-amd64 Pipeline is pending
Details
ci/woodpecker/pr/build-arm64 Pipeline is pending
Details
ci/woodpecker/pr/docs Pipeline is pending
Details
ci/woodpecker/pr/lint Pipeline is pending
Details
ci/woodpecker/pr/test Pipeline is pending
Details
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.
Reviewers
No reviewers
Labels
Clear labels   
approved, awaiting change

   
bug

Something is not working

  
configuration

This requires config changes

  
documentation

This is about human-readable docs

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
extremely low priority

   
feature request

Something new?

  
Fix it yourself

Support has been attempted

  
help wanted

Need some help

  
invalid

Something is wrong

  
mastodon_api

   
needs docs

   
needs tests

   
not a bug

   
planned

   
pleroma_api

   
privacy

   
question

More information is needed

  
static_fe

   
triage

   
wontfix

This won't be fixed

No labels
approved, awaiting change
bug
configuration
documentation
duplicate
enhancement
extremely low priority
feature request
Fix it yourself
help wanted
invalid
mastodon_api
needs docs
needs tests
not a bug
planned
pleroma_api
privacy
question
static_fe
triage
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: AkkomaGang/akkoma#823
No description provided.
Delete branch "ilja/akkoma:use_fep-c16b_formatting_mfm_functions"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?