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.

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

feat(neon_rich_text): Render markdown #2647

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

provokateurin
Copy link
Member

Closes #1929
Just like the emoji picker I thought it would be much harder, but it was not too bad (but still quite a lot of effort, especially the final newline handling as you will see).

This is a fully custom rendering implementation for markdown including rich objects.
Using https://pub.dev/packages/flutter_markdown was not an option because it does not expose the necessary low level APIs to implement everything we need.
I mostly tested this manually with the example from https://markdown-it.github.io.
This likely implements some things that are actually not supported in Talk right now,

I did not write proper tests for the rendering itself (but there is on Talk test that ensures markdown is rendered when requested). There are multiple viable sources we can use to cover the tests:

  1. Dart markdown package tests: https://github.com/dart-lang/markdown/tree/master/test
  2. Github flavored markdown tests: https://github.com/github/cmark-gfm/tree/master/test
  3. nextcloud-vue tests for NcRichText (basically the equivalent Vue component responsible for rendering markdown and rich objects): https://github.com/nextcloud-libraries/nextcloud-vue/blob/master/cypress/component/richtext.cy.ts

IMO option 3 is the best right now, because it will strike a good balance between effort and coverage. Anything not covered by these tests should not ever appear in a Talk message so we should be good.

While writing this I realized I never tested check boxes and they are not implemented either. In Talk there is this cool feature where a click on a checkbox will toggle it and actually edit the message. Implementing that will probably not be easy, so I would like to skip it for now and open a new issue for it once this is merged.

This isn't finished completely, because I first wanted to discuss the general direction before putting even more effort into this.

Copy link

codecov bot commented Nov 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 28.95%. Comparing base (9d0db0a) to head (2bef616).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2647      +/-   ##
==========================================
+ Coverage   28.88%   28.95%   +0.06%     
==========================================
  Files         373      373              
  Lines      138192   138327     +135     
==========================================
+ Hits        39922    40057     +135     
  Misses      98270    98270              
Flag Coverage Δ *Carryforward flag
account_repository 98.47% <ø> (ø)
cookie_store 99.48% <ø> (ø) Carriedforward from 9d0db0a
dashboard_app 96.05% <ø> (ø)
dynamite 31.05% <ø> (ø) Carriedforward from 9d0db0a
dynamite_end_to_end_test 61.75% <ø> (ø) Carriedforward from 9d0db0a
dynamite_runtime 85.40% <ø> (ø) Carriedforward from 9d0db0a
interceptor_http_client 97.18% <ø> (ø) Carriedforward from 9d0db0a
neon_dashboard 96.05% <ø> (ø) Carriedforward from 9d0db0a
neon_framework 59.34% <ø> (ø)
neon_http_client 93.61% <ø> (ø) Carriedforward from 9d0db0a
neon_notifications 100.00% <ø> (ø) Carriedforward from 9d0db0a
neon_rich_text 100.00% <100.00%> (ø)
neon_storage 94.66% <ø> (ø) Carriedforward from 9d0db0a
neon_talk 99.45% <ø> (ø) Carriedforward from 9d0db0a
nextcloud 24.36% <ø> (ø) Carriedforward from 9d0db0a
notifications_app 97.43% <100.00%> (+0.06%) ⬆️
notifications_push_repository 98.11% <ø> (ø)
sort_box 90.90% <ø> (ø) Carriedforward from 9d0db0a
talk_app 98.84% <ø> (-0.01%) ⬇️ Carriedforward from 9d0db0a

*This pull request uses carry forward flags. Click here to find out more.

Files with missing lines Coverage Δ
...ork/packages/neon_rich_text/lib/src/rich_text.dart 100.00% <100.00%> (ø)
...otifications_app/lib/src/widgets/notification.dart 95.74% <100.00%> (+0.18%) ⬆️
...ork/packages/talk_app/lib/src/widgets/message.dart 98.73% <ø> (-0.02%) ⬇️

@Leptopoda
Copy link
Member

So we don't forget it.
I think that this is at a point that it would benefit from being a separate package.
Do you agree? In that case, I'd prefer to have this done first.

@provokateurin
Copy link
Member Author

Yeah let's do that, this feature is getting too big by now.

@provokateurin provokateurin force-pushed the feat/neon_framework/rich-text-markdown-rendering branch from d7d7f8e to 9726cc8 Compare November 27, 2024 15:43
@provokateurin
Copy link
Member Author

Moving to a separate package and the cleanups got to big, so I split that off into #2682.

@provokateurin provokateurin changed the base branch from main to feat/neon_rich_text November 27, 2024 15:43
@provokateurin provokateurin changed the title feat(neon_framework): Render markdown in rich text feat(neon_rich_text): Render markdown Nov 27, 2024
@provokateurin provokateurin marked this pull request as draft November 27, 2024 15:44
@provokateurin
Copy link
Member Author

I didn't add any tests yet and the code in this PR also has to be refactored before a proper review can take place.

@provokateurin provokateurin force-pushed the feat/neon_framework/rich-text-markdown-rendering branch from 9726cc8 to 1809b1f Compare November 29, 2024 08:17
@provokateurin provokateurin force-pushed the feat/neon_rich_text branch 2 times, most recently from cdd12ad to 0a3aa4c Compare December 7, 2024 15:59
Base automatically changed from feat/neon_rich_text to main December 8, 2024 11:38
@provokateurin provokateurin force-pushed the feat/neon_framework/rich-text-markdown-rendering branch from 1809b1f to 5f94b6f Compare December 8, 2024 11:41
@provokateurin
Copy link
Member Author

Just a rebase for neon_rich_text, nothing new or tests added.

@provokateurin provokateurin force-pushed the feat/neon_framework/rich-text-markdown-rendering branch from 5f94b6f to dcdcc0d Compare December 8, 2024 21:25
@provokateurin provokateurin marked this pull request as ready for review December 8, 2024 21:25
@provokateurin
Copy link
Member Author

I implemented all tests from https://github.com/nextcloud-libraries/nextcloud-vue/blob/master/cypress/component/richtext.cy.ts plus a few that are simply missing in those tests but are working in the web interface and are implemented by us.

I think this is the perfect use-case for using flutter goldens to ensure the rendering is exactly as expected. Proper fonts had to be used in order to make sure the results are correct. Let's hope the font rendering is consistent...

@provokateurin provokateurin force-pushed the feat/neon_framework/rich-text-markdown-rendering branch 2 times, most recently from cc67f17 to d85fea4 Compare December 8, 2024 21:50
@Leptopoda
Copy link
Member

I think this is the perfect use-case for using flutter goldens to ensure the rendering is exactly as expected.

I agree.

Can you please make a separate PR for the two fixes? This is already a quite big one.

@provokateurin provokateurin force-pushed the feat/neon_framework/rich-text-markdown-rendering branch from d85fea4 to 3c24ecf Compare December 9, 2024 16:37
@provokateurin provokateurin changed the base branch from main to fix/neon_rich_text/rich-objects-stop-inherit-textstyle December 9, 2024 16:37
@provokateurin
Copy link
Member Author

Can you please make a separate PR for the two fixes?

Done, but the second fix is required for this PR so I based this PR on it.

@provokateurin provokateurin force-pushed the feat/neon_framework/rich-text-markdown-rendering branch from 3c24ecf to 055a2e4 Compare December 9, 2024 16:54
@provokateurin provokateurin marked this pull request as draft December 9, 2024 17:07
@provokateurin
Copy link
Member Author

(Put it in draft to avoid accidentally merging it into the other branch)

Base automatically changed from fix/neon_rich_text/rich-objects-stop-inherit-textstyle to main December 10, 2024 12:39
@provokateurin provokateurin marked this pull request as ready for review December 10, 2024 12:41
@provokateurin
Copy link
Member Author

This likely needs a rebase for #2703 as the rendering might have changed slightly.

Signed-off-by: provokateurin <kate@provokateurin.de>
@provokateurin provokateurin force-pushed the feat/neon_framework/rich-text-markdown-rendering branch from 055a2e4 to 2bef616 Compare December 19, 2024 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support markdown in chat messages
2 participants