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

Extract media related code from EditPostActivity to a new class #10664

Conversation

oguzkocer
Copy link
Contributor

This PR is a part of the ongoing refactor for the EditPostActivity. Borrowing from the very similar photo picker PR:

The extraction is almost a one to one conversion where the old implementation is kept the same, sometimes (possibly) to a fault. The goal is moving common logic into new components rather than improving said components, so the review should be done with this in mind. Having said that, some basic improvements are made where it's obvious that the previous logic is untouched.

This extraction is a lot trickier than the photo picker one and considering we are planning to refactor the EditorMedia component further, I think it's best to merge this PR into a feature branch instead of develop. This branch shouldn't live for a long time, so merge conflicts shouldn't be a big deal. I am also unsure about how best to test these changes, I just don't know enough about the EditPostActivity.

P.S: I have not been able to test this PR yet due to a crash I am experiencing in develop and in this branch. I'll mark the PR as draft for now.

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.

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.
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Oct 22, 2019

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

@@ -916,6 +894,7 @@ public void onPhotoPickerHidden() {
public void onPhotoPickerMediaChosen(@NonNull final List<Uri> uriList) {
mEditorPhotoPicker.hidePhotoPicker();

// TODO: It looks like we might be calling addMediaList twice
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand this comment correctly but haven't you just missed the return statement after WPMediaUtils.advertiseImageOptimization(this, () -> mEditorMedia.addMediaList(uriList, false));?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really remember what I was thinking with this. The TODO doesn't make sense 😕 Removed in 48f8d02.

@malinajirka
Copy link
Contributor

malinajirka commented Oct 23, 2019

Thanks @oguzkocer!

I've reviewed the code as best I could and it LGTM as a first iteration. Tbh I don't think we need to test this branch before we merge it into the working branch. We'll change big chunks of the code anyway so it feels like a waste of time.

SideNote: I've already started refactoring AddMediaListThread so I run some basic tests there and it worked as expected.

I'm not sure why the tests are timing out 🤷‍♂

@peril-wordpress-mobile
Copy link

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

Generated by 🚫 dangerJS

@oguzkocer
Copy link
Contributor Author

@malinajirka I fixed the lint issues and removed that TODO you left a comment on. I also tested basic functionality. I agree that we shouldn't spend too much time on testing right now. Marking this PR as ready to review!

@oguzkocer oguzkocer marked this pull request as ready for review October 23, 2019 18:14
Copy link
Contributor

@malinajirka malinajirka left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@malinajirka malinajirka merged commit 4a4db9f into issue/master-extract-editor-media Oct 23, 2019
@oguzkocer oguzkocer deleted the issue/extract-media-from-edit-post-activity branch October 25, 2019 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants