Skip to content

Conversation

@marecar3
Copy link
Contributor

@marecar3 marecar3 commented Apr 16, 2019

Fixes wordpress-mobile/gutenberg-mobile#855

There are also some more TODO items in this issue: wordpress-mobile/gutenberg-mobile#687 (comment) which are not handled in this PR

gutenberg PR: WordPress/gutenberg#14912
gutenberg-mobile PR: wordpress-mobile/gutenberg-mobile#854

To test:

Prerequisites:
Run yarn clean:install
Run yarn start -- --reset-cache

Test 1

First of all try all the media upload test cases described here: wordpress-mobile/gutenberg-mobile#855 (Except, in order to fail the upload try turning off the internet and waiting a bit)

Test 2

Try uploading by video capture by selecting Take a Video in a real device:
Screen Shot 2019-04-15 at 19 22 56

Test 3 Media filters

Media is filtered due to the block type with this PR
So you should be able to pick/capture only videos for video block, and only images for image block
Check both blocks and verify that this is true for all the options here:
Screen Shot 2019-04-15 at 19 22 56

Test 4 Add caption

Add a video caption text and save the post
Reopen the same post and verify the caption is added

Test 5 Simultaneous uploads

Try adding multiple video and image blocks
And try simultaneous uploads
Verify that there's no confusion and every upload is completed successfully to their intended block

Test 6 Switch to Classic editor

Add a video block and add video in it
Switch to Classic editor
Verify that video is there and it is playable

Update release notes:

  • If there are user facing changes, I have added an item to RELEASE-NOTES.txt.
    We aren't shipping the video block to production yet

Updated gutenberg-mobile submodule dependency to point to add-video-block branch
Invoke proper functions per media type
@marecar3 marecar3 added [Status] Not Ready for Review Gutenberg Editing and display of Gutenberg blocks. gutenberg-mobile labels Apr 16, 2019
@marecar3 marecar3 self-assigned this Apr 16, 2019
@peril-wordpress-mobile
Copy link

Warnings
⚠️ PR is not assigned to a milestone.

Generated by 🚫 dangerJS

@hypest
Copy link
Contributor

hypest commented Apr 17, 2019

👋 @marecar3 , feel free to update from develop since a fix for the JitPack failure happening in this PR was just merged a few minutes ago (#9614).

compileOptions {
sourceCompatibility JavaVersion.VERSION_1_8
targetCompatibility JavaVersion.VERSION_1_8
}
Copy link
Contributor

Choose a reason for hiding this comment

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

wondering, given this is a React Native requisite, is there a way we can move this gradle block to the RN bridge instead of adding this to the WordPress app? Not sure if it's possible though, but sounds like it belongs there rather than here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @mzorz, thank for the comment.
I tried to put this into the module, but it didn't pass. Let me take one more chance with it, maybe something can be done. I also don't like this, so let try to move it :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me take one more chance with it, maybe something can be done. I also don't like this, so let try to move it :)

👋 @marecar3 , curious what was the result of the extra effort on this one. I'm assuming that since this PR got merged with this change in, the effort didn't lead to something usable but still, can you share your findings if any? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, @hypest for returning on this one. I tried to move those into react-native-gutenberg-bridge but without luck. Also, I have done some googling and tried some of my own ideas but not sure how this can be resolved. If you have some ideas, please share with me and I am willing to try them :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@jtreanor jtreanor May 15, 2019

Choose a reason for hiding this comment

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

Maybe this needs to be moved to react-native-video rather than the bridge itself? I agree its not ideal to have this here, but if there's no alternative then we can live with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @jtreanor, react-native-video already has set those options https://github.com/wordpress-mobile/react-native-video/blob/enable-android-build-of-standalone-package/android-exoplayer/build.gradle
but for some reason, the main project (WPAndroid) isn't using them in a process of the build.

Not sure how we can force Main project to use those options @hypest @jtreanor

@daniloercoli
Copy link
Contributor

daniloercoli commented May 6, 2019

Tapping onREMOVE label does remove the video from the block, but doesn't stop the uplaoding of the video to the server.
Steps to repro:

  • Add a Video block
  • Choose a video
  • Tap on the video
  • Select REMOVE from the dialog
  • The video is properly removed from the block, but the uploading is still ongoing
    (You can check this in the notifications center)
    device-2019-05-06-160855

This is also happening when I remove the block, but a video is uploading.

Note: I also tested Aztec, and removing the video by "deleting" it (backspace) doesn't stop the uploading. I also tried by selecting "Stop uploading" after tapping on the video, but it didn't stop the uploading. The problem may be elsewhere.

@pinarol
Copy link
Contributor

pinarol commented May 6, 2019

Tapping onREMOVE label does remove the video from the block, but doesn't stop the uplaoding of the video to the server.

I've checked this on iOS, it works ok, the upload stops if we tap on the ongoing upload and select REMOVE from the dialog

This is also happening when I remove the block, but a video is uploading.

This happens on iOS too, but it is not directly related with video block, happens also on image block and I think should be investigated under a new issue.

@pinarol
Copy link
Contributor

pinarol commented May 6, 2019

Noticed that the "forward to the end" button is not available in the player menu.

It might be because there aren't multiple tracks on the player and there's nothing to play next ?

@daniloercoli
Copy link
Contributor

daniloercoli commented May 7, 2019

Your latest commits @marecar3 fixed the issues reported during my 1st pass over this PR.

There are still problems in regards notifications, and media uploads, but I can reproduce them on develop & Aztec also, and opened a new issue for them.

I think we can merge this Video PR, and reiterate on it later if needed, when the Gutenberg companion PR is ready - I still see some comments open there.

@marecar3
Copy link
Contributor Author

marecar3 commented May 7, 2019

Thanks @daniloercoli!

@marecar3
Copy link
Contributor Author

marecar3 commented May 7, 2019

Hey @daniloercoli, I have fixed also the problem with uploading multiple videos from WordPress Library. Now when you select more than one, it will be shown in the Post.

@marecar3
Copy link
Contributor Author

Hey @daniloercoli , @pinarol I have found some strange bug on Android.

  1. Create new post
  2. Create new video block
  3. Upload video
  4. Create new paragraph block
  5. Turn off the screen with power button
  6. Turn on the screen with power button

Result: Last video frame on video block is lost, so it seems that video block is empty, but it isn't as if you click on it, you will see video controls and if you click on the play button, the video will show up.

@pinarol
Copy link
Contributor

pinarol commented May 10, 2019

hey @marecar3 you are turning the device off/on right? I wonder if this is happening when you set controls=false. maybe with controls=true it is starting to fill the video buffer right away and that buffer is reset after device on/off. I am asking because setting controls=false might be something we want in the future to get more close to the designs, we are limited by the fact that react-native-video's full page display isn't working on Android for some reason, if we can figure that out we'd be able to do it and that would also solve this issue.

Another option would be checking to see if onError callback is called in this situation, maybe we can try to handle this somehow, not sure how but we can think.

But in any ways, this didn't seem like a big problem to me, I mean they can see the controls with a tap and someone would usually tap to see if there's any response. I suggest we open a new issue for this and inspect later, wdyt?

@marecar3
Copy link
Contributor Author

marecar3 commented May 10, 2019

Hey @pinarol, yeah we can open another ticket for that one.

Update: I have opened a new ticket : wordpress-mobile/gutenberg-mobile#974

@marecar3
Copy link
Contributor Author

Summary of reported issues from @daniloercoli

It seems there is a problem when tapping on an already uploaded video , and another video is currently uploading . A Retry Uploading is shown with the Retry, and Retry All option in it.

Fixed.

Noticed that the "forward to the end" button is not available in the player menu.

I think that @pinarol have a point, it's disabled because there isn't the next track for playing.

I also noticed that in order to show the player I need to tap on the caption, and then on the video. Otherwise the Dialog "Retry/Remove" is shown instead.

Fixed.

Tapping onREMOVE label does remove the video from the block, but doesn't stop the uplaoding of the video to the server.

Fixed.

Opened issues :

  1. Media Uploading notification doesn't seem to be in synch with the media uploading/uploaded: Media Uploading notification doesn't seem to be in synch with the media uploading/uploaded #9802

Hey @daniloercoli is there anything else that we should address from this PR as an issue?

Copy link
Contributor

@daniloercoli daniloercoli left a comment

Choose a reason for hiding this comment

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

Thanks @marecar3, it works fine now.
LGTM!

@marecar3
Copy link
Contributor Author

Thanks @daniloercoli ! :)

@marecar3 marecar3 merged commit b81fc3f into develop May 12, 2019
@marecar3 marecar3 deleted the gutenberg/video-upload branch May 12, 2019 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Gutenberg Editing and display of Gutenberg blocks.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Video block (first iteration)

8 participants