Android 8.x dymanic layout crash preventer

[mzorz]

Fix

This PR tackles the symptom (not the culprit) identified as the cause in #729.
The culprit having been identified as laying in the lower level within the Android OS itself https://android-review.googlesource.com/c/platform/frameworks/base/+/634929, and for which a real solution means to fix it at that level (see wordpress-mobile/WordPress-Android#8828 (comment) for alternatives explored around a real solution to the specific problem), we can't do much about it.
As discussed elsewhere, I decided to try and approach the issue from the symptom point of view, and try and workaround the problem without affecting functionality.

The problem was narrowed down and isolated (symptom-wise) as per the following statement:

"the Editor crashes when inserting anything right before an image."

One of the proposed approaches was to just overwrite onDraw() (where the crash is about to happen) and just filter out any non-acceptable values (that is, anything below 0) for coordinates as suggested in #729 (comment). I think at that point however, it would mean that previous calculations were wrong for some reason, and while I haven't dove in too deep it might also mean some state variables (i.e. class members in DynamicLayout) could end up keeping a wrongly calculated state, thus generating another problem later on.

Due to these reasons, I decided a different approach would be to:

• a) first, see if we can programmatically detect the exact moment when the problem statement above is true
• b) then, somehow make the TextView to reflow / recalculate the view elements as appropriate.

One of the first ideas that came to mind was to make use of the already in place mechanism of TextWatchers to go ahead and make any needed changes. However, TextWatcher only is an observer that cannot affect the flow of events - that means that whatever is going to happen is still going to happen, you only get to listen to what's going on and only do something with it after the fact.

Then, we have InputFilter. This effectively is a mechanism in place that allows you to look into what's about to happen (text change) and decide on what should be replaced and what not.

I observed that initializing AztecText with actually anything before an Image is not a problem (only insertion is), so I thought "ok, what if we replace not the specific part that normally would get replaced, but make it look like a block and replace both the thing being modified and the Image together?".

Unfortunately that's not exactly possible from within an InputFilter. When an insertion happens, the Editable will call its whatever InputFilters have been set, passing them the place where the insertion needs to happen, and what the source string (or Spannable) to insert is. What this means is, even if we return the block of "inserted char" plus the Image right next to it, the whole block still will try to get inserted before the original image, causing the crash anyway.

What this PR does

So, we'd have to first keep a copy of the image span, then delete the original image span, then insert the block of "inserted text plus image" and, for the run of this inputFilter (when the case is first detected), just return an empty string, actually preventing the original change from happening. This is exactly what the InputFilter in this PR is doing and, so far so good, seems to be working.

We're effectively artificially re-creating the change as an insertion of "original insertion plus Image" and then making a substitution, if that makes sense.

Test

1. start the demo app
2. place the cursor at the beginning, right before the image
3. enter anything
4. observe it doesn't crash

Other things I've tested:

• test with other pluggable keyboards (tried 3rd party such as SwiftKey)
• test with different types of content in between image and insertion (heading, list, quote, another image, etc)
• test pasting paragraphs (not just typing characters)
• tested integration with WPAndroid (used these steps and can't reproduce)

[malinajirka]

Great job, thanks for the PR @mzorz! I'm behind a schedule so I won't have time to review it today. I'll make sure to review it on Monday/Tuesday.

[daniloercoli]

Hi @mzorz 👋 - Would you mind to delete the branch issue/729-dymanic-layout-crash-preventer on the remote site if it's not required?
There are chances that reviewers will pick it up, instead the correct one ;)

[daniloercoli]

Quickly tested on Android 8 and it does fixes the problem, I need to test it more extensively though.

In the meanwhile, I noticed that once the image is deleted and re-inserted the "Media deleted" Toast message " is shown twice on the screen. I guess the delete callback is called on the Media obj.

[daniloercoli]

Is it possible to move this check before val spans = dest.getSpans(dstart, dend+1, AztecImageSpan::class.java) ?
It should improve performances a bit, considering that this filter is called at any key pressed.

[mzorz]

gotcha, nice improvement, made change as suggested in 1ff08ac

[daniloercoli]

Can we move this check if (dest.length > dend+1 as first line in the filter, and do not create the temp variable since it's not needed in that case?

[mzorz]

It wasn't actually creating a new variable (the object referenced to by temp and source were the same), but nevertheless thanks to your comment I looked into the docs again, and made an adjustment in 59ff4c0 as per the docs (initializing temp with null by default so the original change gets accepted if we don't need to make any other changes here).

Then we only override it in the case we actually go forward with the text change, by making temp = "" in this other line , to actually discard the original change.

Ref: InputFilter docs

[mzorz]

Would you mind to delete the branch issue/729-dymanic-layout-crash-preventer on the remote site if it's not required?
There are chances that reviewers will pick it up, instead the correct one ;)

oops sorry :D just deleted it - also this:

In the meanwhile, I noticed that once the image is deleted and re-inserted the "Media deleted" Toast message " is shown twice on the screen. I guess the delete callback is called on the Media obj.

hah, I forgot to copy the code I put in the first experimental branch, that takes care of this. I put the code back in commit 79e1740 🙇 - thanks for the heads up!

[daniloercoli]

Nice work @mzorz 🔆 LGTM!
Wondering if it's necessary to test this PR on wp-android, since the Calypso mode should be ON there. I guess there are no differences, but a quick check would be nice.

[mzorz]

Nice work @mzorz 🔆 LGTM!

Thank you @daniloercoli 😄 ! Glad you think it's worth merging 🙇

Wondering if it's necessary to test this PR on wp-android, since the Calypso mode should be ON there. I guess there are no differences, but a quick check would be nice.

Good idea, I've tested it there before and found no problems, and just tested it again with latest commit and seems to be working alright as well :) - also I think Gutenberg won't be a problem as well given we're not using text and images combined in a single Aztec instance so, should be good to go.

Thanks for the review @daniloercoli !

[malinajirka]

Thanks for the fix @mzorz! I've tested several different use cases and everything works like a charm, great job!!!

I'm still not sure the Editor crashes when inserting anything right before an image is the only case when the editor crashes, but it's definitely the only case we are able to reproduce. So it'll be interesting to see whether we fix all the crashes or just some.

LGTM