Skip to content
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

fix: keep the format of imported images and remove 1MB limit #17014

Merged
merged 1 commit into from
Oct 2, 2024

Conversation

criticalAY
Copy link
Contributor

@criticalAY criticalAY commented Sep 3, 2024

Purpose / Description

Gif was compressed and renamed with jpg extension this PR fixes the issue

Fixes

How Has This Been Tested?

Tested on Google emulator API 35
image

Checklist

Please, go through these checks before submitting the PR.

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

@criticalAY

This comment was marked as outdated.

@lukstbit

This comment was marked as outdated.

@lukstbit lukstbit added the Needs Author Reply Waiting for a reply from the original author label Sep 4, 2024
@criticalAY

This comment was marked as outdated.

@lukstbit lukstbit added Needs Review and removed Needs Author Reply Waiting for a reply from the original author labels Sep 4, 2024
Copy link
Member

@BrayanDSO BrayanDSO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue also applies to PNGs, Animated PNGs, WebPs, BMPs, AVIFs, and any format that isn't a JPG file.

It was introduced in #16798 when all the files started being converted to JPGs for no explicit reason.

Why the change from just importing the file as is? This only complicates things and create issues. Files shouldn't be compressed or converted without any warn

@BrayanDSO BrayanDSO added Needs Author Reply Waiting for a reply from the original author and removed Needs Review labels Sep 4, 2024
@criticalAY
Copy link
Contributor Author

We only allow max 1 MB image size, camera images are greater than that on physical devices, so I placed it to compress the images by default, for images that are static it should not matter?

@criticalAY criticalAY removed the Needs Author Reply Waiting for a reply from the original author label Sep 6, 2024
@criticalAY
Copy link
Contributor Author

Now the compression won't take place by default but if the image is over 1MB then there will be an option to compress or crop it when user tries to save it, otherwise the original name and extension will be retained

@BrayanDSO
Copy link
Member

for images that are static it should not matter

It breaks transparency, metadata, quality and makes it a lossy image, so it matters a lot.

@BrayanDSO BrayanDSO added the Needs Author Reply Waiting for a reply from the original author label Sep 6, 2024
@criticalAY criticalAY added Strings and removed Needs Author Reply Waiting for a reply from the original author labels Sep 6, 2024
@BrayanDSO
Copy link
Member

Given #17030, you can remove the 1MB limit and the compressing option in this PR

Copy link
Member

@BrayanDSO BrayanDSO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@BrayanDSO BrayanDSO added the Needs Second Approval Has one approval, one more approval to merge label Sep 9, 2024
@BrayanDSO BrayanDSO changed the title fix: handle gif import and retain original properties of gif fix: keep the format of imported images and remove 1MB limit Sep 9, 2024
Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is too far. A feature request from long ago when camera images were only 3MB was that a user would see that an image was large and have the option to crop and/or compress it --> #5629

This PR is the equivalent of "The feature you requested is now mandatory" which is not the way to go.

We should not compress all the time, no. But we should not remove the notice to users that the image is really large and they might consider cropping/compressing it.

@mikehardy mikehardy added Needs Author Reply Waiting for a reply from the original author and removed Needs Second Approval Has one approval, one more approval to merge labels Sep 16, 2024
@mikehardy
Copy link
Member

Note that there is a 100MB per file limit, so there is a limit, even if it is quite large:

https://faqs.ankiweb.net/are-there-limits-on-file-sizes-on-ankiweb.html

At the moment there are no limits on the size of your media, although the size of individual media files is limited to 100MB

@criticalAY criticalAY added Needs Review and removed Needs Author Reply Waiting for a reply from the original author labels Sep 24, 2024
@criticalAY
Copy link
Contributor Author

@mikehardy waiting for your input

@david-allison david-allison added the Review High Priority Request for high priority review label Sep 30, 2024
Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay with this in general

I left a nitpick + constructive suggestion on the comment related to the limit as it was outdated size and didn't have a reference to upstream source

I believe all these commits should be squashed and re-pushed as a single commit, since the code involved flaps from deleted to not deleted between commits, it only made sense to review as a single commit, should only be merged as a single commit

If the comment is updated and it's a single commit, I'm good with a merge

@mikehardy mikehardy added Needs Author Reply Waiting for a reply from the original author Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) Needs Second Approval Has one approval, one more approval to merge and removed Needs Review labels Sep 30, 2024
fix: remove the 1MB limit for the images in AnkiDroid

Follow the Anki convetions for image size see https://faqs.ankiweb.net/are-there-limits-on-file-sizes-on-ankiweb.html
@criticalAY criticalAY removed the Needs Author Reply Waiting for a reply from the original author label Oct 2, 2024
Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - thanks for plowing through this one

@mikehardy mikehardy removed Review High Priority Request for high priority review Needs Second Approval Has one approval, one more approval to merge labels Oct 2, 2024
@mikehardy mikehardy added this pull request to the merge queue Oct 2, 2024
Merged via the queue into ankidroid:main with commit 56ae53d Oct 2, 2024
9 checks passed
Copy link
Contributor

github-actions bot commented Oct 2, 2024

Maintainers: Please Sync Translations to produce a commit with only the automated changes from this PR.

Read more about updating strings on the wiki,

@github-actions github-actions bot removed the Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) label Oct 2, 2024
@github-actions github-actions bot added this to the 2.19 release milestone Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants