wordpress-mobile / WordPress-Android

WordPress for Android
http://android.wordpress.org
GNU General Public License v2.0
2.99k stars 1.32k forks source link

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

Open SiobhyB opened 3 years ago

SiobhyB commented 3 years ago

Expected behavior

I expected dialogs to remain on screen, even when the orientation is changed from portrait to landscape.

Actual behavior

The following dialogs declared in the GutenbergEditorFragment file are lost/dismissed with screen rotation:

This is not only unexpected but can lead to some buggy behaviour, an example of which is given in the below steps to reproduce.

Steps to reproduce the behavior

Steps for showCancelMediaUploadDialog

Additional buggy behaviour to note from this specific example: The image will appear to be permanently in an "uploading" state and any attempts to tap on it result in the "cancel" dialog appearing again, even though the image has been uploaded to the site's media library.

The "after" from the following screenshot shows the loading indicator overlaying the image, following the above steps, even though the image has actually been uploaded to the site's media library. If I tap on the image in this case, the "cancel" dialog appears again, even though it's not actually possible to prevent the upload.

Before Rotation After Rotation

Steps for Other Dialogs

showCancelMediaCollectionSaveDialog - Navigate to the post editor. - Add a new story block and choose the option to upload multiple images directly from your device. - Immediately tap on the story block as the uploads begin in order to generate the dialog. - Switch the orientation of the device and observe how the dialog is lost.
showCancelMediaCollectionUploadDialog - Navigate to the post editor. - Add a new story block and choose the option to upload multiple images directly from your device. - Immediately tap on the story block as the uploads begin. The `showCancelMediaCollectionSaveDialog` will be the first one to display, but the `showCancelMediaCollectionUploadDialog` will eventually display some time between the image saving and the uploads completing. - Switch the orientation of the device and observe how the dialog is lost.
showRetryMediaCollectionUploadDialog - Navigate to the post editor. - Add a new story block and choose the option to upload multiple images directly from your device. - Immediately enable airplane mode in order to prevent the images from uploading. - Return to the story block and top on it to generate the dialog. - Switch the orientation of the device and observe how the dialog is lost.
showRetryMediaUploadDialog - Navigate to the post editor. - Add a new image block and choose the option to upload a new image directly from your device. - Immediately enable airplane mode in order to prevent the image from uploading. - Return to the image block and tap on it to generate the dialog. - Switch the orientation of the device and observe how the dialog is lost.
screenshots for reference | showCancelMediaCollectionSaveDialog | showCancelMediaCollectionUploadDialog | | ------------- | ------------- | | | | | showRetryMediaCollectionUploadDialog | showRetryMediaUploadDialog | | ------------- | ------------- | | | |
Tested on Pixel 5, Android 11, WPAndroid 17.1
SiobhyB commented 3 years ago

The background to this issue being created can be found in https://github.com/WordPress/gutenberg/pull/31705#issuecomment-842242744. I'm looking to add a new showFeaturedImageConfirmationDialog dialog to this file and would like it to persist even when the screen is rotated.

From some initial research, I believe we might be able to utilize the BasicFragmentDialog class here to counter the problem: https://github.com/wordpress-mobile/WordPress-Android/blob/develop/WordPress/src/main/java/org/wordpress/android/ui/posts/BasicFragmentDialog.kt

I'd be grateful for any feedback around whether I'm on the right path with this, before I fall too deep down a rabbit hole.

Slack discussion for reference: p1621335436003900-slack-C02QANACA

planarvoid commented 3 years ago

Hey @SiobhyB ! You're on the right track. For Dialog to survive rotation we have to use DialogFragment instead 👍. We should never use only Dialogs. You could but don't have to extend BasicFragmentDialog. Creating a custom dialog fragment is quite simple. For example org.wordpress.android.ui.prefs.homepage.HomepageSettingsDialog

SiobhyB commented 3 years ago

A new GutenbergDialogFragment.kt class was created as part of https://github.com/wordpress-mobile/WordPress-Android/pull/14503 as a starting point for addressing this issue. @jd-alexander has the following feedback for optimizing the class and making it more re-usable:

Ideally, I would utilize generics, so we could make it a param value that isn't bound to a type and in this case, we could pass the Int but yes, we don't have to do that optimization at this time so we could probably evaluate changing the name for now.

SiobhyB commented 2 years ago

Removing my assignment to reflect the fact I don't have plans to work on this in the immediate future. I had made an initial pass at the code in https://github.com/wordpress-mobile/WordPress-Android/pull/14810, which might be useful if others take over this issue.