Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue/10680 extract upload media phase 2 #10763

Conversation

@malinajirka
Copy link
Contributor

malinajirka commented Nov 8, 2019

Fixes #10670

This is a "final" PR of Extract media related functionality from EditPostActivity. Most parts of this code has already been reviewed (except tests). However, I've been iteratively refactoring the same code so seeing only the changes from the last PR is actually more confusing than seeing all the changes in a new PR => I decide to create a PR with all the changes. Over 1800 lines are tests and imports so the PR isn't as big as it seems - but I agree it's still really big.
All the new classes are located in org.wordpress.android.ui.posts.editor.media package. I've basically introduced a bunch of UseCases each of which takes care of a "single" task.

AddExistingMediaToPostUseCase and AddLocalMediaToPostUseCase are the most important UseCases as they orchestrate the whole process of adding a media into the editor - loading the file, copying it to app sotrage, optimizing it, creating media upload model in the local db, adding a "place holder" into the editor and initiating an upload.

To test:
Everything you can think of related to media

Screenshot 2019-11-12 at 11 02 24

Some test can be found on pb3aDo-ag-p2

PR submission checklist:

  • I have considered adding unit tests where possible.

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

oguzkocer and others added 30 commits Oct 15, 2019
This commit attempts to copy over the some (most?) of media related methods of EditPostActivity to a new class EditorMedia. If the dependencies are resolved, this will provide a basis for extracting media logic from EditPostActivity.
This is an attempt to create the communicatino between the EditPostActivity and EditorMedia. Some, if not all, of the functions in EditorMediaListener are temporary, because they are not the ideal way to communicate the information. We'll need to move a lot of logic between EditPostActivity and EditorMedia to have more meaningful functions in the listener.
…/refactor-add-media-to-editor-functionality

# Conflicts:
#	WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostActivity.java
#	WordPress/src/main/java/org/wordpress/android/ui/posts/editor/EditorMedia.kt
…rom-edit-post-activity

Extract media related code from EditPostActivity to a new class
…add-media-to-editor-functionality
…dia-to-editor-functionality

Issue/refactor add media to editor functionality
…dia-improvements

Minor EditorMedia improvements
@malinajirka malinajirka added this to the 13.7 milestone Nov 8, 2019
@malinajirka malinajirka requested a review from planarvoid Nov 8, 2019
@peril-wordpress-mobile

This comment has been minimized.

Copy link

peril-wordpress-mobile bot commented Nov 8, 2019

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

@peril-wordpress-mobile

This comment has been minimized.

Copy link

peril-wordpress-mobile bot commented Nov 8, 2019

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

@@ -2301,8 +2268,8 @@ private String migrateLegacyDraft(String content) {
while (matcher.find()) {
String stringUri = matcher.group(1);
Uri uri = Uri.parse(stringUri);
MediaFile mediaFile = FluxCUtils.mediaFileFromMediaModel(
queueFileForUpload(uri, getContentResolver().getType(uri), MediaUploadState.FAILED));
MediaFile mediaFile = FluxCUtils.mediaFileFromMediaModel(mEditorMedia

This comment has been minimized.

Copy link
@malinajirka

malinajirka Nov 8, 2019

Author Contributor

Please try to carefully compare the code in develop against the code in this branch as we can't easily test this.

Imho we invoke queueFileForUpload but the upload isn't started as MediaUploadState is set to FAILED and upload is started only for items with MediaUploadState.QUEUED.

@planarvoid

This comment has been minimized.

Copy link
Contributor

planarvoid commented Nov 11, 2019

I've found a crash:

  • Create a new post
  • Add a video
  • Rotate your device
  • The app crashes
2019-11-11 11:33:09.723 1645-1645/org.wordpress.android.beta E/AndroidRuntime: FATAL EXCEPTION: main
    Process: org.wordpress.android.beta, PID: 1645
    java.lang.RuntimeException: Unable to start activity ComponentInfo{org.wordpress.android.beta/org.wordpress.android.ui.posts.EditPostActivity}: java.lang.IllegalArgumentException: Parameter specified as non-null is null: method kotlin.jvm.internal.Intrinsics.checkParameterIsNotNull, parameter <set-?>
        at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2817)
        at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2892)
        at android.app.ActivityThread.handleRelaunchActivity(ActivityThread.java:4754)
        at android.app.ActivityThread.-wrap18(Unknown Source:0)
        at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1599)
        at android.os.Handler.dispatchMessage(Handler.java:105)
        at android.os.Looper.loop(Looper.java:164)
        at android.app.ActivityThread.main(ActivityThread.java:6541)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.Zygote$MethodAndArgsCaller.run(Zygote.java:240)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:767)
     Caused by: java.lang.IllegalArgumentException: Parameter specified as non-null is null: method kotlin.jvm.internal.Intrinsics.checkParameterIsNotNull, parameter <set-?>
        at org.wordpress.android.ui.posts.editor.media.EditorMedia.setDroppedMediaUris(Unknown Source:2)
        at org.wordpress.android.ui.posts.EditPostActivity.onCreate(EditPostActivity.java:451)
        at android.app.Activity.performCreate(Activity.java:6975)
        at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1213)
        at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2770)
        at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2892) 
        at android.app.ActivityThread.handleRelaunchActivity(ActivityThread.java:4754) 
        at android.app.ActivityThread.-wrap18(Unknown Source:0) 
        at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1599) 
        at android.os.Handler.dispatchMessage(Handler.java:105) 
        at android.os.Looper.loop(Looper.java:164) 
        at android.app.ActivityThread.main(ActivityThread.java:6541) 
        at java.lang.reflect.Method.invoke(Native Method) 
        at com.android.internal.os.Zygote$MethodAndArgsCaller.run(Zygote.java:240) 
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:767) 
malinajirka added 2 commits Nov 11, 2019
…tract-upload-media-phase-2
@malinajirka malinajirka mentioned this pull request Nov 11, 2019
2 of 2 tasks complete

suspend fun loadMediaByRemoteId(
site: SiteModel,
mediaModelsRemoteIds: Iterable<Long>

This comment has been minimized.

Copy link
@planarvoid

planarvoid Nov 12, 2019

Contributor

I'd just keep this as a list (as well as all the other iterables)

Copy link
Contributor

planarvoid left a comment

Looks good and works well 👍 good job. Consider my suggestions

@malinajirka

This comment has been minimized.

Copy link
Contributor Author

malinajirka commented Nov 12, 2019

Thanks @planarvoid for the review! I've implemented the suggested improvements and commented on the rest;). It's ready for another round 🙇 .

Copy link
Contributor

planarvoid left a comment

thanks for the changes, looks good 👍

@planarvoid planarvoid merged commit 6585b62 into master-edit-post-activity-refactor Nov 12, 2019
6 checks passed
6 checks passed
Peril Found some issues. Don't worry, everything is fixable.
Details
ci/circleci: Installable Build Your tests passed on CircleCI!
Details
ci/circleci: connected-tests Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: strings-check Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
@planarvoid planarvoid deleted the issue/10680-extract-upload-media-phase-2 branch Nov 12, 2019
@malinajirka malinajirka mentioned this pull request Nov 13, 2019
2 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.