wordpress-mobile / AztecEditor-Android

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

Keep media inside BlockSpan when at buffer end #668

Closed hypest closed 6 years ago

hypest commented 6 years ago

Additional PR to fix the issue reported at https://github.com/wordpress-mobile/AztecEditor-Android/pull/666#issuecomment-387121262

It seems that the media insertion code doesn't properly account for the buffer end position and ends up appending the media outside the BlockSpan. This PR adds a check for the buffer end.

While at it, added a fix for the cursor position issue when switching between html and visual mode as mentioned by Mario. BTW, that cursor issue is present in current develop too.

cc @daniloercoli , @mzorz

mzorz commented 6 years ago

Thank you @hypest! thanks for throwing light on how end of buffer should work (I actually learn with the explanations and the minimum code changes here helps me understand a lot better how spans are meant to be used). I tested and could observe this fix does indeed solve the end of buffer edge case 🎉 We still have the problems in the example mentioned in https://github.com/wordpress-mobile/AztecEditor-Android/pull/666#issuecomment-387491813 though

screen shot 2018-05-08 at 23 31 40 screen shot 2018-05-08 at 23 30 20

As seen here, the starting Gutenberg delimiter comment is lost when inserting the image at the beginning of text.

But good news is that seems to be fixed by adding a similar check like this following one:

selectionStart == editableText.getSpanStart(it) && (selectionStart != 0)

in line 147 of LineBlockFormatter.kt.

With that change there, I could confirm both the original issue #659 and the ones being exposed in https://github.com/wordpress-mobile/AztecEditor-Android/pull/666#issuecomment-387491813 and here above are gone (at least by following the provided steps to reproduce by the letter).

Would be good to see and run the tests @daniloercoli is working on in this branch (with that requested change in line 147) and maybe we can take it from here.

hypest commented 6 years ago

Made the change you mentioned @mzorz (bedae13), good call.

daniloercoli commented 6 years ago

I'm a bit confused about all of these edges cases honestly. It seems that the Spans approach is easily error prone in general; this comment is not meant for this PR, but it's a general consideration.

Back to the code, I'm running into a situation where adding a picture at the beginning of the 2nd paragraph ends up stripping GB code from it.

screen shot 2018-05-09 at 12 26 22 screen shot 2018-05-09 at 12 26 36 screen shot 2018-05-09 at 12 27 00 screen shot 2018-05-09 at 12 27 10

hypest commented 6 years ago

I noticed yesterday as I was investigating the root cause that insertMedia() takes a rather simplistic approach when adding media: it only tries to shift around the first BlockSpan it finds. I think that doesn't work correctly in the general case when BlockSpans are nested. I will look into an idea I was having, to try and shift the innermost BlockSpan, not the first found, though I believe that even that is simplistic and media insertion needs to be depended on the specific blockspans (e.g. lists might need to behave differently than other blockspans).

All in all, this doesn't seem to be an issue of the GB comments handling anyway.

mzorz commented 6 years ago

I think that doesn't work correctly in the general case when BlockSpans are nested.

I thought about the same (probably in a rather broader or superficial way than you can do) yesterday, and also that thought linked to nested Gutenberg Blocks, which are going to be there some time in the future.

I'm a bit confused about all of these edges cases honestly. It seems that the Spans approach is easily error prone in general; this comment is not meant for this PR, but it's a general consideration.

I must admit I've been approaching the general span-based PRs in a more "let's make it fail" and "gotcha" mood than anything else 😉 but when testing and trying to understand it makes sense to follow the spans architecture overall, as it's the core mechanism Aztec has to understand "what's where" in visual mode without having to keep a "map" up to date elsewhere.

hypest commented 6 years ago

Added ebcb60e that attempts to fix the media insertion issues reported so far.

daniloercoli commented 6 years ago

:shipit: