Contain and retain GB on rotation

hypest commented 5 years ago

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:

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

mzorz commented 5 years ago

🥇 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:

let's remove onConfigurationChanged() from GutenbergEditorFragment, given it's not being run at all.
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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

mzorz commented 5 years ago

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 https://github.com/wordpress-mobile/WordPress-Android/issues/9148

Will continue testing and update here if anything 👍

mzorz commented 5 years ago

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

hypest commented 5 years ago

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 commented 5 years ago

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

hypest commented 5 years ago

Added https://github.com/wordpress-mobile/gutenberg-mobile/pull/483/commits/cb025c85917829a6d72f353275b6a8c98e7bb871 on the glue side to remove the special init for new posts. I think that fixes #9148 . Can you check when you get the chance @mzorz ? Thanks!

mzorz commented 5 years ago

Added wordpress-mobile/gutenberg-mobile@cb025c8 on the glue side to remove the special init for new posts. I think that fixes #9148 .

It seems this effectively makes #9148 go away 👏

One thing though, that I haven't been able to reproduce but saw at least once:

2019-01-31 14:55:01.486 1105-1379/? V/AudioService.RotationHelper: publishing device rotation =1 (x90deg)
2019-01-31 14:55:01.490 1105-1321/? I/InputReader: Reconfiguring input devices.  changes=0x00000004
2019-01-31 14:55:01.490 1105-1321/? I/InputReader: Device reconfigured: id=5, name='synaptics_dsxv26', size 1080x1920, orientation 1, mode 1, display id 0
2019-01-31 14:55:01.491 1105-1135/? I/ActivityManager: Config changes=480 {1.0 722mcc34mnc [en_US,es_AR,sl_SI,de_DE,agq_CM] ldltr sw411dp w683dp h387dp 420dpi nrml widecg land finger -keyb/v/h -nav/h appBounds=Rect(0, 0 - 1794, 1080) s.477}
2019-01-31 14:55:01.501 1105-1135/? I/ActivityManager: Override config changes=480 {1.0 722mcc34mnc [en_US,es_AR,sl_SI,de_DE,agq_CM] ldltr sw411dp w683dp h387dp 420dpi nrml widecg land finger -keyb/v/h -nav/h appBounds=Rect(0, 0 - 1794, 1080) s.477} for displayId=0
2019-01-31 14:55:01.536 22826-22826/? I/EmojiSearchExtension: onDeactivate()
2019-01-31 14:55:01.539 26630-26630/org.wordpress.android I/WordPress-STATS: 🔵 Tracked: editor_closed
2019-01-31 14:55:01.548 26630-28081/org.wordpress.android E/AndroidRuntime: FATAL EXCEPTION: Thread-162
    Process: org.wordpress.android, PID: 26630
    java.lang.IllegalStateException: Fragment has not been attached yet.
        at android.support.v4.app.Fragment.instantiateChildFragmentManager(Fragment.java:2300)
        at android.support.v4.app.Fragment.getChildFragmentManager(Fragment.java:763)
        at org.wordpress.android.editor.GutenbergEditorFragment.getGutenbergContainerFragment(GutenbergEditorFragment.java:80)
        at org.wordpress.android.editor.GutenbergEditorFragment.getContent(GutenbergEditorFragment.java:379)
        at org.wordpress.android.ui.posts.EditPostActivity.updatePostObject(EditPostActivity.java:1453)
        at org.wordpress.android.ui.posts.EditPostActivity.access$900(EditPostActivity.java:192)
        at org.wordpress.android.ui.posts.EditPostActivity$8.run(EditPostActivity.java:1472)
        at java.lang.Thread.run(Thread.java:764)
2019-01-31 14:55:01.553 22826-28082/? I/DlamWrapper: close(): Successfully wrote DLAM properties file.
2019-01-31 14:55:01.558 1105-2549/? W/ActivityManager:   Force finishing activity org.wordpress.android/.ui.posts.EditPostActivity

I was taking notes so I know what I did, but couldn't reproduce even following the steps. Steps for me were:

turn Don't keep activities ON
start a new draft
enter a title (no body)
send the app to the background (tap home button)
rotate the device (observe the main Android screen won't rotate)
tap on the right button to bring the list of recent apps
choose WP android app (in my phone I have to also touch once on the center of the screen to make the actual list show up)
rotate the device
observe crash

Even though I'm finding trouble to reproduce, I can see how trying to getContent() from an unattached fragment is something that may be requested on different situations. I don't know what's best here, maybe delay the execution for a bit until we know it's attached? wdyt @hypest ?

hypest commented 5 years ago

Thanks for the extra pass @mzorz !

Yeap, I have seen that crash too and indeed is not reproducible all the time. I think it's a timing issue.

It does make sense to be problematic to rely on the fragment to be there, as you say. The good thing is that this crash feels quite contained in the way it can happen and it will hopefully be "easy" to tackle. I've been pondering upon this and I have an idea to try out. I think it would be OK to continue working on it in a follow up PR. Let me know if you think this is a blocker instead.

mzorz commented 5 years ago

I've been pondering upon this and I have an idea to try out. I think it would be OK to continue working on it in a follow up PR.

@hypest sure thing, I see the point of going forward with this PR as it tackles something greater, and then focusing on this as an isolated issue.

👍

hypest commented 5 years ago

Oops, had basically forgotten there my preference to wait merging this PR until the media work done in parallel is landed first. So, while waiting, I'll work on that crash 👍.

mzorz commented 5 years ago

Media work done and merged in https://github.com/wordpress-mobile/WordPress-Android/pull/9158, so, after merge conflicts are resolved this should be ready to go 👍 @hypest thank you for your deep analysis and smart solution here! 🙇

hypest commented 5 years ago

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

hypest commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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- https://github.com/wordpress-mobile/gutenberg-mobile/pull/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 commented 5 years ago

I think we can merge this PR, and iterate later

Agreed!

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

hypest commented 5 years ago

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

wordpress-mobile / WordPress-Android

Contain and retain GB on rotation #9030

Implementation details