wordpress-mobile / AztecEditor-Android

A reusable native Android rich text editor component.
Mozilla Public License 2.0
675 stars 112 forks source link

Simplify containsPreformat method #994

Closed planarvoid closed 1 year ago

planarvoid commented 1 year ago

Fix

The containsPreformat method doesn't work well when the preformat doesn't contain any code. In this PR I'm simplifying it so that it works in all cases. I don't know why it was originally so complicated. I think the original behaviour doesn't have any benefits.

Test

Enable preformat action by adding it to toolbar actions

        val defaultBasicLayout = BasicLayout(
                ToolbarAction.PREFORMAT,
...

to make it easier set the EXAMPLE in the MainActivity to something like this:

"""
<pre></pre>
<p>paragraph 1</p>
""".trimIndent()
  1. Run the app
  2. Click into the preformat line
  3. Trigger the toolbar preformat action
  4. Notice the preformat disappears and is not duplicated

Review

@khaykov

Make sure strings will be translated:

khaykov commented 1 year ago

@planarvoid At this point I don't remember all the use cases for the complicated method, but I think it's related to line breaks (and maybe something else.

On the left is this branch, on the right is trunk. As you can see the cursor is positioned below the pre block on a new line. In trunk it's correctly detecting that it's not a pre (icon is not highlighted in toolbar) and in this branch it detects pre incorrectly (icon is highlighted). Image from Gyazo

planarvoid commented 1 year ago

Thanks for the review @khaykov . I've returned the original method but it was broken as well so I've fixed it. The previous The previous solution didn't support something like this:

<p>First line</p>
<pre>Pre line 1<br></pre>
<p></p>
<p>Last line</p>

If you had an empty line at the end of PRE block, the PRE button wasn't highlighted when you clicked into it. I think it should be fixed now.

I've also uncommented and fixed all the preformat tests.