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

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

Merged
merged 6 commits into from
Nov 30, 2018

Conversation

daniloercoli
Copy link
Contributor

@daniloercoli daniloercoli commented Nov 29, 2018

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

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

daniloercoli commented Nov 29, 2018

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

daniloercoli commented Nov 30, 2018

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.

…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)
@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
…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
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
Development

Successfully merging this pull request may close these issues.

4 participants