Skip to content

Issue/11145 start ucrop with high res image#11376

Merged
ashiagr merged 12 commits intofeature/media-editing-basics-phase_onefrom
issue/11145-start-ucrop-with-high-res-image
Feb 26, 2020
Merged

Issue/11145 start ucrop with high res image#11376
ashiagr merged 12 commits intofeature/media-editing-basics-phase_onefrom
issue/11145-start-ucrop-with-high-res-image

Conversation

@ashiagr
Copy link
Copy Markdown
Contributor

@ashiagr ashiagr commented Feb 25, 2020

Fixes #11145

This PR

  • Starts uCrop from the preview fragment with the high res image file path.
  • Returns to the original post on the Gutenberg editor skipping preview fragment on back action.

P.S.
Few observations related to uCrop fragment which may require separate tasks (adding them as a checklist):

  • 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.
    Using popUpToInclusive property 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 uCrop fragment. Will further investigate it when we pass edited image to the Gutenberg editor.

    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.

device-2020-02-25-140701

  • The cropping rectangle in the uCrop fragment is currently shown behind the toolbar. As a result we cannot drag upper corners. Propagating events to the view behind (by setting clickable, focusable to false) doesn't seem to be working in case of Toolbar as it eats all touch events, regardless of the clickable attribute and @null background.
@Override
public boolean onTouchEvent(MotionEvent ev) {
  // Toolbars always eat touch events

Will take care of this issue in a separate PR.
UPDATE: Will take care of it in this issue: #11387.

  • 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.

    Related uCrop issue 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 uCrop on 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:

    If a lifecycle becomes inactive, it receives the latest data upon becoming active again. For example, an activity that was in the background receives the latest data right after it returns to the foreground.

    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.

LR_image

PR submission checklist:

  • I have considered adding unit tests where possible.
  • 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.

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
@ashiagr ashiagr added this to the 14.4 milestone Feb 25, 2020
@ashiagr ashiagr self-assigned this Feb 25, 2020
@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Feb 25, 2020

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

Copy link
Copy Markdown
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 @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?

@ashiagr
Copy link
Copy Markdown
Contributor Author

ashiagr commented Feb 26, 2020

Great review @malinajirka ! I've addressed most of your comments. Answered some here:

We have a FrameLayout there which draws the toolbar on top (z-axis) of the fragment instead of drawing it above (y-axis).

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 ImageView behind it. It could have benefited us in the landscape mode where we have really less space.

device-2020-02-26-095712

Then thought of setting app bar background to @null and allow touch events to reach the ImageView behind it and have a free style crop rectangle. I can discuss the design with @mbshakti and update it accordingly.

Is the FrameLayout even necessary?

Removed, it was an oversight 🤦‍♀️.

I'd suggest leaving this issue to the end of the project after everything else is done. Wdyt?

Agree

Imo this workaround is ok, especially since we'll want to have a different behavior when we introduce support for editing multiple images. Wdyt?

Agree

I noticed there isn't any "Done" button in the uCrop fragment. Is that something we plan to add?

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 popToInclusive in that task.
I plan to use the UCropResult received here:

I will create a separate task for it.

@ashiagr ashiagr requested a review from malinajirka February 26, 2020 06:13
Copy link
Copy Markdown
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.

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! ;)

@ashiagr ashiagr merged commit 06dbaf6 into feature/media-editing-basics-phase_one Feb 26, 2020
@malinajirka malinajirka deleted the issue/11145-start-ucrop-with-high-res-image branch February 27, 2020 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Media [Status] Needs Design Review A designer needs to sign off on the implemented design.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants