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 photo picker logic from EditPostActivity #10608

Merged
merged 13 commits into from
Oct 21, 2019

Conversation

oguzkocer
Copy link
Contributor

This PR extracts photo picker related logic from EditPostActivity into a new class EditorPhotoPicker. 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.

The first commit where PostLoadingState is moved to a different file does NOT belong to this PR. However, before I started working on photo picker, I made that change and forgot to branch off. I can definitely remove that commit, but it's such a straightforward change that I think it'll be easier to review it as part of this PR rather than doing a separate one. Let me know if you disagree @malinajirka.

I suggest reviewing commit by commit instead of as a whole. At least checking out each commit should help with the general review.

To test:

  • Test the photo picking capabilities in the editor. I've tested adding media from both the editor itself and post settings, but further testing might be necessary, I am just not sure where. @malinajirka If you know what else we should test, could you also let me know so I can do it as well?

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.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Oct 14, 2019

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

@malinajirka malinajirka self-assigned this Oct 21, 2019
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.

Thanks @oguzkocer! I've checked the code and it looks good overall.

However, I've encountered this crash which doesn't seem to be happening on develop.

  1. Open Editor
  2. Open PhotoPicker
  3. Rotate the device
  4. Boom
E/AndroidRuntime: FATAL EXCEPTION: main
    Process: org.wordpress.android.beta, PID: 20254
    java.lang.RuntimeException: Unable to resume activity {org.wordpress.android.beta/org.wordpress.android.ui.posts.EditPostActivity}: java.lang.IllegalStateException: Fragment already added: PhotoPickerFragment{ffb26b7 #5 id=0x7f0903cf photo_picker}
        at android.app.ActivityThread.performResumeActivity(ActivityThread.java:4205)
        at android.app.ActivityThread.handleResumeActivity(ActivityThread.java:4237)
        at android.app.servertransaction.ResumeActivityItem.execute(ResumeActivityItem.java:52)
        at android.app.servertransaction.TransactionExecutor.executeLifecycleState(TransactionExecutor.java:176)
        at android.app.servertransaction.TransactionExecutor.execute(TransactionExecutor.java:97)
        at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2016)
        at android.os.Handler.dispatchMessage(Handler.java:107)
        at android.os.Looper.loop(Looper.java:214)
        at android.app.ActivityThread.main(ActivityThread.java:7356)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:492)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:930)
     Caused by: java.lang.IllegalStateException: Fragment already added: PhotoPickerFragment{ffb26b7 #5 id=0x7f0903cf photo_picker}
        at androidx.fragment.app.FragmentManagerImpl.addFragment(FragmentManager.java:1916)
        at androidx.fragment.app.BackStackRecord.executeOps(BackStackRecord.java:765)
        at androidx.fragment.app.FragmentManagerImpl.executeOps(FragmentManager.java:2625)
        at androidx.fragment.app.FragmentManagerImpl.executeOpsTogether(FragmentManager.java:2411)
        at androidx.fragment.app.FragmentManagerImpl.removeRedundantOperationsAndExecute(FragmentManager.java:2366)
        at androidx.fragment.app.FragmentManagerImpl.execPendingActions(FragmentManager.java:2273)
        at androidx.fragment.app.FragmentController.execPendingActions(FragmentController.java:391)
        at androidx.fragment.app.FragmentActivity.onResume(FragmentActivity.java:517)
        at org.wordpress.android.ui.posts.EditPostActivity.onResume(EditPostActivity.java:686)
        at android.app.Instrumentation.callActivityOnResume(Instrumentation.java:1446)
        at android.app.Activity.performResume(Activity.java:7939)
        at android.app.ActivityThread.performResumeActivity(ActivityThread.java:4195)
        at android.app.ActivityThread.handleResumeActivity(ActivityThread.java:4237) 
        at android.app.servertransaction.ResumeActivityItem.execute(ResumeActivityItem.java:52) 
        at android.app.servertransaction.TransactionExecutor.executeLifecycleState(TransactionExecutor.java:176) 
        at android.app.servertransaction.TransactionExecutor.execute(TransactionExecutor.java:97) 
        at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2016) 
        at android.os.Handler.dispatchMessage(Handler.java:107) 
        at android.os.Looper.loop(Looper.java:214) 
        at android.app.ActivityThread.main(ActivityThread.java:7356) 
        at java.lang.reflect.Method.invoke(Native Method) 
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:492) 
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:930) 
2019-10-21 15:23:38.761 20254-20254/org.wordpress.android.beta E/WordPress-EDITOR: HTML content of Aztec Editor before the crash:

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Oct 21, 2019

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

@oguzkocer
Copy link
Contributor Author

Thanks @oguzkocer! I've checked the code and it looks good overall.

However, I've encountered this crash which doesn't seem to be happening on develop.

1. Open Editor
2. Open PhotoPicker
3. Rotate the device
4. Boom

Thanks for catching this @malinajirka! This happened because while moving the code to Kotlin, I accidentally took out the fragment addition code from under the null check if we had. 3061e49 should fix it.

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.

Thanks @oguzkocer! I've tested it after the change and it looks good;).

@malinajirka malinajirka merged commit ee2d556 into develop Oct 21, 2019
@malinajirka malinajirka deleted the issue/extract-editor-photo-picker branch October 21, 2019 20:44
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.

2 participants