wordpress-mobile / AztecEditor-Android

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

Issue/18 add special comments #85

Closed theck13 closed 7 years ago

theck13 commented 7 years ago

Fix

Replace <!--more--> and <!--nextpage--> comments with images as described in https://github.com/wordpress-mobile/WordPress-Aztec-Android/issues/18.

Test

Toolbar

  1. Select location or text.
  2. Tap More or Page Break button in format toolbar.
  3. Notice image replaces selection.
  4. Change device orientation.
  5. Notice image scaling.
  6. Tap HTML button in format toolbar.
  7. Notice <!--more--> or <!--nextpage--> in code.

HTML

  1. Tap HTML button in format toolbar.
  2. Input <!--more--> or <!--nextpage--> in code.
  3. Tap HTML button in format toolbar.
  4. Notice image replaces comment.
theck13 commented 7 years ago

Maybe we should implement same logic as in Calypo - when you add comment while you cursor is in block element, comment would be added right after it?

I’m not sure if I found a bug in Calypso or it’s intentional, but there is some weird behavior when adding a special comment while selecting multiple lines around a block element. See the screenshots below for examples.

comments

One option is to always add the special comment after the end of the selection regardless of block or inline element.

khaykov commented 7 years ago

@theck13 Maybe selection of block+inline and toggling comment is not something they though about in Calypso :)

I agree, I think we should go with simple way for now, and add special comment below block element. We can expand (maybe later?) this by checking if the element at end of selection is inline, ignore block element (if any) and everything selected above it, and apply comment only to inline part, as it's working right now.

theck13 commented 7 years ago

I almost have it, but I can't figure out the last part. The spans and HTML are inserted as expected, but the visual text isn't rendered correctly until I toggle the source view. Here's an example. I selected part of non-block text and part of block text, tapped the More button, tapped the HTML, then tapped the HTML button again.

test

Any thoughts?

khaykov commented 7 years ago

Hmmm. Hard to tell what it might be without seeing your implementation. Maybe there is some confusion about span index and where styling is removed. You are using setSelection to change index, and I think the error might hide somewhere there.

How about using static indexes, something like this:

fun applyComment(comment: AztecCommentSpan.Comment) {
        val commentStartIndex = selectionStart + 1
        val commentEndIndex = selectionStart + comment.html.length + 1

        editableText.replace(selectionStart, selectionEnd, "\n" + comment.html + "\n")

        removeInlineStylesFromRange(commentStartIndex, commentEndIndex + 1)

        val span = AztecCommentSpan(
                context,
                when (comment) {
                    AztecCommentSpan.Comment.MORE -> resources.getDrawable(R.drawable.img_more)
                    AztecCommentSpan.Comment.PAGE -> resources.getDrawable(R.drawable.img_page)
                }
        )

        editableText.setSpan(
                span,
                commentStartIndex,
                commentEndIndex,
                Spanned.SPAN_EXCLUSIVE_EXCLUSIVE
        )
    }

Btw, are you sure about replacing the text inside block element instead of just adding the comment to top/bottom?

theck13 commented 7 years ago

I should have described the implementation a little. I'm removing the block styling with a new function removeBlockStylesFromSelection which calls removeBlockStyle on all block formats as shown below. It's called before removing inline styles with removeInlineStylesFromRange(selectionStart, selectionEnd + 1).

    private fun removeBlockStylesFromSelection() {
        removeBlockStyle(TextFormat.FORMAT_ORDERED_LIST)
        removeBlockStyle(TextFormat.FORMAT_UNORDERED_LIST)
        removeBlockStyle(TextFormat.FORMAT_QUOTE)
    }

I thought about what the user expects in this scenario based on other behavior. If they have selected text, it is replaced with the special comment so I kept that behavior with block elements.

theck13 commented 7 years ago

The latest commit (https://github.com/wordpress-mobile/WordPress-Aztec-Android/pull/85/commits/cd1499f908b750ef2c61c0c9ad40011a5bcf91ec) uses the following behavior.

What do you think?

khaykov commented 7 years ago

Ok, looks like to make it work we will require a minor change to block removal logic. I'll try to come up with something and will make a PR to this branch.

theck13 commented 7 years ago

Your changes look good to me. I merged your branch into this one.

khaykov commented 7 years ago

Awesome. Maybe we could add tests for this case too, to not break anything by accident in a future.