wordpress-mobile / AztecEditor-Android

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

Fix for History slowness #615

Closed daniloercoli closed 6 years ago

daniloercoli commented 6 years ago

This PR improves the way we're storing visual editor content in the History class (to be used later for undo/redo). Before this PR the visual editor stored its content into History almost on each key pressed, and the call to retrieve the HTML code from it required some time, resulting in UI lags on devices.

In details:

I run all the tests on this PR without any problem, but I guess the History now, that I changed the lag to 900ms, could be less reliable than before (since we're rely on the lag time only to execute the code that stores content in history stack). In any case the lag on my devices is WAYYY less noticeable now.

cc @mzorz Ready fro review.

mzorz commented 6 years ago

I run all the tests on this PR without any problem, but I guess the History now, that I changed the lag to 900ms, could be less reliable than before (since we're rely on the lag time only to execute the code that stores content in history stack).

Code looks good, and been testing with demo app content mutiplied 10 times and works much better than before - note that so many things are deleted in 900 ms while leaving the DELETE key pressed, that when you tap on UNDO, it takes a while for it to finish running, leaving the "pressed" overlay on the arrow icon and being unresponsive for just a bit.

I tried changing it back to 500 ms and it felt slightly better in that sense (although it does still present the "frozen for a bit" problem, which I think has more to do with the total amount of content that is re-set each time, rather than the amount of changes / deletions / inserts), what do you think about leaving it in 500ms but still merge all the other code changes in @daniloercoli ?

daniloercoli commented 6 years ago

Yes, I had noticed that too. Moving it to 900ms was one of my first tentative on fixing the problem, before changing other/more stuff. You can switch back it to 500ms.

mzorz commented 6 years ago

Also I noticed that we can get away with the UNDO / REDO icons not getting freezed if we do something like this in fun undo(editText: EditText):

        mainHandler.postDelayed(object : Runnable {
            override fun run() {
                // actual undo() code here
            }
        }, 200)

And the same for redo().

(The content manipulation would still lag as observed, but that's a matter for another discussion / PRs)

daniloercoli commented 6 years ago

Switched the delay back to 500ms.

Didn't applied the other changes to make the undo/redo icons responsive, since it was not a so obvious fix, and probably belongs to another PR. Note the code that actualy does the undo/redo needs to access the UI and get the focus on UI fields. Not sure we can run it in a bg thread.

mzorz commented 6 years ago

Note the code that actualy does the undo/redo needs to access the UI and get the focus on UI fields. Not sure we can run it in a bg thread.

That's ok we'd be using mainHandler which is taken from the main loop. I tested it and worked alright, but we can put another PR with that if you want

mzorz commented 6 years ago

LGTM :shipit:

daniloercoli commented 6 years ago

That's ok we'd be using mainHandler which is taken from the main loop. I tested it and worked alright, but we can put another PR with that if you want

Right, I noticed that, but wondering why we want to make the buttons responsive if the editor is unusable for a while, until the runnable that does the undo/redo is executed? It's because it feels more faster?

mzorz commented 6 years ago

Right, I noticed that, but wondering why we want to make the buttons responsive if the editor is unusable for a while, until the runnable that does the undo/redo is executed? It's because it feels more faster?

I used my brain for the past 10 minutes and realized what you meant - you're right there's no point in trying to make things "feel" faster when they aren't. Making the button responsive won't make Aztec responsive. Comment dismissed!