Contain and retain GB on rotation

[hypest]

Fixes state loss issues on Gutenberg-mobile (example)

Implementation details

The Gutenberg-mobile editor is of course living inside a React Native view (ReactRootView). The solution here manages the View via a retained (headless) Fragment (GutenbergContainerFragment.java).

Especially when rotating, setContent() is bypassed so the content is not written again. It's not needed and it will also cause problems in the GB state.

To test:

• Open a Gutenberg post in the editor
• Play around, making changes to the post if desired
• Rotate the device. Notice how fast the rotation performs and, notice the post changes still be there, along with the undo/redo history.

Note: at the time of writing, the Travis build fails, apparently due to some trouble with Jitpack checking out the branch (Git error. Checkout conflict with file: Cannot delete file: react-native-aztec/example/ios). Opened ticket on JitPack here.

Update release notes:

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

[mzorz]

🥇 thanks @hypest for this 👏

This solution caters for the problem that was raised as concerning in the past: that the system can still kill the Activity for reasons other than rotation, and that would effectively make the undo/redo history lost for blocks, scroll positioning and such.

I think detecting mIsRotating flag is not enough here, as the Activity can be killed for reasons other than rotating.

For example, turn on the Don't keep activities setting on the developer options, and send the app to the background and bring it back to the foreground. You'll see the content doesn't get set at this point because the code flow thinks it's rotating, which is not good.

To prevent this and other potential problems we should make sure any re-initialization such as setContent is only bypassed on rotation. I first tried a fix by adding code into onConfigurationChanged but it's not only hard to test because well, first, with Don't keep activities enabled we always go through onCreate and would only get through onConfigurationChanged after the fact, but secondly and most important, found out actually we already have an onConfigurationChaned override there, that is never called due to the Activity not declaring to be handling any android:configChanges

According to this https://developer.android.com/guide/topics/resources/runtime-changes#HandlingTheChange

To declare that your activity handles a configuration change, edit the appropriate element in your manifest file to include the android:configChanges attribute with a value that represents the configuration you want to handle.
[...]
Now, when one of these configurations change, MyActivity does not restart. Instead, the MyActivity receives a call to onConfigurationChanged().

I think we can remove that piece of code (onConfigurationChanged entirely) even as part of this PR as it's not doing anything - or move the code elsewhere if merits.

There are more of these onConfigurationChaned overrides in other Activities in the app that I'm sure can be removed, but that's another story.

So, back to our thing: I think we may use a couple of options here (there may be more but these came to mind):

1. we could check the orientation in onCreate and keep a static variable to remember what the latest orientation was (tested this and seems to work, see diff here)
2. we could use the ComponentCallbacks2 listener already existing in WordPress.java, and implement some kind of interface/listener registry there (seems a bit more of work and adds some complexity).
3. A third option might be to just check for rotation change in onConfigurationChanged() in WordPress.java as per option 2 and when a change occurs then broadcast an event through EventBus and register to listen to it in GutenbergEditorFragment, thus setting the flag there. (similar to option 2 above but simpler to implement).

Does that make sense?

To summarize:

1. let's remove onConfigurationChanged() from GutenbergEditorFragment, given it's not being run at all.
2. for mIsRotating, let's implement one of the 3 solutions above (or, if you want to think of a fourth).

What do you think?

[hypest]

I just pushed 4cf21c8 @mzorz . With it, I took a kinda different approach:

I tried to understand how the setContent() logic works on rotation and all, and it seems that the editor fragment triggers it by signalling its parent with caused it EditorFragmentListener.onEditorFragmentInitialized(). It's a signal to the parent that the fragment got initialized so the parent can continue with finishing up the editor setup (including sending it the content and various media related subtasks).

So, I tried to suppress the signal, instead of detecting rotations and it seems to work. The "trick" is with the flag in the GB container fragment (mHasReceivedAnyContent), which denotes whether we've sent content to the editor's instance before. If we haven't, we should at some point but, if we have then we don't need to send the onEditorFragmentInitialized() signal when the activity rotates.

This seems to work and is quite contained as a solution. Granted, not the easiest to grok perhaps. Reason is that EditPostActivity and GutenbergEditorFragment are allowed to be recreated, but the container fragment only gets recreated in some cases.

Let me know what you think... thanks!

[hypest]

A new issue I've seen so far is that the post is marked as "modified" even by just entering and exiting the editor. Will work on it.

[hypest]

Turns out that the change detection trigger is due to a regression elsewhere (and it's in gutenberg-mobile develop), not related to this PR.

[hypest]

Updated from develop @mzorz , ready for another pass, thanks!

[mzorz]

TIL about commitNow(). Thanks

[mzorz]

This looks generally good code-wise, I've been testing and found an issue with content loss on this branch, that I also confirmed happens in develop as well (but making the mention here, to bring context):

If Don't keep activities is turned ON, and you open the Gutenberg editor and type something in it, then send the app to the background and bring it back to the foreground, the editor appears empty (the content is not saved / re-populated).

Made issue here #9148

Will continue testing and update here if anything 👍

[mzorz]

Do you think #9148 should be addressed in this PR @hypest ?

[hypest]

Nice catch @mzorz ! Confirmed the issue happening on this PR too.

Even though the issue is not introduced by this PR, I think we should fix it here. Reason is that, if we merge this PR, I expect some potential problems arising anyway (i.e. potential problems with media work that is currently in progress in parallel) so, I'd like to avoid disrupting the other, parallel efforts. #9148 is about lifecycle behavior so, it's not too far from the topic of this very PR anyway.

So, let's handle it here, cool?

[mzorz]

Agreed @hypest ! nice if we can handle it here 👍

[hypest]

Fixed the Fragment has not been attached yet crash with 0e2237f. It accesses the fragment directly via a cached reference to it.

[hypest]

Updated from develop and fixed known issues. This should be ready for another pass.

@daniloercoli , can you pick up reviewing this now that Mario is AFK? Let me know if you need anything to help with passing the baton. Thanks!

[daniloercoli]

Whoaa, nice solution implemented here! We had already discussed this over slack, voice and comments, and this seems the only viable solution that fixes many problems (redo/undo/caret position), even though it could seem a bit of hacky.

For me is a 👍 to this PR.

Not sure how much of the new media work was added back here, but during tests I found out that cannot add a media to a post with don't keep activities ON.

• Turn Don't keep activities ON
• Start a new post
• Add an image block and select a picture from device library or site library
• observe that no media is added

[hypest]

Thanks for the review pass Danilo!

I'll merge whatever is latest from all develops and will can try out media again to make sure nothing is broken!

Also, I have an additional fix for retaining the exact caret position (without any RN involvement for saving state and such) and I'll push it as well.

[hypest]

Added c6dee6a which points to a gutenberg-mobile + Aztec side fix for maintaining the exact caret position (inside Aztec) on rotation. Known issue: scroll position is not maintained though. I think we could address that in a separate PR.

Also, updated from develop.

... during tests I found out that cannot add a media to a post with don't keep activities ON.

I'm afraid that's also true for current develop too @daniloercoli , because we rely on a callback passed from RN to Java to set the media data after selecting it from the picker. But, the picker itself is a separate Activity and causes the EditPostActivity (along with the GB fragments) to get recreated. In that whole process, the callback reference is gone. We will need to revise our implementation to not rely on a callback. I think that's beyond the scope of this PR and we better handle it in a separate one.

WDYT?

cc @marecar3 since all RN-callback-based features (including media upload and such) can be affected or broken when "Don't Keep Activities" is ON.

[daniloercoli]

We will need to revise our implementation to not rely on a callback. I think that's beyond the scope of this PR and we better handle it in a separate one.
WDYT?

👍

Will do another round of testing with the fix for retaining the exact caret position.

[daniloercoli]

Run another round of tests on devices running Android 9 and Android 6 and this PR works as expected.

Although I noticed that the caret is not preserved on the Title block. The title block was recently moved to RN, but we're already in the middle of migrating it to use a new component under the hood (RichText- wordpress-mobile/gutenberg-mobile#513).
I think we can merge this PR, and iterate later, when the title block is migrated to RichText, if the problem with the caret is still there.

[hypest]

I think we can merge this PR, and iterate later

Agreed!

So, let's review/merge the gutenberg-mobile side PR first (wordpress-mobile/gutenberg-mobile#483), update this one and I can merge. Cool?

[hypest]

For the record, I tried the wordpress-mobile/gutenberg-mobile#513 against this and the focus is properly maintained in the title too. So, I think that issue will fix itself.