Skip to content

Media Async: Visual editor support#5833

Merged
mzorz merged 11 commits intofeature/async-mediafrom
feature/fluxc-media-upload-service-visual-editor
May 10, 2017
Merged

Media Async: Visual editor support#5833
mzorz merged 11 commits intofeature/async-mediafrom
feature/fluxc-media-upload-service-visual-editor

Conversation

@aforcier
Copy link
Copy Markdown
Contributor

@aforcier aforcier commented May 3, 2017 •
edited
Loading

Adds support for asynchronously (with the editor closed) updating in-progress media uploads in posts written in the visual editor.

To test:

  1. Enable 'Visual' editor
  2. Create a post and upload media
  3. Exit the editor before the media completes
  4. Wait long enough for the media to complete (or watch the logs)
  5. Re-open the post in the editor
  6. The media item should load as already uploaded

Try above for photos and video, on WordPress.com and self-hosted sites, and on devices above and below API 19. Also make sure the tests run.

cc @mzorz

aforcier added 7 commits May 3, 2017 15:00
@aforcier
Set height and width when converting from MediaModel
04609d2
@aforcier
Implement static support for uploaded image swap in hybrid editor
ac9b834
@aforcier
Add test cases for uploading images on API<19
9076467
@aforcier
Tweak in-progress image swap to work on API<19
1d4ebb5 
Because the API<19 WebView doesn't support the progress element,
we use nested spans to create an 'Uploading...' overlay instead.
The pattern needs to be more specific in order to guarantee matching
the last closing span tag for that image.
@aforcier
Implement async uploaded video swap for hybrid editor
9b7e70d
@aforcier
Drop unneeded TODO
a7cb1fb
@aforcier
Rename test methods for clarity
f670807
@aforcier
Copy link
Copy Markdown
Contributor Author

aforcier commented May 4, 2017

Updated to move the tests to the new non-connected test folder (#5836), since they're actually 'unit' tests.

@aforcier aforcier mentioned this pull request May 9, 2017
Merged
@aforcier aforcier changed the base branch from feature/fluxc-media-upload-service-master to feature/async-media May 10, 2017 15:41
mzorz
mzorz reviewed May 10, 2017
View reviewed changes
WordPress/src/main/java/org/wordpress/android/ui/posts/services/MediaUploadReadyProcessor.java
// TODO implement visual editor implementation to update image/video in post
} else {
// TODO implement legacy editor implementation to update image/video in post
post.setContent(EditorFragment.replaceMediaFileWithUrl(post.getContent(), mediaFile));
Copy link
Copy Markdown
Contributor

@mzorz mzorz May 10, 2017

Choose a reason for hiding this comment

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

I like this is good, can we also eliminate the intermediate modifiedContents variable up here in line 22? Not part of your PR but just saying we could replicate the same way in there

Copy link
Copy Markdown
Contributor Author

@aforcier aforcier May 10, 2017

Choose a reason for hiding this comment

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

Makes sense to me too, I'll update it 👍

Copy link
Copy Markdown
Contributor Author

@aforcier aforcier May 10, 2017

Choose a reason for hiding this comment

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

Done in e286012.

Copy link
Copy Markdown
Contributor

@mzorz mzorz May 10, 2017

Choose a reason for hiding this comment

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

👍

@mzorz
Copy link
Copy Markdown
Contributor

mzorz commented May 10, 2017 •
edited
Loading

The results of my tests were overall good in regards to functionality. I found some things that are not strictly related to this PR, but if you don't mind I'll leave my notes here and we can discuss elsewhere and branch to other issues as we see fit:

On Android 7.2.1:

  • wordpress.com ok images

  • wordpress.com ok videos:
    Comment: when the first item in the post is a video, the preview in the Posts list show then [wpvideo] tag (see screenshot). Might be a nitpicking but thought it was worth mentioning if we ever happen to get this out before Aztec is ready.
    untitled

  • self-hosted, jetpack connected: ok images
    Comment: I noticed that the posts doesn’t update automatically in the Post List once the upload finished (you have to either get back to the main screen or pull to refresh), this might be happening as well in the Aztec-reattach upload thing now that I think of it.

  • self-hosted, jetpack connected: ok videos

  • self-hosted, not jetpack-connected: ok images
    Something happened once but couldn’t reproduce: If I start uploading something to a post, then change the site to another site…. it will be marked as a failed upload, but didnt' tell me anything (you can only know it failed by getting to edit the post again and actually see the failed overaly on the image).
    Probably not related to our PRs, but something we need to take care of on a separate PR a part of the media project.

  • self-hosted, not jetpack-connected: ok videos

Pre-API 19 (Genymotion Samsung Galaxy S3 - 4.2.2)

  • wordpress.com ok images
  • wordpress.com videos:
  • self-hosted, jetpack connected: ok images
  • self-hosted, jetpack connected: ok videos
  • self-hosted, not jetpack-connected: ok images
    Observation: in trying to open the post, once the image was uploaded, it felt a bit stuck when switching to the Editor activity. Here the logs show a delay of more than 3 seconds (almost 4, if I’m reading correctly):
05-10 18:39:40.968 1385-1385/org.wordpress.android I/Choreographer: Skipped 34 frames!  The application may be doing too much work on its main thread.
05-10 18:39:41.068 365-380/system_process I/ActivityManager: Displayed org.wordpress.android/.ui.posts.EditPostActivity: +3s586ms
05-10 18:39:41.104 1385-1412/org.wordpress.android D/WordPress-EDITOR: New field created, id=zss_field_title
05-10 18:39:41.112 1385-1412/org.wordpress.android D/WordPress-EDITOR: New field created, id=zss_field_content
05-10 18:39:41.128 1385-1385/org.wordpress.android D/WordPress-PROFILING: Visual Editor Startup: begin
05-10 18:39:41.128 1385-1385/org.wordpress.android D/WordPress-PROFILING: Visual Editor Startup:      0 ms, EditorFragment.onCreate
05-10 18:39:41.128 1385-1385/org.wordpress.android D/WordPress-PROFILING: Visual Editor Startup:      2895 ms, EditorFragment.initJsEditor
05-10 18:39:41.128 1385-1385/org.wordpress.android D/WordPress-PROFILING: Visual Editor Startup:      731 ms, EditorFragment.onDomLoaded
05-10 18:39:41.128 1385-1385/org.wordpress.android D/WordPress-PROFILING: Visual Editor Startup:      12 ms, EditorFragment.onDomLoaded completed
05-10 18:39:41.128 1385-1385/org.wordpress.android D/WordPress-PROFILING: Visual Editor Startup: end, 3638 ms
05-10 18:39:41.160 577-577/com.android.inputmethod.latin V/InputMethodService: onEvaluateInputViewShown: config.hardKeyboardHidden = 1
05-10 18:39:41.160 577-577/com.android.inputmethod.latin V/InputMethodService: onEvaluateInputViewShown: config.hardKeyboardHidden = 1
05-10 18:39:41.236 1385-1412/org.wordpress.android D/WordPress-EDITOR: callback-response-string: function=getFailedMedia~ids=
05-10 18:39:41.292 1385-1412/org.wordpress.android D/WordPress-EDITOR: Focus in callback received
  • self-hosted, not jetpack-connected: ok videos

Continuing now with the code review 👍

@mzorz
Copy link
Copy Markdown
Contributor

mzorz commented May 10, 2017

thank you for this @aforcier! :shipit:

@mzorz mzorz self-assigned this May 10, 2017
@mzorz mzorz merged commit 69592a8 into feature/async-media May 10, 2017
@aforcier aforcier deleted the feature/fluxc-media-upload-service-visual-editor branch May 10, 2017 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mzorz mzorz mzorz left review comments

Assignees

@mzorz mzorz

Labels

Media

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@aforcier @mzorz