-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Add Crop functionality #6543
Add Crop functionality #6543
Conversation
2d87143
to
a4500ed
Compare
I want to explicitly note that there is a bounty on the upstream issue and @NightXlt should get the proceeds https://www.bountysource.com/issues/29271594-crop-edit-image-before-adding-to-a-note - it was important to reshape his PR a bit but all I did was bend his work containing the original effort to the current codebase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is one of those things that I'll want to test manually.
It looks OK, but there are so many edge cases that we're bound to run into which are going to slip through the testing net.
Personally, that's acceptable for a new feature as long as we don't have code paths which will crash, and we're performing appropriate exception reporting. Could we add a (beta) label to the Crop Image button, just to set expectations?
Could we get a TODO that this should ideally be both unit tested and instrumentation tested, as we're got a very high regression risk due to the variety of implementations, APIs and inputs
I'll also note that there are currently bugs in this area, which revolve around trying to extract a file from a URI, rather than copying out of a InputStream
from a ContentProvider
: #5973
AnkiDroid/src/main/java/com/ichi2/anki/multimediacard/activity/MultimediaEditFieldActivity.java
Outdated
Show resolved
Hide resolved
Uri uri = FileProvider.getUriForFile(this, this.getApplicationContext().getPackageName() + ".apkgfileprovider", file); | ||
DecimalFormat decimalFormat = new DecimalFormat(".00"); | ||
String size = decimalFormat.format(length); | ||
String content = getString(R.string.save_dialog_content, size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe go with Formatter.formatShortFileSize
.
You could bring the format string inside the resource.
AnkiDroid/src/main/java/com/ichi2/anki/multimediacard/fields/BasicImageFieldController.java
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/multimediacard/fields/BasicImageFieldController.java
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/multimediacard/fields/BasicImageFieldController.java
Show resolved
Hide resolved
} else if ("content".equalsIgnoreCase(uri.getScheme())) { | ||
imagePath = getImagePathFromContentResolver(context, uri, null); | ||
} else if ("file".equalsIgnoreCase(uri.getScheme())) { | ||
imagePath = uri.getPath(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has this path been exercised on a later API? I know there were changes to the way file://
data could be accessed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of changes - I exercised all the functions on later APIs, but I'm not sure if every case was hit. It should be that on later APIs people are providing files via content provider so this is not actually hit, but I'm not sure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my understanding: yes and no. they should, but I think a user using an old file browser on a later API can still trigger a crash on this path (or sometimes using the file path in a content provider, but we check this path already).
Might just be best to confirm that exchanging these paths for a throw
will send an exception report and won't crash the app.
Where's the can of worms emoji?
AnkiDroid/src/main/java/com/ichi2/anki/multimediacard/fields/BasicImageFieldController.java
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/multimediacard/fields/BasicImageFieldController.java
Show resolved
Hide resolved
Nice - thanks for the review. Lots to chew on, I'll address what I can, but I don't have the time to add automated tests as I was just carrying this over the finish line. It was all tested as mentioned in a full skew of emulators from the perspective of the API breaks in the code. It was asserted quite strongly in the original PR that there are device-specific faults requiring some of the strangest bits. I thought about labeling beta or something as well, but honestly it seems to work well, so I didn't. I'll address everything in a batch in the next day or two though |
Thanks, agreed that (sadly) it's not the right time/place to be adding tests, but it's one of the aspects of the program that could really do with it. Addressing the points in the OP, Anki's somewhat moved from the concept of "One image per field" to "potentially mixed content per field". We're still mostly following the old model in our note editor, and that'll cause UX issues. |
Maybe next three days I would be available, I will add some comment and give you reply @mikehardy @david-allison-1 |
Here's a parallel APK you can use for side-by-side testing (I chose the 'D' variant randomly) for easy testing if you guys want. @NightXlt I would especially love if you can check it on the devices you knew had problems (Huawei? Xioami? I'm not sure) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
- Load Image From Gallery
- Crop
- Back
"You may wish to compress" message shows.
- Crop
- "Couldn't load the image"
- Select Image from Gallery (471KB)
- Crop
- Image is now 1.02MB. I assume this is the compression stage making things larger.
- Save
- Current image size is X, do you want to crop it?
- "Current image size is X, do you want to crop it".
- "Cancel" is ambiguous here.
- Maybe "Crop", "Save/Continue" "Cancel"?
What does a user expect out of "crop" if they have two cards with the same image? Currently it only affects one card.
It would be useful to add file sizes as Timber.d output at each stage to see what is happening where That first series you did is something interesting, What API? When you say "Back", like back button, or back like the checkmark to go back? Crop copies before edit at this point, I think that's the majority use case. I could see it going either way but clobbering would be altering the code of course |
The Android OS back button on my device. "Crop Image" - select "Crop Picture" (Huawei intent for cropping) - "Press Android Back button (triangle)" once the cropping UI appears |
I changed the dialog to "OK" vs "No" because Cancel was ambiguous yes I changed the default rotateAndCompress format from PNG @90% to JPG @90% after adding size statements that showed rotateAndCompress was bloating the size. Now it's actually shrinking. Why was it PNG before? No idea Just curious, were you running with system dev setting don't save activities? Because none of this stuff in advanced editor works with that AFAICT Still working through your scenarios but cropping fails on macOS emulators, I can only test it on linux ugh |
Could a TODO/Defect Ticket/Fix be added:
AFAIK, no. Taking camera images or selecting from my phone gallery doesn't work if Don't Keep Activities is enabled. Let me know which scenarios you'd like better debugging output for |
The filename changes every time in our implementation so there will never be collision Your scenarios seem fine just I switch between main machine every couple days (from Linux to macOS in order to do Xcode-y react-native-y iOS-y things) and I was on my macOS machine last couple days, which doesn't handle crop in emulators, so haven't been able to test that. Will tack this down when I'm back on linux later today |
also improve error handling in rotateAndCompress start setting up for handling crop results
whew! This one is better. Still hard to tease apart the commits post-hoc but there was a reason to do this much change:
You'll probably find a scenario that breaks it but it's getting more hardened and featureful at the same time so I'm not too concerned if you can still break it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still needs a manual test
AnkiDroid/src/main/java/com/ichi2/anki/multimediacard/activity/MultimediaEditFieldActivity.java
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/multimediacard/activity/MultimediaEditFieldActivity.java
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/multimediacard/fields/BasicImageFieldController.java
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/multimediacard/fields/BasicImageFieldController.java
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/multimediacard/fields/BasicImageFieldController.java
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/multimediacard/fields/BasicImageFieldController.java
Outdated
Show resolved
Hide resolved
Happy path works well. One bug relating to invalid images (unsure if we want to hold this for later, or deal with now).
Note line:
logcat```2020-06-28 16:24:16.821 9481-9481/com.ichi2.anki W/Settings: Setting device_provisioned has moved from android.provider.Settings.Secure to android.provider.Settings.Global.
|
Pretty easy to roll through at least the most common unhappy path, I'll throw invalid files at it and see what happens, what I suspect will happen based on what you saw in the logcat is that it will suss out something with regard to the temp-file cleanup work I did where I mess up the accounting somewhere and clean up the wrong thing. Glad the happy path works! The internalizeFile / URI-handling stuff should be generalizable later for imports and if the person from the mailing list wants to add media insert into the ContentProvider even |
I'd recommend adding a EDIT: No exception EDIT: Actually... maybe |
Arrays are convenient for the same reason they are dangerous, using a Pair in place makes it impossible to have anything other than 2 elements, which was the prior intention
Makes me think that in the getting help screen we should have a "send exception report right now" button that clears any ACRALimiter clamping and does a broader logcat sweep. If users encounter random problems here or in other parts of the app and we have Timber categorizing correctly, we'll get traces like you just gave me, which would help a lot for recoverable errors |
This is where the Bitmap.decode happens, so it can provide useful signal in case something about the image crop or camera selection resulted in a bad image combined with image picker clamped to actual images, we should only allow valid images to pass through the controller to NoteEditor for save now
added in all sorts of protection against ingesting or using bad data, should handle most bad cases |
I'm going to go ahead and merge this one, it's passing all our happy path tests and I can't make it fail anywhere any more. I will obviously hop on anything that needs mop up but I want to generate an alpha of what will likely become 2.12 |
Pull Request template
Purpose / Description
There was no way to crop images previously, but cameras especially are generating very large images these days
This allows the advanced editor's image field component to crop images, and prompts users to do so for photos or if the image is large
This is entirely based on the work from @NightXlt - everything about how to do the Intent correctly, to the URI / File path conversion is their work
I took the PR contents of #5301 and re-factored them to fit better with the work done since @NightXlt originally submitted the work, then I tested, tested, and tested some more with little fixes related to preview handling and button visibility added in.
Finally, I deconstructed the work into chunks as small as I could so review would be easier.
Please understand the commit separation was done after the fact and there may be a couple cross-dependencies still present in the commit. The separate commits are for easier review but I make no guarantee they will function separately, testing has only been performed with the complete work
Fixes
Fixes #2554
Obsoletes #5301
Approach
The basic idea is to
How Has This Been Tested?
API16, 18, 24, 29, 30 emulators
The scenarios I ran are:
Note that after adding images but before you save the note, the field has data and the paperclip menu behaves differently than if you were editing a note and it had data because the paperclip menu listener hasn't re-been rebound. This is an existing bug and not addressed here
Same as above but with an existing note with image data:
Learning (optional, can help others)
Of interest from this PR:
Attempting to implement an "EDIT" intent vs stricly crop is better but fails in mysterious ways, especially with API30
API30 in general may require more testing
Of interest that existed prior to the PR and still exist:
The advanced editor is rough to deal with, it's pretty fluffy with regard to number of objects and how control passes around
The paperclip menu behaves inconsistently right now based on whether the note had data in the field to start or was empty. If empty to start it will show a popup menu, if it had data to start it sends you right to the advanced editor.
The advanced editor doesn't handle the case of multiple things in the same a field very well.
The advanced editor doesn't handle the case of editing a multimedia field very well (it just keeps appending the edits) but at least with preview from note editor this is easy to fix
Checklist
Please, go through these checks before submitting the PR.
if
statements)