Issue/11145 start ucrop with high res image#11376
Issue/11145 start ucrop with high res image#11376ashiagr merged 12 commits intofeature/media-editing-basics-phase_onefrom
Conversation
popUpToInclusive is set to skip showing preview fragment on returning from ucrop fragment
Using popUpToInclusive for popping the start destination of the graph off the back stack in a multi-module project doesn't seem to be working. Explicitly invoking back action as a workaround. Related issue: https://issuetracker.google.com/issues/147312109
|
You can test the changes on this Pull Request by downloading the APK here. |
malinajirka
left a comment
There was a problem hiding this comment.
Thanks @ashiagr! The changes look great overall. I've left a couple of comments.
The cropping rectangle in the uCrop fragment is currently shown behind the toolbar.
I think the issue here is the way our activity_edit_image layout is created. We have a FrameLayout there which draws the toolbar on top (z-axis) of the fragment instead of drawing it above (y-axis). Is the FrameLayout even necessary? It feels weird to have a FrameLayout nested in ConstraintLayout - ConstraintLayout should get away without nesting any layouts, wdyt?
uCrop doesn't seem to be saving image state and restoring it on orientation change (at least with the version we're using2.2.2). Will investigate it more in a separate task.
I'd suggest leaving this issue to the end of the project after everything else is done. Wdyt?
On back action from the app bar on the uCrop fragment, we want to go to the original post on the Gutenberg editor skipping preview fragment.
Imo this workaround is ok, especially since we'll want to have a different behavior when we introduce support for editing multiple images. Wdyt?
I noticed there isn't any "Done" button in the uCrop fragment. Is that something we plan to add?
ImageEditor/src/main/kotlin/org/wordpress/android/imageeditor/preview/PreviewImageFragment.kt
Outdated
Show resolved
Hide resolved
ImageEditor/src/main/kotlin/org/wordpress/android/imageeditor/preview/PreviewImageFragment.kt
Outdated
Show resolved
Hide resolved
ImageEditor/src/main/kotlin/org/wordpress/android/imageeditor/preview/PreviewImageViewModel.kt
Outdated
Show resolved
Hide resolved
ImageEditor/src/main/kotlin/org/wordpress/android/imageeditor/preview/PreviewImageFragment.kt
Outdated
Show resolved
Hide resolved
|
Great review @malinajirka ! I've addressed most of your comments. Answered some here:
Actually that was intentional. I initially planned to not include free style crop rectangle (not requiring any touch events behind the app bar) and just provide aspect ratios similar to iOS implementation. As we're not displaying any title on the app bar, so thought of utilising the space behind it by extending the Then thought of setting app bar background to
Removed, it was an oversight 🤦♀️.
Agree
Agree
Yes, I planned to include a check mark on the app bar on the right corner as part of #11146. That's why I mentioned to investigate more about |
malinajirka
left a comment
There was a problem hiding this comment.
and allow touch events to reach the ImageView behind it and have a free style crop rectangle.
I personally probably wouldn't even try clicking on the toolbar + the corner might be displayed under the back/done buttons, so it won't be reachable. I don't think it matters that the image will be smaller, so I'd personally limit its space so it's displayed below (y-axis) the toolbar. But let's leave this decision to Shakti.
Everything else LGTM, feel free to merge this PR and tackle ☝️ in a new PR. Great job! ;)

Fixes #11145
This PR
uCropfrom the preview fragment with the high res image file path.Gutenbergeditor skipping preview fragment on back action.P.S.
Few observations related to
uCropfragment which may require separate tasks (adding them as a checklist):On back action from the app bar on the
uCropfragment, we want to go to the original post on theGutenbergeditor skipping preview fragment.Using
popUpToInclusiveproperty for this purpose doesn't pop the start destination (preview fragment) off the back stack in a module.On doing so, activity finishes and then a failed attempt is made to reopen the start destination. As a workaround, explicitly invoking back action for the
uCropfragment. Will further investigate it when we pass edited image to theGutenbergeditor.Related Android issue here.
UPDATE: As discussed in comments, workaround looks ok.
Using temporary options (aspect ratios, crop frame/ grid) and styling as shown in this screenshot. Will update with actual options after a discussion with the designer.
cc @mbshakti
UPDATE: Will take care of it in this issue: Update crop screen design #11387.
uCropfragment is currently shown behind the toolbar. As a result we cannot drag upper corners. Propagating events to the view behind (by settingclickable,focusabletofalse) doesn't seem to be working in case of Toolbar as it eats all touch events, regardless of theclickableattribute and@nullbackground.Will take care of this issue in a separate PR.
UPDATE: Will take care of it in this issue: #11387.
uCropdoesn't seem to be saving image state and restoring it on orientation change (at least with the version we're using2.2.2). Will investigate it more in a separate task.Related
uCropissue here.UPDATE: As discussed in comments, leaving this issue to the end of the project. Added a note in the Media Editing project.
Yet to decide output file path.
UPDATE: As discussed in comments, output file location is ok.
Noticed a bug, low res(LR) image was saved to file and sent to
uCropon moving the app to background while LR image was loading and bringing the app to foreground after it was loaded but before HR image was loaded.It is probably happening because
viewModel.onLoadIntoImageViewSuccess(url)is called again due to the LiveData fact:Will create an issue and fix it in a separate PR.
UPDATE: Will take care of it in this issue: Low res(LR) image is saved to file on moving the app to background and bringing it back to foreground when images are loading #11377.
PR submission checklist:
RELEASE-NOTES.txtif necessary.