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

Exclude EditPostActivity from handling orientation change for alpha #8800

Closed
wants to merge 1 commit into from

Conversation

mzorz
Copy link
Contributor

@mzorz mzorz commented Dec 17, 2018

Fixes wordpress-mobile/gutenberg-mobile#377, #8739, and load time issues mentioned in #8739 (comment) and elsewhere

This PR adds a specific AndroidManifest.xml merge for EditPostActivity, declaring the following attributes:
keyboard|keyboardHidden|orientation|screenSize. What this effectively means is that EditPostActivity now does not handle orientation changes, and lets the changes in configuration be listened to and handled by the underlying RN implementation.

Been looking into this and trying different approaches:

  • first one the natural/default approach being actually handling the orientation change ourselves which would mean to do all kinds of artifacts to keep undo/redo history for each block + keeping the scroll and caret positioning stated to re-set those on re-render. But this meant not only a lot of code and maintenance problems, but also a time overhead in re-loading Post on each time the Activity would be re-created.
  • While into this, realized the common problem was that the RN setup + loading of the Post took too long, so ideally we would need to make the Post available on rotation right away. Given most of our code lives in GutenbergEditorFragment as a host, a first idea was to use setRetainInstance(true) (see Avoid Gutenberg reload times on rotation #8790 and Keeps reference to mReactInstanceManager to avoid re-creating on rotation gutenberg-mobile#382) and keep the fragment's instance alive with the RN code alive as well. Unfortunately, even by doing this meant the re-wiring needed was not trivial, and even if so, the RN lifecycle relies heavily on listening to the host's Activity lifecycle # #, apparently re-initializing things internally as the Activity lifecycle wired events were run (so, keeping the Fragment instance wasn't really helping here).
  • Looked into the demo apps, realized they were using the orientation|screenSize modifiers in the orientation attribute, which indicates the Activity is in charge of handling orientation changes (and thus it not going to be destroyed for this reason). Also searching a bit for it found out most of the work is handled by RN itself on one activity, so it makes sense generally speaking to keep the Activity instance as well #.

Given the situation in which the user is writing has the EditPostActivity on the foreground most of the time, an orientation change losing the focus / scroll position or making the user wait to reload seems less than ideal. On the contrary, if the user is writing and switches the app to do something else, then it perfectly makes sense for it to occasionally need to re-load the content.
This seems a good solution for the situation we're trying to handle, which aims at recognizing the orientaiton change (which RN does well and adjusts the contents to fit the new screen size after rotation) while not affecting the writing flow (which until this PR meant waiting for reload, losing scroll position and undo/redo history among other hurdles).

Note: this PR makes the EditPostActivity not be killed on screen orientation changes also for Aztec and Visual Editors as the implementation for each is done by means of Fragments, but I chose to keep the PR simple for alpha only as opposed to making a new EditPostActivity class only for Gutenberg. We'll have to create one though, once we get into Beta.

To test:

  1. load up a long Gutenberg post
  2. scroll somewhere, place the caret at a specific location
  3. rotate the device
  4. observe the caret remains there
  5. rotate again
  6. observe position and caret is kept
  7. do some edits and rotate
  8. observe the undo/redo history is kept as well.

Update release notes:

  • If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

Copy link
Contributor

@marecar3 marecar3 left a comment

Choose a reason for hiding this comment

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

Tested and it seems that it's working good 👍

While I was working on wordpress-mobile/gutenberg-mobile#377
and trying different solutions I had exactly the same thoughts as you had, so it seems that this solution is fair enough for now.

@mzorz
Copy link
Contributor Author

mzorz commented Dec 18, 2018

Thanks for the review @marecar3 - as discussed elsewhere we're deferring it for mobile Gutenberg Beta to allow for more testing on our side, as concerns were expressed regarding Aztec behavior being affected.

@mzorz
Copy link
Contributor Author

mzorz commented Dec 20, 2018

From what has been discussed we can derive we're not leaning towards risking changing EditPostActivity as a whole (for all editors) but rather will have an isolated Activity to handle Gutenberg's RN needs to cope with orientation changes. This short PR made sense in the scarce time to launch Alpha which is happening now so, closing this one.

@mzorz mzorz closed this Dec 20, 2018
@mzorz mzorz deleted the fix/gutenbeg-editor-keep-activity branch December 20, 2018 14:26
@mzorz mzorz mentioned this pull request Dec 26, 2018
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants