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

Keep media inside BlockSpan when at buffer end #668

Merged

Conversation

hypest
Copy link
Contributor

@hypest hypest commented May 8, 2018

Additional PR to fix the issue reported at #666 (comment)

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

@hypest hypest mentioned this pull request May 8, 2018
@mzorz
Copy link
Contributor

mzorz commented May 9, 2018

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 #666 (comment) 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 #666 (comment) 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
Copy link
Contributor Author

hypest commented May 9, 2018

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

@daniloercoli
Copy link
Contributor

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
Copy link
Contributor Author

hypest commented May 9, 2018

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
Copy link
Contributor

mzorz commented May 9, 2018

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
Copy link
Contributor Author

hypest commented May 9, 2018

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

@daniloercoli daniloercoli self-assigned this May 9, 2018
@daniloercoli
Copy link
Contributor

:shipit:

@daniloercoli daniloercoli merged commit 53e54b3 into issue/659-gb-blocks May 9, 2018
@daniloercoli daniloercoli deleted the issue/659-gb-blocks-img-insertion-fix branch May 9, 2018 13:45
@koke koke added this to In progress in Aztec-Gutenberg compatibility via automation Jun 14, 2018
@koke koke moved this from In progress to Done in Aztec-Gutenberg compatibility Jun 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants