[Gutenberg]Video insert & upload by pinarol · Pull Request #11454 · wordpress-mobile/WordPress-iOS

pinarol · 2019-04-11T09:44:29Z

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-mobile PR: wordpress-mobile/gutenberg-mobile#854

To test:

Prerequisites:
Run metro on gutenberg-mobile PR branch
Run rake dependencies

Test 1

First of all try all the media upload test cases described here:wordpress-mobile/gutenberg-mobile#854 (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:

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:

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

# Conflicts: # Podfile # Podfile.lock # WordPress/Classes/ViewRelated/Gutenberg/GutenbergViewController.swift

# Conflicts: # Podfile.lock

pinarol · 2019-04-17T13:10:25Z

We can experiment on running tests in the test plan I prepared for video block https://wordpress-mobile.testquality.com/project/6587/plan/11476/run/91298

pinarol · 2019-04-17T13:11:19Z

cc @koke

# Conflicts: # Podfile # Podfile.lock

SergioEstevao

I tested the iOS app, and things are working great. I tested the following:

Add a video from device
Add a video from media library
Cancel a video upload by tapping the image
Cancel a video upload by deleting the block
Retested the image functionality.

SergioEstevao · 2019-04-24T15:43:55Z

WordPress/Classes/ViewRelated/Gutenberg/GutenbergMediaInserterHelper.swift

                break
            }
-            gutenberg.mediaUploadUpdate(id: mediaUploadID, state: .succeeded, progress: 1, url: url, serverID: mediaServerID)
+            if media.mediaType == .image {


Maybe a switch will be better, just in case in the future we add some extra cases.

SergioEstevao · 2019-04-24T15:45:20Z

WordPress/Classes/ViewRelated/Gutenberg/GutenbergMediaInserterHelper.swift

        }
    }
+
+    func fetchVideoPressUrl(for media: Media, completion: @escaping ( Result<(videoURL: URL, posterURL: URL?)> ) -> Void) {


I think the name should be fetchVideoPressURL

that method's implementation has changed so I updated its name as fetchRemoteURL

SergioEstevao · 2019-04-24T15:48:34Z

WordPress/Classes/ViewRelated/Gutenberg/GutenbergMediaInserterHelper.swift

+            if media.mediaType == .image {
+                gutenberg.mediaUploadUpdate(id: mediaUploadID, state: .succeeded, progress: 1, url: url, serverID: mediaServerID)
+            } else if media.mediaType == .video {
+                fetchVideoPressUrl(for: media) { [weak self] (result) in


VideoPress information will only be available if the site is WordPress.com or using Jetpack.
If it's a self-hosted site without Jetpack the video will not be videoPress and you will need to send the 'media.remoteURL' directly.

great catch! I have updated the code accordingly

SergioEstevao · 2019-04-24T15:55:37Z

WordPress/Classes/ViewRelated/Gutenberg/GutenbergMediaInserterHelper.swift

+
+    func fetchVideoPressUrl(for media: Media, completion: @escaping ( Result<(videoURL: URL, posterURL: URL?)> ) -> Void) {
+        guard let videoPressID = media.videopressGUID else {
+            completion(Result.error(NSError()))


If there is no videoPressID the site can be a self-hosted site without VideoPress so you should return directly the media.remoteURL

Great insights @SergioEstevao ! Luckily I have a self-hosted site which I only use for testing, I had the chance to test on that and yes media.remoteURL works like a charm

cc @marecar3 don't forget to handle the case for self-hosted sites on WPAndroid also. I can give you login info to my self hosted site if you need for testing.

Thanks for the heads up @pinarol!

# Conflicts: # Podfile.lock

SergioEstevao

Working great on iOS!

daniloercoli · 2019-04-30T10:44:59Z

Cancel a video upload by tapping the image

Hey @SergioEstevao 👋

Did you try to tap on an already uploaded video, when there is another video currently uploading?
On Android, when I tapped on an already uploaded video, and the other one was in progress, i saw the following popup:

Remove is OK, but Retry should not be there, as well as Retry All.

SergioEstevao · 2019-05-01T13:36:05Z

Did you try to tap on an already uploaded video, when there is another video currently uploading?

@daniloercoli on iOS it's working ok, when you tap on already it opens a full screen preview of tha video.

If you tap on a current uploading video you get a an action sheet asking to cancel upload or dismiss.

# Conflicts: # Podfile # Podfile.lock

Otherwise local url remains if upload completes outside of gutenberg editor

pinarol · 2019-05-09T07:02:16Z

hey @etoledom, @SergioEstevao 👋 I pushed a new commit to address this:

2. Close post with an ongoing video upload.
This in general works well, but there's a particular case where there's an issue:
When the upload finishes in the Post List, and then the post is open again, the video block is loaded with a local URL.

I made a video to showcase the problem.

About the crash on background thread I opened an issue for Groundskeeping: #11664

original comment too see more context: wordpress-mobile/gutenberg-mobile#854 (review)

SergioEstevao · 2019-05-09T08:19:50Z

WordPress/Classes/Extensions/AbstractPost+Media.swift

+
+extension AbstractPost {
+
+    func fetchRemoteVideoURL(for media: Media, completion: @escaping ( Result<(videoURL: URL, posterURL: URL?)> ) -> Void) {


I don't think this is the best place for this method because of:

The method isn't related to the Post object in anyway, if we are doing an extension maybe it should be to the Media object.

Even as an extension to the media object it will be a bit out of place because it's creating a MediaService inside and the entity objects should avoid creating service, it should be the other way around.

What about creating a helper class? What was the initial reason to move it here?

Actually I don't have a strong reason, I was hesitant about putting it here too. Yes I can put that into a helper

etoledom · 2019-05-09T09:36:52Z

Fwiw, this fixes the issue on my tests 🎉

pinarol · 2019-05-09T10:24:06Z

@SergioEstevao Could you please re-review? (I moved the method to EditorMadiaUtility)

SergioEstevao

Looking good!

etoledom · 2019-05-10T08:17:18Z

@pinarol - I've found a new issue uploading multiple videos. This wasn't happening before so it seems to be a regression from the last update:

Intent: Upload many different videos.
Result: All videos uploaded are the same one.
Steps:

Create a few empty video blocks.
Add a different video on each of them (hopefully heavy ones).
Publish the post before the videos finish uploading.
In the Post List, when the post is finally published, open it again.
All videos will be the same one (the last one uploaded).

I believe that this happens even if some videos already finished uploading.

# Conflicts: # Podfile.lock

pinarol · 2019-05-10T13:59:18Z

hey @marecar3 could you be able to check if this is happening on Android ? #11454 (comment)

if so at least we'd know where to look for.

pinarol · 2019-05-10T14:09:49Z

WordPress/Classes/Services/PostCoordinator.swift

+                    }
+                    switch media.mediaType {
+                    case .video:
+                        EditorMediaUtility.fetchRemoteVideoURL(for: media, in: post) { [weak self] (result) in


@SergioEstevao I have second thoughts about where to put this service call because this is being called for classical editor posts also. is there any chance you know a better place to put this?

pinarol · 2019-05-10T14:12:21Z

WordPress/Classes/Services/PostCoordinator.swift

+                            case .error:
+                                self?.change(post: post, status: .failed)
+                            case .success(let value):
+                                media.remoteURL = value.videoURL.absoluteString


also I am not sure if that's a right thing to change a managed object like this :(

marecar3 · 2019-05-10T15:55:01Z

hey @marecar3 could you be able to check if this is happening on Android ? #11454 (comment)

if so at least we'd know where to look for.

Hey, @etoledom @pinarol I have tested this scenario on WPAndroid, and is seems that everything is working fine. I have tested with 2 videos and when the upload was finished in the Post list, I opened the Post and both videos were different.

…correct media finished.

WordPress/Classes/ViewRelated/Gutenberg/Processors/GutenbergVideoUploadProcessor.swift

SergioEstevao · 2019-05-10T21:03:31Z

@pinarol - I've found a new issue uploading multiple videos. This wasn't happening before so it seems to be a regression from the last update:

Intent: Upload many different videos.
Result: All videos uploaded are the same one.
Steps:
* Create a few empty video blocks.

* Add a different video on each of them (hopefully heavy ones).

* Publish the post before the videos finish uploading.

* In the Post List, when the post is finally published, open it again.

* All videos will be the same one (the last one uploaded).
I believe that this happens even if some videos already finished uploading.

@etoledom I pushed some changes to the PR that address this issue, do you mind to retest it?

marecar3 · 2019-05-10T21:27:01Z

Hey @SergioEstevao nice work here! I have tested this and it's working good :)

cc @etoledom @pinarol

pinarol added 6 commits April 3, 2019 18:53

Show media library due to media type

bb45c9d

Merge branch 'develop' into gutenberg/video-upload

98d7b03

# Conflicts: # Podfile # Podfile.lock # WordPress/Classes/ViewRelated/Gutenberg/GutenbergViewController.swift

Update podfile

424ac14

Fetch videopress url for video uploads

be1ed4e

Update Podfile

0bf939a

Merge branch 'develop' into gutenberg/video-upload

e8621bd

# Conflicts: # Podfile.lock

pinarol added the Gutenberg Editing and display of Gutenberg blocks. label Apr 11, 2019

pinarol added this to the Someday milestone Apr 11, 2019

pinarol self-assigned this Apr 11, 2019

pinarol added the [Status] Not Ready for Review label Apr 11, 2019

This was referenced Apr 11, 2019

Add video block ( first iteration ) wordpress-mobile/gutenberg-mobile#854

Merged

Video block (first iteration) wordpress-mobile/gutenberg-mobile#855

Closed

pinarol added 2 commits April 11, 2019 14:54

Update podfile

349006b

Remove unnecessary whitespace

97b51f5

pinarol force-pushed the gutenberg/video-upload branch from 6594f42 to 97b51f5 Compare April 11, 2019 11:59

pinarol added 6 commits April 11, 2019 15:01

Reverse Workspace settings

1f5e054

Update podfile.lock

6adfd92

Merge branch 'develop' into gutenberg/video-upload

41f6e41

# Conflicts: # Podfile.lock

Refresh fetched records when media filter changes

9c5f5be

Allow capturing video also

e3d09a0

Merge branch 'develop' into gutenberg/video-upload

363d666

# Conflicts: # Podfile.lock

pinarol requested a review from etoledom April 17, 2019 13:09

pinarol removed the [Status] Not Ready for Review label Apr 17, 2019

pinarol requested a review from SergioEstevao April 17, 2019 13:11

Merge branch 'develop' into gutenberg/video-upload

5aeef3d

# Conflicts: # Podfile # Podfile.lock

SergioEstevao approved these changes Apr 24, 2019

View reviewed changes

SergioEstevao requested changes Apr 24, 2019

View reviewed changes

Merge branch 'develop' into gutenberg/video-upload

583e41e

# Conflicts: # Podfile.lock

pinarol requested a review from SergioEstevao April 24, 2019 17:32

SergioEstevao approved these changes Apr 29, 2019

View reviewed changes

pinarol added 3 commits May 6, 2019 18:50

Merge branch 'develop' into gutenberg/video-upload

05c8272

# Conflicts: # Podfile # Podfile.lock

Fix merge issue

9ec151e

Update post content with real video url and video id

605df1f

Otherwise local url remains if upload completes outside of gutenberg editor

SergioEstevao reviewed May 9, 2019

View reviewed changes

pinarol requested a review from SergioEstevao May 9, 2019 10:08

Move fetchRemoteVideoURL to EditorMediaUtility

4c8fde0

SergioEstevao approved these changes May 9, 2019

View reviewed changes

marecar3 mentioned this pull request May 10, 2019

[Mobile] Video block ui/ux enhancements WordPress/gutenberg#15551

Merged

pinarol added 2 commits May 10, 2019 15:36

Update gutenberg commit hash

ff418ad

Merge branch 'develop' into gutenberg/video-upload

521e1da

# Conflicts: # Podfile.lock

pinarol commented May 10, 2019

View reviewed changes

Make sure the GutenbergVideoProcessor only changes the source of the …

edad05b

…correct media finished.

houndci-bot reviewed May 10, 2019

View reviewed changes

WordPress/Classes/ViewRelated/Gutenberg/Processors/GutenbergVideoUploadProcessor.swift Outdated Show resolved Hide resolved

WordPress/Classes/ViewRelated/Gutenberg/Processors/GutenbergVideoUploadProcessor.swift Outdated Show resolved Hide resolved

Address lint issues.

07e6ad6

Point Podfile to latest gutenberg mobile develop commit ref

feac242

marecar3 merged commit c0ddace into develop May 12, 2019

marecar3 deleted the gutenberg/video-upload branch May 12, 2019 06:59


		extension AbstractPost {

		func fetchRemoteVideoURL(for media: Media, completion: @escaping ( Result<(videoURL: URL, posterURL: URL?)> ) -> Void) {

Conversation

pinarol commented Apr 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pinarol commented Apr 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pinarol commented Apr 17, 2019

Uh oh!

SergioEstevao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pinarol Apr 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pinarol Apr 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pinarol Apr 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SergioEstevao left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daniloercoli commented Apr 30, 2019

Uh oh!

SergioEstevao commented May 1, 2019

Uh oh!

pinarol commented May 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etoledom commented May 9, 2019

Uh oh!

pinarol commented May 9, 2019

Uh oh!

SergioEstevao left a comment

Choose a reason for hiding this comment

Uh oh!

etoledom commented May 10, 2019

Uh oh!

pinarol commented May 10, 2019

Uh oh!

pinarol May 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marecar3 commented May 10, 2019

Uh oh!

Uh oh!

Uh oh!

SergioEstevao commented May 10, 2019

Uh oh!

marecar3 commented May 10, 2019

Uh oh!

Reviewers

Assignees

Labels

pinarol commented Apr 11, 2019 •

edited

Loading

pinarol commented Apr 17, 2019 •

edited

Loading

pinarol Apr 24, 2019 •

edited

Loading

pinarol Apr 24, 2019 •

edited

Loading

pinarol Apr 24, 2019 •

edited

Loading

SergioEstevao left a comment •

edited

Loading

pinarol May 10, 2019 •

edited

Loading