search

Automattic / stories-android

Loop concept app - WP Stories library
GNU General Public License v2.0
17 stars 6 forks source link

Trigger onFrameSaveCompleted when saving unmodified recorded video #701

Closed aforcier closed 3 years ago

aforcier commented 3 years ago

Fixes https://github.com/wordpress-mobile/WordPress-Android/issues/14787.

I'm pretty sure this used to work and this must be a regression, though I haven't narrowed down when exactly it happened.

The problem starts here:

https://github.com/Automattic/stories-android/blob/a8a068f5307a1d44df2c8128604a022bc8a7a7ba/stories/src/main/java/com/wordpress/stories/compose/frame/FrameSaveManager.kt#L156-L166

The else arm is only run when there are no views added to the video, and when the source is a Uri (which is the case when selecting an existing local video from the picker rather than recording one). (This explains why the bug doesn't accur when the video is added from device or has had views added.)

Since it doesn't call saveVideoFrame(), we never end up adding success data to the FrameSaveResult as we do for the other case:

https://github.com/Automattic/stories-android/blob/a8a068f5307a1d44df2c8128604a022bc8a7a7ba/stories/src/main/java/com/wordpress/stories/compose/frame/FrameSaveService.kt#L330

This doesn't immediately break anything, however there are a few places where we check that the number of frames in FrameSaveResult matches the overall number of frames for the story, to confirm that saving is complete. The specific case that caused the bug I think is here. isStorySavingComplete() compares the frameSaveResult size to the size of the story, and if they don't match assumes saving isn't complete yet. The result is that the two never match, so we never end up triggering the media upload.

Triggering onFrameSaveCompleted() in the unmodified recorded video case updates the FrameSaveResult and allows the story save to be recognized as completed.

To test

  1. Use the corresponding WPAndroid PR
  2. Create a story post by recording a video
  3. Don't add any text to the story
  4. Publish
  5. Confirm that the story publishes and contains the video as expected
  6. Try the above with a story with multiple slides, one of which is a video recorded on the spot
  7. Confirm that story publishes correctly as well
peril-automattic[bot] commented 3 years ago

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

aforcier commented 3 years ago

Thanks @cameronvoell !