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

Introducing Frame Saving process into a Service #278

Merged

Conversation

mzorz
Copy link
Contributor

@mzorz mzorz commented Mar 15, 2020

Closes #273

Intro

This PR takes the former work done in order to implement a Story Frames Save architecture (see #257 and related PRs) and encapsulate the work into a Service.

This is needed given sometimes, saving a Story's frames may take time (especially if they have video-background in one or more of their Story frames), and we don't want to block the user on a screen, waiting for things to get processed.

Services

The Android documentation describes a few of the possibilities we have for performing background work. We're going to use a Foreground Service given that's the mechanism that best fits the description of our use case as explained here.

For user-initiated work that need to run immediately and must execute to completion, use a foreground service. Using a foreground service tells the system that the app is doing something important and it shouldn’t be killed. Foreground services are visible to users via a non-dismissible notification in the notification tray.

In our case, the user taps on Save button and makes it explicit that they want their Story and Story frames to get saved. Hence, it makes sense to encapsulate this work in a Foreground Service.

In our specific case, we can roughly say there are 2 big parts to finishing a Story:

  1. Saving the composed Story frames to disk so these are available for upload
  2. upload the frames and Story to a server.

For now, this PR will start with part 1, and move the already existing code from being carelessly called from the Activity (in the click listener for the Next button) into a foreground Service, as this operation can potentially be lengthy depending on hardware and amount of frames to be saved.
For the upload part we're going to delegate that to the existing WordPress Android UploadService, which will manage both the adding of images and uploading a custom typed Post to an existing site (to be covered in further PRs when integration of the functionality into WPAndroid is planned).

How it works

There's a problem I found while doing this: a started Service is passed parameters with an Intent extras bundle, and our parameters here would be a list of images with their added Views, so unless we access the ViewModel's Repository from the Service (with the Repository actually persisting the data, which is not our case right now as we just keep these in memory) then we can't pass the frames with their added views if we don't first serialize this information (that is, positioning, rotation and scale information for each of the added views).

Given our Service is actually triggered from an Activity and is quite related to it (it's a user initiated action), looks like this is a good candidate for a bound service, thus allowing us to bypass the serialization process which among other things would also incur into some time penalty (both for execution and development time). In this case, for the reasons outlined above we also need to making it both a bound Service and a foreground service (as we need the background saving process to exist even if the user navigates away from the Activity or even further, away from the app itself). This PR only tackled the bound Service part, and leaves the foreground Service for an upcoming PR so to keep things simpler and easier to review.

Once the processing is ready, it can either have a successful ending, or some of the frames may have not been saved successfully. The error collection strategy will be tackled on another PR, for now we're introducing EventBus here just as a means to signal the saving process has ended (either successfully or not, it doesn't matter for now in this PR), so the event listener can properly do the cleanup they want.

Next steps (to be tackled in subsequent PRs):

A word on future WPAndroid integration

Before tackling this PR, I studied how other services were implemented, particularly SiteCreationService as a model for a well written Service using the latest guidelines and good stuff Kotlin brings (such as corroutines) and the UploadService given our process output here will be fed into the UploadService afterwards. We followed the same pattern as used for other Services in WPAndroid, which will make it easier later when we integrate this functionality there:

  • the new class extends Service
  • we make it a foreground Service, like the UploadService is. Although a Story can be saved in just a few seconds (while this depends, it's generally less than 2 seconds for ~14 frames as per our tests), we could also improve this part later when integrating with WPAndroid and make use of AutoForeground which is implemented there for the SiteCreationService.

To test:

  1. open portkey
  2. tap on the + FAB
  3. either capture pictures or hold the big button on the bottom to start recording a video, several times
  4. add some text / emoji to your frames (this makes processing harder, just 1 added view should be fine for this PR purposes)
  5. once you're ready, tap NEXT
  6. observe a "Saving..." scrim appears (navigating away from this will probably break stuff as this is just introducing a bound Service, so please just stay there for now :) )
  7. once the process ends, the "Saving" scrim disappears
  8. got to the Photos app and find your newly created frames (videos, pictures) with your added text / emoji there.

@peril-automattic
Copy link

peril-automattic bot commented Mar 15, 2020

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

@mzorz mzorz changed the base branch from develop to feature/273-save-frame-service-base March 16, 2020 11:02
@mzorz mzorz marked this pull request as ready for review March 17, 2020 14:07
@mzorz mzorz requested review from jd-alexander and marecar3 March 17, 2020 15:00
@mzorz
Copy link
Contributor Author

mzorz commented Mar 18, 2020

Ok! Figured out a few more things now, describing them here:

  • in 51ae33c we moved the creation of FrameSaveManager itself within the Service, given it's no longer an Activity's task to deal with. Also note that FrameSaveManager is a CoroutineScope that gets definitively cancelled when the Service is destroyed, hence it makes sense to encapsulate it as close to where it's used.

  • This commit 281a350 moves the async{} job builder calls out of the withContext(coroutineContext){} block by removing such block, given the FrameSaveManager is already a CoroutineScope and its context is defined by val coroutineContext so, no need to place this context switch here (it's already in the context we're asking it to run in).

  • Also in the same commit 281a350 we're removing the call to start the Service from a coroutine, it didn't make any sense it was just code leftover from the previous stage where the Activity was responsible for triggering the FrameSaveManager coroutines (and now the Service is).

This should be ready for another round @jd-alexander 🙇

@jd-alexander jd-alexander self-requested a review March 19, 2020 01:46
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 I tested this and it worked!
I even exited the app and the service continued in the background and I was able to see the newly created stories in my gallery.

I stumbled on two crashes that you could resolve before we proceed.

 Process: com.automattic.portkey, PID: 28239
    java.lang.IllegalStateException: Fragment already added: EmojiPickerFragment{c8ed3ee (de450151-c4e1-4835-a2c7-c00ac471d1cb)}
        at androidx.fragment.app.FragmentManagerImpl.addFragment(FragmentManagerImpl.java:1379)
        at androidx.fragment.app.BackStackRecord.executeOps(BackStackRecord.java:399)
        at androidx.fragment.app.FragmentManagerImpl.executeOps(FragmentManagerImpl.java:2079)
        at androidx.fragment.app.FragmentManagerImpl.executeOpsTogether(FragmentManagerImpl.java:1869)
        at androidx.fragment.app.FragmentManagerImpl.removeRedundantOperationsAndExecute(FragmentManagerImpl.java:1824)
        at androidx.fragment.app.FragmentManagerImpl.execPendingActions(FragmentManagerImpl.java:1727)
        at androidx.fragment.app.FragmentManagerImpl$2.run(FragmentManagerImpl.java:150)
        at android.os.Handler.handleCallback(Handler.java:883)
        at android.os.Handler.dispatchMessage(Handler.java:100)
        at android.os.Looper.loop(Looper.java:214)
        at android.app.ActivityThread.main(ActivityThread.java:7356)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:492)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:930)
    java.lang.IndexOutOfBoundsException: Index: 0, Size: 0
        at java.util.ArrayList.get(ArrayList.java:437)
        at java.util.Collections$UnmodifiableList.get(Collections.java:1356)
        at com.automattic.portkey.compose.story.StoryViewModel.getCurrentStoryFrameAt(StoryViewModel.kt:84)
        at com.automattic.portkey.compose.ComposeLoopFrameActivity.addCurrentViewsToFrameAtIndex(ComposeLoopFrameActivity.kt:598)
        at com.automattic.portkey.compose.ComposeLoopFrameActivity.access$addCurrentViewsToFrameAtIndex(ComposeLoopFrameActivity.kt:105)
        at com.automattic.portkey.compose.ComposeLoopFrameActivity$addClickListeners$9.onClick(ComposeLoopFrameActivity.kt:542)
        at android.view.View.performClick(View.java:7125)
        at android.view.View.performClickInternal(View.java:7102)
        at android.view.View.access$3500(View.java:801)
        at android.view.View$PerformClick.run(View.java:27336)
        at android.os.Handler.handleCallback(Handler.java:883)
        at android.os.Handler.dispatchMessage(Handler.java:100)
        at android.os.Looper.loop(Looper.java:214)
        at android.app.ActivityThread.main(ActivityThread.java:7356)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:492)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:930)

@mzorz
Copy link
Contributor Author

mzorz commented Mar 20, 2020

Thanks for these changes @mzorz I tested this and it worked!
I even exited the app and the service continued in the background and I was able to see the newly created stories in my gallery.

Awesome! Glad it worked 🎉

I stumbled on two crashes that you could resolve before we proceed

Nice spotting those! I think both issues are unrelated to this specific PR, but I'm so glad you've run into those and reported here 🙇.

  • The first one is one that I've been aware of for some time, it's tracked here Crash when tapping on Stickers button rapidly twice #201
  • Re: the second one, I've opened a new issue here Crash in ComposeLoopFrameActvity when in Story Editing Mode #280 with steps to reproduce and an explanation of why it rather entails a different kind of effort which is planned for later in the project's roadmap. tl;dr we're not saving the ViewModel's state there, but we plan to implement the StoryRepository as something that has some more lasting information (right now the StoryRepository is simply backed up by an array in memory, that's why having Don't keep activities ON makes it fail in all its glory 🤷‍♂), still have doubts about making it part of FluxC or a proper database on the client app's side of things. Will circle back on this for sure 👍

Let me know if there are any other observations that I'm happy to look into @jd-alexander !

@jd-alexander jd-alexander self-requested a review March 20, 2020 15:17
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.

Let me know if there are any other observations that I'm happy to look into @jd-alexander !

Okay great! Yes, once there aren't related to this PR and their are issues to track them then we are all good here and ready to go!

LGTM 🚢

@mzorz
Copy link
Contributor Author

mzorz commented Mar 20, 2020

Thank you very much for your review 🙇 ! Going to merge into the feature branch and update the next PR's base branch 👍

@mzorz mzorz merged commit 8e44922 into feature/273-save-frame-service-base Mar 20, 2020
@mzorz mzorz deleted the issue/273-save-frame-service branch July 29, 2020 12:45
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.

2 participants