Progress and Error notifications: keep state of multiple concurrent Stories being saved #297

mzorz · 2020-04-09T22:40:26Z

Builds on top of #287

This PR fixes the problem identified in #286 (comment), by keeping state in the FrameSaveNotifier class of the amount of frames being saved belonging to different Stories.
This gives us 2 precious things:

the ability to change the foreground progress notification title from Saving <story title>... to Saving several stories... and viceversa depending on how things are going
the ability to have distinct error notifications for each Story being uploaded.

Screenshots:

Video showcasing this here:

https://cloudup.com/cMS87Fa9j3i

To test

CASE A: Happy path:

we'll need to have a way to distinguish Stories, so replace line 150 in ComposeLoopFrameActivity with this:

storyViewModel.setCurrentStoryTitle("Test" + index)

This will set the story title to be something like Test0 or Test1, etc.

run the app, select some 6-8 videos to upload
add some text / emoji to each one of them
tap NEXT to start the service
as soon as you exit the Activity, drag from the top border of the screen down to the center, to observe the foreground notification. You should see the Saving Test0... title on it.
tap on + to create a new story
add another 2 videos there, and add some emoji/text (need to be quick here)
drag from the top border of the screen down to the center, to observe the foreground notification. You should now see Saving several stories... title on it.
Just wait and see how it changes, at some point the title should switch to Saving Test1... note it's no longer Test0, but Test1, indicating Test0 has finished now and we only have frames from Test1 remaining to be saved.

CASE B: Error path

apply the patch provided in this gist. This is got several modifications just to deliberately throw an artificial exception for two different stories, for one of their video frames, when these reach 80% of progress being saved.
select some 6-8 videos to upload
add some text / emoji to each one of them
tap NEXT to start the service
as soon as you exit the Activity, tap on + to create a new story
add another 2 videos there, and add some emoji/text (need to be quick here)
when everything (or most) ends, the first video for each of these 2 stories should have failed, and hence you should have 2 distinct error notifications showing on your system dashboard.
tap on one, verify it's for the long story
tap on the second one, verify it's for the short (2 video frames) story.

…he StoryRepository

…we're saving concurrently, to show the right progress notifications

…n the foregorund and we tap on a new error notification

peril-automattic · 2020-04-09T22:42:43Z

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

jd-alexander · 2020-04-10T00:40:20Z

Hi @mzorz 😄 so I tried the Happy path but I kept getting a crash when I tried to create the second story. So what I think is happening is

I create the first story and several videos.
I go back to create the second story but the previous frames are already there and I click next again and these are added to the current story being processed.
Then when I try to create another story again I get a crash. I wasn't able to get the stacktrace. I am trying to see if I can get it after the crash and if so I will share here 🙏

app/src/main/java/com/automattic/portkey/compose/ComposeLoopFrameActivity.kt

app/src/main/java/com/automattic/portkey/compose/frame/FrameSaveManager.kt

mzorz · 2020-04-13T13:12:37Z

Thank you for your review, tests, and comments all around @jd-alexander !

#297 (comment)

I create the first story and several videos.

I go back to create the second story but the previous frames are already there and I click next again and these are added to the current story being processed.

I haven't been able to reproduce this issue here (...create the second story but the previous frames are already there ) on the Pixel 3 on this branch - although a change introduced later on in PR #289, particularly in this commit 2201c0f should also help avoid this kind of situation completely, given new stories are always signaled with the special case DEFAULT_NONE_SELECTED.

Then when I try to create another story again I get a crash. I wasn't able to get the stacktrace. I am trying to see if I can get it after the crash and if so I will share here 🙏

Thanks for this one 🙏. As discussed, tracked this one here #299 and going to tackle it separately (it's happening on develop as well). In the meantime (so to test this PR) you should be able to workaround this crash by setting a fixed size here like this:

            //.size(originalCanvasWidth, originalCanvasHeight)
            .size(1440, 2560)

Or alternatively, use a Pixel 2 emulator where we know this issue won't be run into.

I think we should be able to continue unless on your side you see anything else blocking the tests for this PR? wdyt?

app/src/main/java/com/automattic/portkey/compose/ComposeLoopFrameActivity.kt

jd-alexander · 2020-04-14T19:49:33Z

app/src/main/java/com/automattic/portkey/compose/frame/FrameSaveNotifier.kt

@@ -318,8 +377,11 @@ class FrameSaveNotifier(private val context: Context, private val service: Frame
        doNotify(notificationId, notificationBuilder.build()) // , notificationType)
    }

-    fun getNotificationIdForError(): Long {
-        return BASE_MEDIA_ERROR_NOTIFICATION_ID.toLong()
+    fun getNotificationIdForError(storyIdx: StoryIndex): Int {


Could this be renamed to storyIndex when I was scrolling and I saw it I thought it meant id and then I realized it's in an index. Of course, it's not necessary. Just a thought :)

sure 👍 fixed in 03f31b5

jd-alexander · 2020-04-14T19:51:16Z

I just got another crash. Not sure if it's related. I was trying to follow the happy path. I added the first set of stories but upon adding the second set the crash occurred. Let me know if this is related to this PR #292

    java.lang.IllegalStateException: The specified child already has a parent. You must call removeView() on the child's parent first.
        at android.view.ViewGroup.addViewInner(ViewGroup.java:5106)
        at android.view.ViewGroup.addView(ViewGroup.java:4935)
        at android.view.ViewGroup.addView(ViewGroup.java:4907)
        at com.automattic.photoeditor.PhotoEditor.addViewToParent(PhotoEditor.kt:370)
        at com.automattic.photoeditor.PhotoEditor.addViewToParent$default(PhotoEditor.kt:365)
        at com.automattic.portkey.compose.ComposeLoopFrameActivity.onStoryFrameSelected(ComposeLoopFrameActivity.kt:1309)
        at com.automattic.portkey.compose.story.StoryFrameSelectorFragment$onCreate$2.onChanged(StoryFrameSelectorFragment.kt:43)
        at com.automattic.portkey.compose.story.StoryFrameSelectorFragment$onCreate$2.onChanged(StoryFrameSelectorFragment.kt:27)
        at com.automattic.portkey.util.SingleLiveEvent$observe$1.onChanged(SingleLiveEvent.kt:50)
        at androidx.lifecycle.LiveData.considerNotify(LiveData.java:131)
        at androidx.lifecycle.LiveData.dispatchingValue(LiveData.java:149)
        at androidx.lifecycle.LiveData.setValue(LiveData.java:307)
        at androidx.lifecycle.MutableLiveData.setValue(MutableLiveData.java:50)
        at com.automattic.portkey.util.SingleLiveEvent.setValue(SingleLiveEvent.kt:58)
        at com.automattic.portkey.compose.story.StoryViewModel.setSelectedFrameByUser(StoryViewModel.kt:69)
        at com.automattic.portkey.compose.story.StoryViewModel$createUiStateFromModelState$$inlined$forEachIndexed$lambda$1.invoke(StoryViewModel.kt:190)
        at com.automattic.portkey.compose.story.StoryViewModel$createUiStateFromModelState$$inlined$forEachIndexed$lambda$1.invoke(StoryViewModel.kt:12)
        at com.automattic.portkey.compose.story.StoryFrameSelectorAdapter$StoryFrameHolder$StoryFrameHolderItem$1.onClick(StoryFrameSelectorAdapter.kt:108)
        at android.view.View.performClick(View.java:7125)

jd-alexander

So I was able to get the happy path working on my Pixel 4 XL device.
For the error path, I applied the patch and got the error notifications as expected.
I was also able to click on the stories and I was taken to the compose screen with the videos I selected for processing. LGTM 🚢 🕺

I left some minor comments 👍

jd-alexander · 2020-04-14T20:39:19Z

@mzorz something I noticed when testing on my Pixel 4 XL. I wasn't able to add the emojis. Let me know if this is something you can reproduce or if you have been tracking this. Thanks, I think it might be device-related. It works fine on the emulator.

app/src/main/java/com/automattic/portkey/compose/frame/FrameSaveNotifier.kt

Co-Authored-By: Joel Dean <[email protected]>

…ttps://github.com/Automattic/portkey-android into issue/273-save-frame-service-no-state-multi-errror

…ave-frame-service-no-state-multi-errror

mzorz · 2020-04-15T13:48:03Z

I just got another crash. Not sure if it's related. I was trying to follow the happy path. I added the first set of stories but upon adding the second set the crash occurred. Let me know if this is related to this PR #292

Yes exactly that crash was fixed in #290, merged in #292 and also another layer of protection was added (as I ran into another case) in this WIP PR. That one should cover it 💪

@mzorz something I noticed when testing on my Pixel 4 XL. I wasn't able to add the emojis. Let me know if this is something you can reproduce or if you have been tracking this. Thanks, I think it might be device-related. It works fine on the emulator.

Yeah saw this one too on the Pixel 3 emulator, on develop. Tracking it down in #301. Will prioritize accordingly, once I get the main feature ready 👍

mzorz · 2020-04-15T13:56:31Z

Thanks for your review once more Joel! Merging now 👍

mzorz added 3 commits April 9, 2020 11:55

added specific error notification id, based on the Stoyr's index in t…

015b1cc

…he StoryRepository

added structure and functionality to hold how many different Stories …

3cfb65c

…we're saving concurrently, to show the right progress notifications

added onNewIntent override, so we can reload the Activity when it's o…

a4355d0

…n the foregorund and we tap on a new error notification

fixed lint warning

b3761cb

mzorz marked this pull request as ready for review April 9, 2020 22:44

mzorz requested review from jd-alexander and marecar3 April 9, 2020 22:44

This was referenced Apr 9, 2020

Start / Success / Error snackbars #286

Merged

Save frame service - introduce StorySaveProcessor to handle state #287

Merged

mzorz changed the title ~~Keep state of multiple concurrent Stories being saved~~ Progress and Error notifications: keep state of multiple concurrent Stories being saved Apr 10, 2020

mzorz commented Apr 13, 2020

View reviewed changes

app/src/main/java/com/automattic/portkey/compose/ComposeLoopFrameActivity.kt Show resolved Hide resolved

mzorz commented Apr 13, 2020

View reviewed changes

app/src/main/java/com/automattic/portkey/compose/frame/FrameSaveManager.kt Show resolved Hide resolved

mzorz mentioned this pull request Apr 13, 2020

Crash (Error) when attempting to save video frame on Pixel 3 emulator with API 29 #299

Closed

jd-alexander reviewed Apr 14, 2020

View reviewed changes

app/src/main/java/com/automattic/portkey/compose/ComposeLoopFrameActivity.kt Outdated Show resolved Hide resolved

jd-alexander reviewed Apr 14, 2020

View reviewed changes

jd-alexander approved these changes Apr 14, 2020

View reviewed changes

jd-alexander reviewed Apr 14, 2020

View reviewed changes

app/src/main/java/com/automattic/portkey/compose/frame/FrameSaveNotifier.kt Outdated Show resolved Hide resolved

mzorz and others added 6 commits April 15, 2020 10:04

fixed merge conflict

7de92fb

use easier to read isNotEmpty()

4686956

Co-Authored-By: Joel Dean <[email protected]>

Merge branch 'issue/273-save-frame-service-no-state-multi-errror' of h…

20f1d2b

…ttps://github.com/Automattic/portkey-android into issue/273-save-frame-service-no-state-multi-errror

renamed param

03f31b5

made title not null and removed a couple of unused methods

1725a80

Merge branch 'issue/273-save-frame-service-no-state' into issue/273-s…

99b0de6

…ave-frame-service-no-state-multi-errror

mzorz merged commit 78132a6 into issue/273-save-frame-service-no-state Apr 15, 2020

mzorz deleted the issue/273-save-frame-service-no-state-multi-errror branch April 15, 2020 13:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Progress and Error notifications: keep state of multiple concurrent Stories being saved #297

Progress and Error notifications: keep state of multiple concurrent Stories being saved #297

mzorz commented Apr 9, 2020 •

edited

Loading

peril-automattic bot commented Apr 9, 2020 •

edited

Loading

jd-alexander commented Apr 10, 2020

mzorz commented Apr 13, 2020

jd-alexander Apr 14, 2020

mzorz Apr 15, 2020

jd-alexander commented Apr 14, 2020 •

edited

Loading

jd-alexander left a comment

jd-alexander commented Apr 14, 2020 •

edited

Loading

mzorz commented Apr 15, 2020

mzorz commented Apr 15, 2020

Progress and Error notifications: keep state of multiple concurrent Stories being saved #297

Progress and Error notifications: keep state of multiple concurrent Stories being saved #297

Conversation

mzorz commented Apr 9, 2020 • edited Loading

Screenshots:

Video showcasing this here:

To test

peril-automattic bot commented Apr 9, 2020 • edited Loading

jd-alexander commented Apr 10, 2020

mzorz commented Apr 13, 2020

jd-alexander Apr 14, 2020

Choose a reason for hiding this comment

mzorz Apr 15, 2020

Choose a reason for hiding this comment

jd-alexander commented Apr 14, 2020 • edited Loading

jd-alexander left a comment

Choose a reason for hiding this comment

jd-alexander commented Apr 14, 2020 • edited Loading

mzorz commented Apr 15, 2020

mzorz commented Apr 15, 2020

mzorz commented Apr 9, 2020 •

edited

Loading

peril-automattic bot commented Apr 9, 2020 •

edited

Loading

jd-alexander commented Apr 14, 2020 •

edited

Loading

jd-alexander commented Apr 14, 2020 •

edited

Loading