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

Serializing Story in onSaveInstanceState, retrieving it in onCreate #435

Merged
merged 24 commits into from
Jul 19, 2020

Conversation

mzorz
Copy link
Contributor

@mzorz mzorz commented Jul 8, 2020

Builds on top of #414

This PR aims at utilizing the kotlinx.serialization framework introduced in #414 to serialize/deserialize the WIP Story currently living in the StoryRepository when the Activity enters a paused state and gets its onSaveInstanceState lifecycle method called.

General

This PR basically serializes and saves the current Story state in onSaveInstanceState(), and inflates it back in onCreate() when savedInstanceState != null, initializing and populating the ComposeLoopFrameActivity, BackgroundSurfaceManager, StoryViewModel, and StoryRepository back from the previously serialized state.

BackgroundSurfaceManager & related changes

When the Acivity is re-created, we need to wait for Surface initialization properly before we can draw video / camera live preview on it. For that we're now making use of the BackgroundSurfaceManagerReadyListener, and there are also some other related changes as followsL

  • 68470c3 (checking isVideoPlayerVisible for proper video handler fragment initialization)
  • added methods launchCameraPreviewWithSurfaceSafeguard() and showPlayVideoWithSurfaceSafeguard (see
    1a70555 and later addition 436946a), if the a video player or live camera preview needs be started but surface is not ready yet, we just turn a flag and retrieve it once the surface is ready to play. This is also an improvement over the past mechanism where we had an artificial wait of 500ms to launch the players. It's now more tightly done (no needless wait if it's ready before 500ms) and proves to have a consistent behavior across devices and execution conditions.

Serializers

The serializers per se live in StorySerializerUtils, but there's not much there. Annotations and custom serializers have been added to the following classes:

  • Story
  • StoryFrameItem, StoryFrameItemType
  • BackgroundSource
  • Uri / File serializers
  • AddedViewList
  • AddedView
  • SaveResultReason

This last one's possible values SaveSuccess and SaveError inherit from two classes (its parent class are naturally SaveResultReason, and also Parcelable given we need that to include it in intent bundles and EventBus). As such, the serializer needs to be declared a Module that tells it which actual serializer to use when it finds an instance of a final class (see https://github.com/Kotlin/kotlinx.serialization/blob/master/docs/polymorphism.md#basic-case). In particular this is also a sealed class, which would otherwise not need a custom module since version 0.14 (https://github.com/Kotlin/kotlinx.serialization/blob/master/docs/polymorphism.md#sealed-classes) of the kotlinx-serializaiton plugin (given sealed classes are known of all its possible values at compile time), but given we're also making it Parcelable (and hence inherits from more than one class) we need to use the polymorphic() constructor for the serializer and pass each specialized serializer for each of the sealed class final types.

Other

  • when the Activity got destroyed and then re-created, the working area for the full screen mode (that is, the area where the user can add emoji, text, and move freely around) would then need to be re-calculated. We added getWorkingAreaRect method to the interface so we calculate once and make sure not to have an old copy in PhotoEditor. (8f65a05)
  • in buildViewFromAddedViewInfo() added in the previous PR we forgot to add the touch listener so added views that are reconstructed from metadata (AddedViewInfo) can also be handled with touch. (cac37be)

To Test

CASE A: camera live preview
1. on settings -> advanced -> developer option, set Don't keep activities to ON
2. tap the + icon
3. observe the camera preview mode appears
4. send the app to the background
5. return to the app
6. observe the camera live preview is shown correctly and you can capture a still image or a video.
EDIT: N/A, this PR doesn't contain code to address this situation in particular (we care for serializing / deserializing a Story, there's no code in here to keep the live preview state)

CASE B: remember a Story with added views, with static image being selected

  1. on settings -> advanced -> developer option, set Don't keep activities to ON
  2. tap the + icon
  3. observe the camera preview mode appears. Capture a still image.
  4. on the captured image, add some emoji / text, and move it around
  5. send the app to the background
  6. bring it back to the foreground
  7. observe the state is re-created as you left it, and you can also touch and drag / resize the added views (emoji, text) as well

CASE C: remember a Story with added views, with static image being selected, added from the Media Picker.

  1. on settings -> advanced -> developer option, set Don't keep activities to ON
  2. tap the + icon
  3. observe the camera preview mode appears. Capture a still image.
  4. now you'll see the story composer. Tap on the + icon on the bottom strip to launch the media picker.
  5. select one or more images.
  6. on the selected story frames, add some emoji / text, and move it around
  7. send the app to the background
  8. bring it back to the foreground
  9. observe the state is re-created as you left it, and you can also touch and drag / resize the added views (emoji, text) as well
  10. if you added more than one Story frame, observe you can select different frames and all of them keep their emoji / text on the places, sizes, and rotation as you left them.

You should be able to do this as many times as you want.

CASE D: remember a Story and trigger video playing as soon as it's available

  1. on settings -> advanced -> developer option, set Don't keep activities to ON
  2. tap the + icon
  3. observe the camera preview mode appears. Capture a video.
  4. now you'll see the story composer, with the video you just recorded being played in a loop.
  5. optional: mute / unmute
  6. send the app to the background
  7. bring it back to the foreground
  8. observe the video starts playing seamlessly as you left it.

CASE E: added views on video

  1. follow steps in Case D up til step 4 inclusive.
  2. add some views, rotate, zoom in, etc.
  3. send the app to the background
  4. bring it back to the foreground
  5. observe the video starts playing seamlessly as you left it, and your emoji/text appear in the same way you left them.

CASE F: many items

  1. add many items from the media picker, from camera capture, etc.
  2. add views
  3. send the app to the background, bring it back to the foreground
  4. confirm you can see and switch through all the frames and added views are still kept on the same places as you left them.

CASE G: use WPAndroid

@mzorz mzorz changed the title serializing Story in onSaveInstanceState, retrieving it in onCreate Serializing Story in onSaveInstanceState, retrieving it in onCreate Jul 8, 2020
@peril-automattic
Copy link

peril-automattic bot commented Jul 8, 2020

You can test the changes on this Pull Request by downloading the APK here.

mzorz added 10 commits July 13, 2020 20:29
… we can update the working area rect to adjust multitouchlistener workarea even after adding deserialized views
…edia picker, let onStoryFrameSelected do the handling
…ion, but cancelling them when adding images from the picker as its taken care of by updateBackgroundSurfaceUIWithStoryFrame
…VideoWithSurfaceSafeguard() to sync and wait for Surface to be ready before attempting camera live preview / video playing, and removed artificial wait time
…oach now and wait for the BackgroundSurfaceManagerReadyListener listener to trigger camera / video playing
mzorz added 2 commits July 16, 2020 11:31
…ays check TextureSurface availability from a single source of truth, ditched surfaceReady boolean class member which could hold non up-to-date information
@jd-alexander
Copy link
Contributor

Hi @mzorz quick question for CASE A: camera live preview the camera preview should be shown when we resume the app right? I tried to do that but didn't get it to work. Observe the gif below and let me know your thoughts.

@mzorz
Copy link
Contributor Author

mzorz commented Jul 18, 2020

quick question for CASE A: camera live preview the camera preview should be shown when we resume the app right? I tried to do that but didn't get it to work. Observe the gif below and let me know your thoughts.

This is interesting, I wonder whether I described the case I wanted to test after all! Let me look into it and get back here. Please continue with the other cases for now 🙏

@jd-alexander
Copy link
Contributor

jd-alexander commented Jul 18, 2020

This is happening on my Google Pixel 4 XL, Android 10 while testing Case C. When Do not keep activities is on, the app goes back to the main activity when the images are selected in the MediaPicker. See video below.

Reproduction steps:

  1. Click the plus (+) button. Click the Upload button.
  2. Select images.
  3. See the app back at the main screen.

https://drive.google.com/file/d/1KWBSYRDqyX22evz2qxksgR48tRtZrDWX/view?usp=sharing

Note: This can also be reproduced on the emulator as well 🙏

@mzorz
Copy link
Contributor Author

mzorz commented Jul 18, 2020

hey @jd-alexander re: this comment sorry for that, edited the PR description now with the following: EDIT: N/A, this PR doesn't contain code to address this situation in particular (we care for serializing / deserializing a Story, there's no code in here to keep the live preview state). I think I had my test cases listed on the side and then copied them as they were. 🙏

@jd-alexander
Copy link
Contributor

hey @jd-alexander re: this comment sorry for that, edited the PR description now with the following: EDIT: N/A, this PR doesn't contain code to address this situation in particular (we care for serializing / deserializing a Story, there's no code in here to keep the live preview state). I think I had my test cases listed on the side and then copied them as they were. 🙏

okay no problem 😄

@jd-alexander
Copy link
Contributor

Hey @mzorz not sure if I have ever shown you this or if there's an issue tracking it but noting here since it occurred during testing; the video becomes stretched at times when putting and bringing the app out of the foreground. Let me know what you think.

The reproduction steps are just adding a video as a story frame and then doing the app in and out of the foreground to catch it.

@mzorz
Copy link
Contributor Author

mzorz commented Jul 18, 2020

This is happening on my Google Pixel 4 XL, Android 10 while testing Case C. When Do not keep activities is on, the app goes back to the main activity when the images are selected in the MediaPicker. See video below.

The upload icon is going to be removed shortly as per #442 given it's causing some other navigation issues. The only way to call the picker should be by tapping the + icon on the story slide selector once you have at least one image selected.
Can you check if the following works for you? (pretend for a moment the upload icon to the left of the camera shutter control didn't exist):

  1. tap + to get the live camera preview
  2. capture an image
  3. now you're on the story composer, tap on the + on the story slide selector
  4. the media picker shows up, do your selection here and send the app to the background
  5. bring it back to the foreground, it should remain selected as per step 4.

Here's a gif showing it works for me on a Pixel 3 emu (API 29), would love to see if it's the same for you 🙏

pickerstate

Please note though, this PR does not address anything such as media picker selection (also remember the picker is externally provided as well), but good to check for overall healthiness 👍

@mzorz
Copy link
Contributor Author

mzorz commented Jul 19, 2020

Hey @mzorz not sure if I have ever shown you this or if there's an issue tracking it but noting here since it occurred during testing; the video becomes stretched at times when putting and bringing the app out of the foreground. Let me know what you think.

The reproduction steps are just adding a video as a story frame and then doing the app in and out of the foreground to catch it.

Interesting indeed 🤔 ; just to make sure I'm getting this correctly, can you verify these are the steps to make the bug occur?:

  1. add a video as a story frame
  2. see how it's playing in the background
  3. send the app to the background
  4. bring it back to the foreground
  5. observe the video playing as a background is now stretched

Is this what happened?

EDIT: possibly related: #200

@jd-alexander
Copy link
Contributor

Thanks for the clarification on the #435 (comment) I followed the steps you provided and got the desired results. I also remember the discussion with Megs about removing the upload icon so thank you for pointing that out 🙏

@jd-alexander
Copy link
Contributor

So for #435 (comment) the steps did make the bug occur, for more context the video didn't continue to play stretched. It only happened for about a second and then it went back to normal so the issue is probably related.

Copy link
Contributor

@jd-alexander jd-alexander left a comment

Choose a reason for hiding this comment

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

Thanks for these changes @mzorz apart from the other unrelated bug I noticed, all the tests passed and the changes in the code here looks solid. I didn't find any issues. LGTM 🚢

Base automatically changed from issue/393-keep-state to feature/wp-stories/alpha2 July 19, 2020 01:19
@jd-alexander jd-alexander merged commit 205f70c into feature/wp-stories/alpha2 Jul 19, 2020
@aforcier
Copy link
Collaborator

Gave this a spin as well, it's working great! I also tried the same cases from the WPAndroid branch, and tested publishing immediately after the activity is re-created with no issues.

Incidentally, I did notice that the upload-as-gallery fallback on non-alpha sites seems to be broken (posts are uploaded as empty drafts for me, no media no gallery block). I'll send you a note about that.

@jd-alexander jd-alexander deleted the issue/393-keep-state-part2 branch July 21, 2020 10:08
@mzorz
Copy link
Contributor Author

mzorz commented Jul 21, 2020

So for #435 (comment) the steps did make the bug occur, for more context the video didn't continue to play stretched. It only happened for about a second and then it went back to normal so the issue is probably related.

Wanted to follow up on this - I think this is expected - the effect is that you see a "screenshot" of the Activity in the recents list, and it's zoomed in as you tap on it until it fills the screen, then the actual Activity goes through all the lifecycle methods for startup and starts running. On the Samsung J2 I have here I can even see some pixelation on that screenshot (which is performed at the system level), until the actual rendering takes control.

@mzorz
Copy link
Contributor Author

mzorz commented Jul 21, 2020

Incidentally, I did notice that the upload-as-gallery fallback on non-alpha sites seems to be broken (posts are uploaded as empty drafts for me, no media no gallery block). I'll send you a note about that.

Tracked on #450

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants