Replace paragraphs with line breaks in message output #834

jonnyandrew · 2023-10-02T11:24:26Z

Changes

Replace paragraphs with line breaks in message output.
Parse line breaks to paragraphs (see comment).

Context

codecov-commenter · 2023-10-04T15:51:53Z

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (450db53) 88.08% compared to head (ba2e8c9) 88.29%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #834      +/-   ##
============================================
+ Coverage     88.08%   88.29%   +0.20%     
============================================
  Files           161      114      -47     
  Lines         18167    16722    -1445     
  Branches        971      631     -340     
============================================
- Hits          16003    14764    -1239     
- Misses         1908     1929      +21     
+ Partials        256       29     -227

Flag	Coverage Δ
uitests	`?`
uitests-android	`?`
uitests-ios	`?`
unittests	`88.29% <98.60%> (+1.93%)`	⬆️
unittests-android	`?`
unittests-ios	`76.91% <ø> (+0.23%)`	⬆️
unittests-rust	`89.46% <98.60%> (-0.46%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
crates/wysiwyg/src/composer_model/delete_text.rs	`97.42% <100.00%> (+0.01%)`	⬆️
...rates/wysiwyg/src/composer_model/example_format.rs	`97.31% <100.00%> (-0.16%)`	⬇️
...s/wysiwyg/src/composer_model/format_inline_code.rs	`90.61% <100.00%> (-8.08%)`	⬇️
crates/wysiwyg/src/composer_model/quotes.rs	`97.30% <100.00%> (ø)`
crates/wysiwyg/src/dom/dom_block_nodes.rs	`98.03% <100.00%> (+0.02%)`	⬆️
crates/wysiwyg/src/dom/dom_list_methods.rs	`95.52% <100.00%> (-2.40%)`	⬇️
crates/wysiwyg/src/dom/dom_struct.rs	`91.24% <100.00%> (ø)`
crates/wysiwyg/src/dom/find_extended_range.rs	`99.31% <100.00%> (-0.69%)`	⬇️
crates/wysiwyg/src/dom/iter.rs	`87.68% <100.00%> (-0.27%)`	⬇️
crates/wysiwyg/src/dom/nodes/dom_node.rs	`88.15% <100.00%> (-0.66%)`	⬇️
... and 7 more

... and 61 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

langleyd

I don't have all the context but the premise makes sense, and intuitively approach taken with the implementation makes sense to me and seems like a good direction to bring things. Just a couple of clarifications.

We should probably merge #842 first or we may see even more paragraphs in the message content, maybe we can merge it in to re-run tests.

langleyd · 2023-10-11T11:43:29Z

crates/wysiwyg/src/composer_model/delete_text.rs

-                    _ => ComposerUpdate::keep(),
-                }
-            }
+            DomNode::Container(_) | DomNode::LineBreak(_) => match start_type {


If we convert paragraphs to line breaks do we still need LineBreak type internally in the Dom?

The function to add line breaks is deprecated and fell out of use at the point of doing #472 I believe, so the Dom shouldn't contain these nodes. However, the LineBreak node is still being used to represent line breaks in the Dom immediately after parsing, before being post-processed into paragraphs.

This is a bit problematic because although we can expect that the Dom should no longer contain LineBreaks, there is a lot of existing code that expects that it might which is effectively dead code and a maintenance burden.

Perhaps we can either

Remove the LineBreak node and parse <br> directly to Container. It will be the most effort but, after this PR, we have compatible tests to make this easier. There is also the question of whether this would put too much logic in the HTML parser.

Keep the LineBreak node but refactor it to something like Placeholder(LineBreak), adding an invariant assertion that it should not exist in the Dom. We could remove any logic that handles this node type and replace it with an unreachable! error.

What do you think?

Thanks for that. I'm ok with either option, was more trying to understand it's current use.

Created Line break nodes are (almost) redundant #847

langleyd · 2023-10-11T12:18:42Z

crates/wysiwyg/src/dom/parser/parse.rs

    dom
 }

+// Group consecutive inline nodes into paragraphs.


Could you describe a bit the needs to group consecutive inline nodes into paragraphs?

Is it that we are removing the linebreak type and adding the requirement of inline items to be grouped in a paragraphs, to simplify the implementation and assumptions we can make?

…into jonny/paragraph-to-line-break

langleyd · 2023-10-12T11:54:22Z

It's a snapshot test that's failing and I downloaded both files, they both look the same(used an online tool to compare) and I can't see any pixels that change but the content is different. So I think we can just accept the new one.

This reverts commit 9ed1a4f.

- Reverting snapshot as I think the latest difference might be because of running locally on my machine(maybe as it's intel chip). - Check if changing input to <br/>'s fixes it.

aringenbach

Works on my Mac for this test (if using iOS < 17.0).

Also it's needed to update the other block quote test that way to preserve functional snapshot tests:

    func testMultipleBlocksContent() throws {
        viewModel.setHtmlContent(
            """
            <blockquote>Some<br/>\
            multi-line<br/>\
            quote</blockquote>\
            <br/>\
            <br/>\
            Some text<br/>\
            <br/>\
            <pre>A\n\tcode\nblock</pre>\
            <br/>\
            <br/>\
            Some <code>inline</code> code
            """
        )
        assertSnapshot(
            matching: hostingController,
            as: .image(on: .iPhone13),
            record: isRecord
        )

jonnyandrew · 2023-10-16T08:25:43Z

@langleyd is there any change still needed on iOS to fix the test failures? And about #834 (comment), am I ok to open a separate issue?

sonarqubecloud · 2023-10-16T15:23:03Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

jonnyandrew force-pushed the jonny/paragraph-to-line-break branch from ec0d055 to 2e059e5 Compare October 4, 2023 15:47

jonnyandrew force-pushed the jonny/paragraph-to-line-break branch from 2e059e5 to cab8b83 Compare October 4, 2023 15:54

jonnyandrew added 17 commits October 4, 2023 17:01

Replace paragraphs with line breaks in message output

96a182d

Parse line breaks to paragraphs

1562880

Update rendering tests

07aa46a

Fix deletion tests

d7964e5

Update find range tests

536a4b4

Update inline code tests

15f4166

Update example format tests

983a436

Update paragraphs tests

0268f47

Update mentions tests

e6537e3

Update quotes tests

efd2aed

Update wrap in block tests

b5310c1

Update wrap in list tests

a0bd138

Update dom iterator tests

519bac4

Update characters test

ef5faa0

Update formatting tests

ac09d04

Update link tests

da49ec5

Refactor

519ff31

jonnyandrew force-pushed the jonny/paragraph-to-line-break branch 2 times, most recently from a9b654a to d0a6ad8 Compare October 4, 2023 16:20

Update web test

74337c9

jonnyandrew force-pushed the jonny/paragraph-to-line-break branch from d0a6ad8 to 74337c9 Compare October 4, 2023 16:24

jonnyandrew changed the title ~~POC: Replace paragraphs with line breaks in message output~~ Replace paragraphs with line breaks in message output Oct 4, 2023

jonnyandrew requested a review from a team October 4, 2023 16:31

jonnyandrew marked this pull request as ready for review October 4, 2023 16:31

jonnyandrew added 3 commits October 5, 2023 12:36

Handle paragraphs mixed with inline content

9539239

Fix repeated post-processing by web parser

a40c98a

Don't add line breaks where it is implicit

e6370a3

Fix clippy issue

a642b37

langleyd self-requested a review October 11, 2023 08:23

jonnyandrew added Android iOS labels Oct 11, 2023

langleyd approved these changes Oct 11, 2023

View reviewed changes

jonnyandrew added 2 commits October 11, 2023 15:23

Add documentation to explain paragaph grouping logic

482862b

Merge branch 'main' of github.com:matrix-org/matrix-rich-text-editor …

76fa0b7

…into jonny/paragraph-to-line-break

langleyd added 3 commits October 12, 2023 13:01

update snapshot on ios

9ed1a4f

Revert "update snapshot on ios"

e3ed1f6

This reverts commit 9ed1a4f.

Update SnapshotTests+Blocks.swift

ba2e8c9

- Reverting snapshot as I think the latest difference might be because of running locally on my machine(maybe as it's intel chip). - Check if changing input to <br/>'s fixes it.

aringenbach reviewed Oct 12, 2023

View reviewed changes

Fix testMultipleBlocksContent according to new format.

88e69e5

jonnyandrew merged commit 0663938 into main Oct 18, 2023
1 check passed

jonnyandrew deleted the jonny/paragraph-to-line-break branch October 18, 2023 10:30

jonnyandrew mentioned this pull request Oct 18, 2023

Line break nodes are (almost) redundant #847

Open

jonnyandrew mentioned this pull request Nov 21, 2023

Rich text bubbles have massive paragraph breaks when authored with RTE. element-hq/element-x-ios#1673

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace paragraphs with line breaks in message output #834

Replace paragraphs with line breaks in message output #834

jonnyandrew commented Oct 2, 2023 •

edited

Loading

codecov-commenter commented Oct 4, 2023 •

edited

Loading

langleyd left a comment •

edited

Loading

langleyd Oct 11, 2023

jonnyandrew Oct 11, 2023 •

edited

Loading

langleyd Oct 11, 2023

jonnyandrew Oct 18, 2023 •

edited

Loading

langleyd Oct 11, 2023

langleyd commented Oct 12, 2023

aringenbach left a comment •

edited

Loading

jonnyandrew commented Oct 16, 2023

sonarqubecloud bot commented Oct 16, 2023

Replace paragraphs with line breaks in message output #834

Replace paragraphs with line breaks in message output #834

Conversation

jonnyandrew commented Oct 2, 2023 • edited Loading

Changes

Context

codecov-commenter commented Oct 4, 2023 • edited Loading

Codecov Report

langleyd left a comment • edited Loading

Choose a reason for hiding this comment

langleyd Oct 11, 2023

Choose a reason for hiding this comment

jonnyandrew Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

langleyd Oct 11, 2023

Choose a reason for hiding this comment

jonnyandrew Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

langleyd Oct 11, 2023

Choose a reason for hiding this comment

langleyd commented Oct 12, 2023

aringenbach left a comment • edited Loading

Choose a reason for hiding this comment

jonnyandrew commented Oct 16, 2023

sonarqubecloud bot commented Oct 16, 2023

jonnyandrew commented Oct 2, 2023 •

edited

Loading

codecov-commenter commented Oct 4, 2023 •

edited

Loading

langleyd left a comment •

edited

Loading

jonnyandrew Oct 11, 2023 •

edited

Loading

jonnyandrew Oct 18, 2023 •

edited

Loading

aringenbach left a comment •

edited

Loading