Refactor 'showCancelMediaUploadDialog' to Prevent Dismissal on Screen Rotation by SiobhyB · Pull Request #14810 · wordpress-mobile/WordPress-Android

SiobhyB · 2021-06-09T10:56:41Z

Description

Partial fix for #14676

The showCancelMediaUploadDialog, which is shown when tapping an image that's still in the process of uploading, is currently dismissed/lost with a screen rotation.

This can lead to buggy behavior, as shown in the following screenshots, where the image will permanently look like it's still uploading, with any attempts to tap on it resulting in the dialog appearing again, even though the image has been uploaded to the site's media library:

Before Rotation	After Rotation

This PR seeks to fix the above issue by refactoring the dialog so that it survives screen rotation. A custom dialog fragment, GutenbergDialogFragment, is used to achieve this.

GutenbergDialogFragment

A new GutenbergDialogFragment class was introduced in #14503. This class is based on the BasicFragmentDialog class, with some key differences being the removal of the third setNeutralButton, an addition of a mediaId to the constructor, and the ability for the parent fragment to implement the positive/negative button interfaces.

Part of the goal behind creating the new GutenbergDialogFragment class was to allow for easier refactoring of the other dialogs listed in #14676, which are all currently lost/dismissed with a screen rotation. With that goal in mind, the mediaId param is refactored with this PR to utilize generics and accept any data type. Its message param has also been made optional, as not all dialogs declared in GutenbergEditorFragment include a message.

The aforementioned changes make it relatively straightforward to refactor showCancelMediaUploadDialog to use the custom dialog fragment.

Testing Steps

Navigate to the post editor.
Add a new image block and choose the option to upload an image directly from your device.
Immediately tap on the image block as the upload begins in order to generate the "cancel" dialog.
Switch the orientation of the device and confirm that the dialog is still in place.
Tap the "no" or "yes" buttons after rotating the screen to confirm that they still work as expected.
Try a few combinations of rotations and button pressing to confirm there are no unintended or buggy side effects to this recent change.

Regression Notes

Potential unintended areas of impact

The main unintended side effect of this PR would be for the dialog's existing "yes" or "no" flows to break.

What I did to test those areas of impact (or what existing automated tests I relied on)

I following the outlined testing steps, with various combinations of rotations and button pressing to see if I could create any problems or issues.

What automated tests I added (or what prevented me from doing so)

Automated tests didn't feel warranted given the size of this change.

PR submission checklist:

I have completed the Regression Notes.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

The 'id' param is currently bound to an Int type. This reflects the fact that it's currently only being used to generate one dialog, which passes a mediaId. Ideally, it should be possible to pass any piece of data to the class from Gutenberg. See this comment for more context: #14676 (comment) With the changes proposed in this commit, the 'id' param is refactored to make use of generics, so that it isn't bound to any specific type. It is also renamed to 'dataFromGutenberg', in an attempt to make its purpose clearer, which is to pass in any piece of data from the editor to the dialog.

peril-wordpress-mobile · 2021-06-09T10:56:45Z

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

The dialog we're refactoring as part of this PR doesn't have a 'message' value. This value is currently required by default with the GutenbergDialogFragment that's going to be used. With this commit, GutenbergDialogFragment is changed so that it's possible to pass 'null' as a 'message'.

peril-wordpress-mobile · 2021-06-09T11:10:49Z

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

With this commit, the 'showCancelMediaUploadDialog()' function is refactored to call GutenbergDialogFragment. GutenbergDialogFragment is initialized using the dialog's tag, title, button text, and the 'localMediaId' being passed. The logic that fires when the 'yes' button is pressed has been separated out into a new function, 'cancelMediaUpload()'.

...ressEditor/src/main/java/org/wordpress/android/editor/gutenberg/GutenbergEditorFragment.java

When a user clicks the positive button, the cancelMediaUpload() should fire, as implemented in this commit.

The 'STATE_KEY_ID' is renamed to 'STATE_KEY_DATA_FROM_GUTENBERG' with this commit, to coincide with earlier name changes to the 'id' param.

SiobhyB · 2021-06-09T12:10:09Z

👋 @planarvoid, as we previously talked a bit about this issue, I wondered if you might have some availability this week to review my first pass at fixing one of the dialogs? Let me know. :)

peril-wordpress-mobile · 2021-06-09T12:10:27Z

	Warnings
⚠️	PR is not assigned to a milestone.

Generated by 🚫 dangerJS

SiobhyB · 2021-06-10T16:07:38Z

@planarvoid, I requested you as a reviewer, but let me know if you won't be able to get to this week or next, and I can find someone else. :)

planarvoid · 2021-06-11T13:12:33Z

...dPressEditor/src/main/java/org/wordpress/android/editor/gutenberg/GutenbergDialogFragment.kt

+class GutenbergDialogFragment<T : Serializable?>() : AppCompatDialogFragment() {
    private lateinit var mTag: String
-    private lateinit var mMessage: CharSequence
+    private var mMessage: CharSequence? = null


we shouldn't use the Hungarian notation in Kotlin 👍

planarvoid · 2021-06-11T13:12:52Z

...dPressEditor/src/main/java/org/wordpress/android/editor/gutenberg/GutenbergDialogFragment.kt

-    interface GutenbergDialogPositiveClickInterface {
-        fun onGutenbergDialogPositiveClicked(instanceTag: String, id: Int)
+    interface GutenbergDialogPositiveClickInterface<T> {
+        fun onGutenbergDialogPositiveClicked(instanceTag: String, dataFromGutenberg: Any?)


I assume the type of dataFromGutenberg should be T, not Any

planarvoid · 2021-06-11T13:25:35Z

Thanks for the PR!
I don't think it's a good idea to have the GutenbergDialogFragment as generic (I'm honestly not sure how that would work with creating and restoring fragments in the fragment manager). What kind of data do you expect to pass around? Could we maybe use a Parcelable? or something stored in a database? Or an array? I think it's usually cleaner to not do inheritance in cases like this. Doing an implementation of the dialog fragment for each case should be simple enough, right?
Also I think it would be much cleaner to do something like a GutenbergDialogViewModel - a class that would be responsible for showing, hiding and handling the dialogs, instead of using the old approach with the fragment implementing the positive/negative interface. This would be much more in line with the way we want the app to be written (check StatsSiteSelectionViewModel and StatsWidgetSiteSelectionDialogFragment).

SiobhyB · 2021-06-11T15:39:37Z

I don't think it's a good idea to have the GutenbergDialogFragment as generic (I'm honestly not sure how that would work with creating and restoring fragments in the fragment manager).

Thank you for your candid feedback, would you be able to help me understand the concern with how the fragment manager creates/restores fragments using this approach? What type of problems could it cause?

What kind of data do you expect to pass around? Could we maybe use a Parcelable? or something stored in a database? Or an array? I think it's usually cleaner to not do inheritance in cases like this.

The data varies for each of the six dialogs in GutenbergEditorFragment that need to be refactored. In the case of this PR, the data is a media ID (an integer), but there are other dialogs that pass media files (stored in an ArrayList).

I can definitely research the other options you mentioned and will seek to gain a better understanding of why they're more suitable as part of that research.

Doing an implementation of the dialog fragment for each case should be simple enough, right?

To make sure I understand you correctly, are you suggesting that I create six separate dialog fragments for each of the dialogs declared in GutenbergEditorFragment? That does seem simple on the face of it, but also like it'll mean a lot of copying/pasting and duplicated code. I'll need to take some time to research and understand the concerns you've raised in order to understand the reason why this is a better approach.

Also I think it would be much cleaner to do something like a GutenbergDialogViewModel - a class that would be responsible for showing, hiding and handling the dialogs, instead of using the old approach with the fragment implementing the positive/negative interface. This would be much more in line with the way we want the app to be written (check StatsSiteSelectionViewModel and StatsWidgetSiteSelectionDialogFragment).

Thank you for the context around the pattern I followed being an older approach. It's definitely tricky to distinguish between old and new approaches when looking for patterns as a junior exploring the codebase. In this case, the pattern I copied was from the BasicFragmentDialog class. Do you think it'd be worthwhile for me to create a PR to add a summary of what you mentioned around the preferable approach to the comments of that file?

Thanks again in advance for your patience with my questions as I learn and sharing your knowledge here. 🙇‍♀️

planarvoid · 2021-07-01T12:04:57Z

thanks for the reply!

Thank you for your candid feedback, would you be able to help me understand the concern with how the fragment manager creates/restores fragments using this approach? What type of problems could it cause?
Sorry if my feedback sounded harsh. It wasn't my intention. The generics get stripped during compilation so if I understand it correctly, something like this:

FragmentA<Integer> fragment = (FragmentA<Integer>) fm.findFragmentByTag(TAG);

is not a safe cast. This is what Android Studio tells you:

Unchecked cast: 'androidx.fragment.app.Fragment' to 'org.wordpress.android.ui.mlp.ModalLayoutPickerFragment<java.lang.Integer>'

I think it will still work but I wouldn't do it. A check like this:

fragment is FragmentA<Integer>

will not work in Kotlin.

I can definitely research the other options you mentioned and will seek to gain a better understanding of why they're more suitable as part of that research.

I think Parcelable might work well in this case.

To make sure I understand you correctly, are you suggesting that I create six separate dialog fragments for each of the dialogs declared in GutenbergEditorFragment? That does seem simple on the face of it, but also like it'll mean a lot of copying/pasting and duplicated code. I'll need to take some time to research and understand the concerns you've raised in order to understand the reason why this is a better approach.

What I meant is that you can have 6 children without any logic that would just inherit from the GutenbergEditorFragment and define their own resources/type and pass it up to the parent using the constructor. It could improve readibility.

Thank you for the context around the pattern I followed being an older approach. It's definitely tricky to distinguish between old and new approaches when looking for patterns as a junior exploring the codebase. In this case, the pattern I copied was from the BasicFragmentDialog class. Do you think it'd be worthwhile for me to create a PR to add a summary of what you mentioned around the preferable approach to the comments of that file?

I think we can add that. That's a good idea! Sadly we don't have enough time to deal with the technical debt so we keep the old files around as long as they are used somewhere.

SiobhyB · 2022-01-20T16:24:43Z

I'm going to go ahead to close this PR as it's not currently being worked on or a priority.

SiobhyB added gutenberg-mobile [Type] Bug Gutenberg Editing and display of Gutenberg blocks. Posting/Editing labels Jun 9, 2021

SiobhyB added 2 commits June 9, 2021 12:40

Introduce tag for showCancelMediaUploadDialog

e2baf52

wpmobilebot reviewed Jun 9, 2021

View reviewed changes

...ressEditor/src/main/java/org/wordpress/android/editor/gutenberg/GutenbergEditorFragment.java Outdated Show resolved Hide resolved

SiobhyB added 3 commits June 9, 2021 12:49

Call cancelMediaUpload() upon request

d111f20

When a user clicks the positive button, the cancelMediaUpload() should fire, as implemented in this commit.

Rename constant for clarity

299dffe

The 'STATE_KEY_ID' is renamed to 'STATE_KEY_DATA_FROM_GUTENBERG' with this commit, to coincide with earlier name changes to the 'id' param.

Split line to resolve lint error

7d493c6

SiobhyB mentioned this pull request Jun 9, 2021

Dialogs Declared in GutenbergEditorFragment Lost/Dismissed with Screen Rotation #14676

Open

5 tasks

SiobhyB marked this pull request as ready for review June 9, 2021 12:10

SiobhyB requested a review from planarvoid June 10, 2021 16:07

planarvoid reviewed Jun 11, 2021

View reviewed changes

Merge branch 'develop' into issue/14676-fix-cancel-media-upload-dialog

7121408

SiobhyB closed this Jan 20, 2022

SiobhyB deleted the issue/14676-fix-cancel-media-upload-dialog branch July 9, 2022 20:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor 'showCancelMediaUploadDialog' to Prevent Dismissal on Screen Rotation#14810

Refactor 'showCancelMediaUploadDialog' to Prevent Dismissal on Screen Rotation#14810
SiobhyB wants to merge 8 commits intotrunkfrom
issue/14676-fix-cancel-media-upload-dialog

SiobhyB commented Jun 9, 2021 •

edited

Loading

Uh oh!

peril-wordpress-mobile bot commented Jun 9, 2021 •

edited

Loading

Uh oh!

peril-wordpress-mobile bot commented Jun 9, 2021 •

edited

Loading

Uh oh!

Uh oh!

SiobhyB commented Jun 9, 2021

Uh oh!

peril-wordpress-mobile bot commented Jun 9, 2021

Uh oh!

SiobhyB commented Jun 10, 2021

Uh oh!

planarvoid Jun 11, 2021

Uh oh!

planarvoid Jun 11, 2021

Uh oh!

planarvoid commented Jun 11, 2021 •

edited

Loading

Uh oh!

SiobhyB commented Jun 11, 2021

Uh oh!

planarvoid commented Jul 1, 2021

Uh oh!

SiobhyB commented Jan 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

SiobhyB commented Jun 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

GutenbergDialogFragment

Testing Steps

Regression Notes

Uh oh!

peril-wordpress-mobile bot commented Jun 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peril-wordpress-mobile bot commented Jun 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

SiobhyB commented Jun 9, 2021

Uh oh!

peril-wordpress-mobile bot commented Jun 9, 2021

Uh oh!

SiobhyB commented Jun 10, 2021

Uh oh!

planarvoid Jun 11, 2021

Choose a reason for hiding this comment

Uh oh!

planarvoid Jun 11, 2021

Choose a reason for hiding this comment

Uh oh!

planarvoid commented Jun 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SiobhyB commented Jun 11, 2021

Uh oh!

planarvoid commented Jul 1, 2021

Uh oh!

SiobhyB commented Jan 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SiobhyB commented Jun 9, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Jun 9, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Jun 9, 2021 •

edited

Loading

planarvoid commented Jun 11, 2021 •

edited

Loading