Skip to content

[RNmobile] Fix problems with undo/redo on Android#12417

Merged
gziolo merged 6 commits intomasterfrom
rnmobile/danilo-try-to-fix-undo-redo
Nov 30, 2018
Merged

[RNmobile] Fix problems with undo/redo on Android#12417
gziolo merged 6 commits intomasterfrom
rnmobile/danilo-try-to-fix-undo-redo

Conversation

@daniloercoli
Copy link
Copy Markdown
Contributor

@daniloercoli daniloercoli commented Nov 29, 2018 •
edited
Loading

This PR fixes a problem where the undo/redo feature was not working on Android if small changes were made to the content.

Actually undo/redo feature was not working fine even if there were lot of changes to the content, but in this case the problem was less noticeable, since the undo/redo feature replaced the content with the wrong text.

The problem was that RichText props were not correctly updated on typing. Then on undo the RichText component already had the same "old" value stored in it, and the Native side was not refreshed at all (The setText method of the AztecWrapper was not invoked).

To test this PR follow the steps in the gb-mobile PR here:wordpress-mobile/gutenberg-mobile#297

@daniloercoli daniloercoli mentioned this pull request Nov 29, 2018
Merged
mzorz
mzorz approved these changes Nov 29, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@mzorz mzorz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested, explanation and code make sense, and was able to confirm it fixes the behavior it intends to fix.
I've seen the e2e tests are failing due to reasons unrelated to this PR.
LGTM :shipit: feel free to merge as soon as those get fixed

@mzorz
Copy link
Copy Markdown
Contributor

mzorz commented Nov 29, 2018

I noticed while debugging and setting a breakpoint in shouldComponentUpdate() in rich-text that in this branch all RichText components return true in shouldComponentUpdate(), even if no change happened to them (that is, they are not the focused block or the block where changes are happening). It doesn't make sense to change the state of every single block instance in the list when a simple change happened in one block only.

I couldn't think of a solution for it, so leaving the comment here for now.

@daniloercoli
Copy link
Copy Markdown
Contributor Author

daniloercoli commented Nov 29, 2018 •
edited
Loading

That's right @mzorz, but the shouldComponentUpdate is only called on those blocks rendered on the screen. Since we're using a FlatList, only few blocks are loaded on the screen/memory at the same time, there should no big performance impact on re-rendering those.
Also consider that eventCount is sent together with the content to the Native side, and used there to block the refresh of the native AztecWrapper component when it's not needed (I guess it could be undefined if you don't modify the block).

I agree with you that a better solution is required, but for the alpha I think it's ok, and well tested since returning true very often :) Just need to verify performance problems, since the whole shouldComponentUpdate is there to avoid those bottlenecks we were seeing in aztec-wrapper.

@daniloercoli
Copy link
Copy Markdown
Contributor Author

daniloercoli commented Nov 30, 2018 •
edited
Loading

I did another test this morning to confirm that the native side is only hit (setText on the AztecWrapper called) for the one single block where the changes happens. This is because for the other blocks no props are changed and React doesn't trigger the re-render (render is called React side but does't update the component).

If you want to test this put a breakpoint on the native side setText method, and see that it is only called once on redo/undo.

@daniloercoli
Merge branch 'master' of https://github.com/WordPress/gutenberg into …
231a090 
…rnmobile/danilo-try-to-fix-undo-redo

* 'master' of https://github.com/WordPress/gutenberg:
  Autocompleters: Consider block category (#12287)
  Only init TinyMCE once per instance (#12386)
  RichText: convert HTML formatting whitespace to spaces (#12166)
  Notices: Remove "files" block in package.json (#12438)
  Edit Post: Avoid rendering AdminNotices compatibility component (#12444)
  Correct the docs manifest (#12411)
@daniloercoli
Add TODO item that better explains next steps to fix/remove re-render…
fa68d2e 
…ing when not needed
@gziolo gziolo added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Nov 30, 2018
@gziolo gziolo added this to the 4.7 milestone Nov 30, 2018
@gziolo gziolo merged commit 77dcd00 into master Nov 30, 2018
@gziolo gziolo deleted the rnmobile/danilo-try-to-fix-undo-redo branch November 30, 2018 09:51
daniloercoli added a commit that referenced this pull request Nov 30, 2018
@daniloercoli
Merge branch 'master' of https://github.com/WordPress/gutenberg into …
bb93c4a 
…HEAD

* 'master' of https://github.com/WordPress/gutenberg:
  [RNmobile] Fix problems with undo/redo on Android (#12417)
  Add registry param to withDispatch component (#11851)
  Autocompleters: Consider block category (#12287)
  Only init TinyMCE once per instance (#12386)
  RichText: convert HTML formatting whitespace to spaces (#12166)
  Notices: Remove "files" block in package.json (#12438)
  Edit Post: Avoid rendering AdminNotices compatibility component (#12444)
  Correct the docs manifest (#12411)
@mtias mtias modified the milestones: 4.7, Mobile Alpha Dec 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mzorz mzorz mzorz approved these changes

Assignees

No one assigned

Labels

Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change)

Projects

None yet

Milestone

Mobile Alpha

Development

Successfully merging this pull request may close these issues.

4 participants

@daniloercoli @mzorz @mtias @gziolo