-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[SMARTSCAN] [$500] LOW: Android - Scan-When adding big image, thumbnail display is not clear, image distorted #32649
Comments
Triggered auto assignment to @abekkala ( |
Job added to Upwork: https://www.upwork.com/jobs/~0122f62b0e85d3a0e6 |
Bug0 Triage Checklist (Main S/O)
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @Ollyws ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.Scan-When adding big image, thumbnail display is not clear, image distorted What is the root cause of that problem?This is actually expected behavior What changes do you think we should make in order to solve the problem?The most important thing is we need to have access to better images Line 51 in 10727a7
But from the front side we can use the original image in case, for example, thumbnail photo width is less than 200 pixels (Or we can calculate the aspect ratio If our image is too long, then there is no point in showing thumbnail) For this, we can add new conditions and use
App/src/components/ReportActionItem/ReportActionItemImage.js Lines 49 to 73 in 10727a7
And then if
What alternative solutions did you explore? (Optional)NA |
@ZhenjaHorbach Thanks for the proposal but it seems like a bit of a workaround. @abekkala Could we have an internal engineer take a look? Thanks! |
Current assignee @Ollyws is eligible for the Internal assigner, not assigning anyone new. |
Triggered auto assignment to @yuwenmemon ( |
@yuwenmemon just wanted to get your opinion on this |
Yeah I'd rather go with cropping the image for a thumbnail on the backend than the proposed solution. Let's make this internal. |
Current assignee @Ollyws is eligible for the Internal assigner, not assigning anyone new. |
tagged Greg in #wave6-collect-submitters to get confirmation to add this to that project Edit: this may actually be wave 5 |
We can no longer download the large image attached above... Github shows the image as not available when I try to open it or download it 😕 I found two alternative images:
Testing image 1 screen-20240110-153736.mp4Testing image 2 screen-20240110-154834.mp4 |
Here's the Android Exception: we previously resolved a similar issue by switching image libraires and I'll probably handle this separately.
|
I created a separate issue for resolving the exception here, which will become my top focus. |
Beneath a height/width ratio of about 1:3, the thumbnail becomes indistinguishable: Image1: We currently lock thumbnail resolution to 1024px max. To resolve the issue we'll need to apply further logic to images with a w/h ratio greater than 3:1 in either width or height. I see a few options:
NewDot thumbnails are displayed much larger than OldDot, so we should probably limit these changes to NewDot to avoid breaking OldDot note: our thumbnail suffix for avatars is |
Based on the above, the best course of action is to crop image previews, providing they are NewDot receipts. We still want to keep the scale for tall/wide image attachments. In both OldDot and NewDot, receipts AND image attachments pass through the same Web-PDF code. The next step is to figure out how to crop with Imagemagick CLI tools and how to pass a Current Resize logic:
|
After trialing a few different ImageMagick features (resize, thumbnail, crop) today I figured out the arguments that allows me to perform both cropping and resizing in a single command. Also the ordering of these arguments appear to affect query speed. Crop and resize, but ONLY if the image exceeds 512 in width or height
For images with a width AND height beneath our desired thumbnail size, we run this query to ensure the thumbnail is still created:
I have a WIP PR here, the next step is to pass a shouldCrop param and test against all possible usages of this function |
I made more progress on the PR today:
Next Steps:
|
Implementation is complete:
There is a new CI issue within Web-PDFs that I need to fix. |
I'm ill today, but updated the Web-E PR based on feedback. It should be merged today 🤞 |
I've been sick. I'm back 50% now and trying to get the 2nd half of this task merged. |
I resolved the CI issue and the full PR is now in review 🎉 |
The PR received a small change request which I have made, it's now back in review. |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Version Number: 1.4.9-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause-Internal Team
Slack conversation: @
Action Performed:
Expected Result:
When adding big image, thumbnail display must be clear.
Actual Result:
When adding big image as scan receipt, thumbnail display is not clear, image distorted.
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @JulesssssThe text was updated successfully, but these errors were encountered: