Move deleteCapturedMedia() to OK listener for frame removal dialog

mzorz commented 3 years ago

Fix #680, #678

Getting to the cause

What was happening here is that, in the case we left the Story composer with a locally captured file as the last Story frame visited, we were actually deleting the original captured file. Hence, the next time we visited it, it would be lacking the background file, leading to 2 problems:

"Limited Story Editing" dialog appearing, because we couldn't validate we had all the background files for this Story (#678) - a note on the issue listed there, when I wrote the issue I described what I thought was happening but it's not exactly that it would become available later (I haven't been able to reproduce it since I've been able to wrap my head around this one)
Video files not being found when the video slide was captured with the phone so, getting the spinning wheel altogether. (#680)

History

This was fist introduced in the early days when we didn't even have a slide selector https://github.com/Automattic/stories-android/commit/ddab8bedee4ed0c020542d5103327e51f77041b6

There's a change introduced in this commit https://github.com/Automattic/stories-android/commit/4145288e3148aa4bdd21309a7643206682a5b363 where we were setting the currentOriginalCapturedFile to be the background file path for the selected slide. Then, if the user decided to discard a frame we would be able to delete it's backing captured file.

Later on, we moved the "discard frame?" Dialog code elsewhere when implementing the "tap to delete" behavior (see https://github.com/Automattic/stories-android/commit/427f1bc3ddb3c3252520010d2d7fd5e3d5d4b51e, PR https://github.com/Automattic/stories-android/pull/562). I think in order to keep the same behavior as before, we should be also able to delete the original captured media file there, as it was being done previously.

Solution

As said above, this PR just moves the calls to deleteCapturedMedia() to the OK listener of the "Delete frame" confirmation dialog. This way, we'll make sure to only discard a captured image / video File for which a slide won't exist anymore.

To test

create a Story using the media picker
edit it from Gutenberg
add a new video slide using capture mode (this will create a local File)
observe you can switch slides and see the background image ok for all slides (including the last one)
let the last slide (the one right before the video added in step 3) be the one you're looking at before exiting
exit the editor (tap back and discard changes). At this point in time, we'd delete the file in develop as of now.
edit the Story again
observe you can tap on the last slide and it will be showing up correctly, and also no "Limited story editing" dialog shows up.

aforcier commented 3 years ago

@mzorz I think something may have gone wrong with the merge conflict resolution commit you pushed - the original removal of a deleteCapturedMedia() call (from 02321de) is no longer in the overall PR diff. I don't think that was intentional?

mzorz commented 3 years ago

I think something may have gone wrong with the merge conflict resolution commit you pushed - the original removal of a deleteCapturedMedia() call (from 02321de) is no longer in the overall PR diff. I don't think that was intentional?

Sorry for the miss @aforcier (and nice catch!) Hopefully that's fixed with this change now 71b308b3

aforcier commented 3 years ago

@mzorz it's probably unrelated to this change, but I'm not able to test this in current WPAndroid develop. I get this crash whenever I try to select media from the media picker:

2021-05-12 11:32:15.405 17958-17958/org.wordpress.android.beta E/AndroidRuntime: FATAL EXCEPTION: main
    Process: org.wordpress.android.beta, PID: 17958
    java.lang.RuntimeException: Unable to start activity ComponentInfo{org.wordpress.android.beta/org.wordpress.android.ui.stories.StoryComposerActivity}: kotlin.UninitializedPropertyAccessException: lateinit property editorMediaListener has not been initialized
        at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:3431)
        at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:3595)
        at android.app.servertransaction.LaunchActivityItem.execute(LaunchActivityItem.java:85)
        at android.app.servertransaction.TransactionExecutor.executeCallbacks(TransactionExecutor.java:135)
        at android.app.servertransaction.TransactionExecutor.execute(TransactionExecutor.java:95)
        at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2066)
        at android.os.Handler.dispatchMessage(Handler.java:106)
        at android.os.Looper.loop(Looper.java:223)
        at android.app.ActivityThread.main(ActivityThread.java:7660)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:592)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:947)
     Caused by: kotlin.UninitializedPropertyAccessException: lateinit property editorMediaListener has not been initialized
        at org.wordpress.android.ui.stories.media.StoryEditorMedia.advertiseImageOptimisationAndAddMedia(StoryEditorMedia.kt:61)
        at org.wordpress.android.ui.stories.media.StoryEditorMedia.onPhotoPickerMediaChosen(StoryEditorMedia.kt:99)
        at org.wordpress.android.ui.stories.StoryComposerActivity.handleMediaPickerIntentData(StoryComposerActivity.kt:367)
        at org.wordpress.android.ui.stories.StoryComposerActivity.onLoadFromIntent(StoryComposerActivity.kt:280)
        at com.wordpress.stories.compose.ComposeLoopFrameActivity.onCreate(ComposeLoopFrameActivity.kt:567)
        at org.wordpress.android.ui.stories.StoryComposerActivity.onCreate(StoryComposerActivity.kt:145)
        at android.app.Activity.performCreate(Activity.java:8000)
        at android.app.Activity.performCreate(Activity.java:7984)
        at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1309)
        at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:3404)
        at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:3595) 
        at android.app.servertransaction.LaunchActivityItem.execute(LaunchActivityItem.java:85) 
        at android.app.servertransaction.TransactionExecutor.executeCallbacks(TransactionExecutor.java:135) 
        at android.app.servertransaction.TransactionExecutor.execute(TransactionExecutor.java:95) 
        at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2066) 
        at android.os.Handler.dispatchMessage(Handler.java:106) 
        at android.os.Looper.loop(Looper.java:223) 
        at android.app.ActivityThread.main(ActivityThread.java:7660) 
        at java.lang.reflect.Method.invoke(Native Method) 
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:592) 
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:947)

Steps:

Current develop as of time of writing (4746f7f224bda2502aac4e9fb284a5a28ad0a857)
Switch stories submodule to tip of this branch
Tab fab, select story option
Select 2 or 3 media items
Tap checkmark, observe crash

mzorz commented 3 years ago

Thanks for reporting this @aforcier - I think I came across the same issue and fixed it in https://github.com/Automattic/stories-android/pull/684/commits/92c9d542c79657afe1da756674a40c5d88f06682 in PR #684, will check and get back here 👍

mzorz commented 3 years ago

Just reporting back - it's kind of related but not exactly the same 😅 - will get back here when I've a solution

mzorz commented 3 years ago

Alright, fixed that crash in https://github.com/wordpress-mobile/WordPress-Android/pull/14627 @aforcier - thanks for noticing! 🙌

Ready for another round, this PR can be reviewed along with that other WPAndroid PR. Noteworthy the first time you try you'll find the other issue https://github.com/Automattic/stories-android/issues/682 which is fixed separately in https://github.com/Automattic/stories-android/pull/684.

Automattic / stories-android