search

wordpress-mobile / AztecEditor-Android

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

Android 8.x dymanic layout crash preventer #801

Closed mzorz closed 5 years ago

mzorz commented 5 years ago

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 https://github.com/wordpress-mobile/WordPress-Android/issues/8828#issuecomment-474876535 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 https://github.com/wordpress-mobile/AztecEditor-Android/issues/729#issuecomment-430105339. 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:

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:

malinajirka commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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.

mzorz commented 5 years ago

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 79e17406 🙇 - thanks for the heads up!

mzorz commented 5 years ago

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 commented 5 years ago

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 :shipit: