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

Keep image file names #10329

Merged
merged 3 commits into from
Mar 1, 2022
Merged

Keep image file names #10329

merged 3 commits into from
Mar 1, 2022

Conversation

BrayanDSO
Copy link
Member

@BrayanDSO BrayanDSO commented Feb 11, 2022

Purpose / Description

Keep the selected images original file names or appends its checksum in it if not unique, like Anki desktop does.

Fixes

Fixes #9116

Approach

  • Most of the descriptions is here.
  • To update the <img> tag I just put the field media import before text formatting, so it's possible to update the ImagePath at importMediaToDirectory() if changed.
  • Stopped using saveMedia() because it doesn't make sense to loop all fields just to save the edited one.

How Has This Been Tested?

Couldn't run the automated tests or build new ones because of #8482

Tested it manually with Android Studio Emulator (API 32, Pixel 2) and on my android phone (Samsung Galaxy Note 10 Lite SM-N770F/DS)

Procedure:

  1. Attach an image
  2. Check if file name was kept
  3. Attach other image with the same file name
  4. Check if checksum was appended to its name
  5. Attach the first image again
  6. Check if file name was kept (checksum shouldn't be appended when reading it)

Checklist

Please, go through these checks before submitting the PR.

  • You have not changed whitespace unnecessarily (it makes diffs hard to read)
  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • Your code follows the style of the project (e.g. never omit braces in if statements)
  • 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

Personal considerations

  1. As I've already said on my approach, by changing internalizeUri(), drawings file names now keep their original file names too. Since going from imgRandomNumberSequence.extension to unixtimestamp.extension doesn't matter much IMO, I kept it to keep things simple

@welcome
Copy link

welcome bot commented Feb 11, 2022

First PR! 🚀 We sincerely appreciate that you have taken the time to propose a change to AnkiDroid! Please have patience with us as we are all volunteers - we will get to this as soon as possible.

@BrayanDSO
Copy link
Member Author

A bigger issue this solves is don't making copies when someone reattachs the same image later in another note with the attach button. So, less storage and performance is wasted with time with copies of the same files

Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

Since it's a change to libAnki, requesting a couple of tests


One of our "big" problems which the filename solved was Google Images: It download/replaces the same filename on phones in many cases, and users were getting frustrated that their images were being replaced/not uploaded as Anki assumed same filename == same content.

Your comment on "reattaches the same image" flags a concern, as we may be re-introducing this bug.

@BrayanDSO
Copy link
Member Author

BrayanDSO commented Feb 14, 2022

Since it's a change to libAnki, requesting a couple of tests

Made one, let me know if more is needed.

I fixed another one related to libanki. Kept it simple because of the likely chance to break when updating to Rust backend

One of our "big" problems which the filename solved was Google Images: It download/replaces the same filename on phones in many cases, and users were getting frustrated that their images were being replaced/not uploaded as Anki assumed same filename == same content.

Your comment on "reattaches the same image" flags a concern, as we may be re-introducing this bug.

I don't know if I understood you correctly. This is I got:

  • People commonly use google images to download images and it's easy to get files with the same name (e.g. download.jpg)
  • The random image names added by Ankidroid were useful to avoid not importing files because they have the same name, since same filename == same content, as you said.

If this is what you meant, this now isn't a problem.
If someone try to attach a different file with the same filename, it will add a -checksum suffix to it, like upstream does.

Tecnically, before my PR, Ankidroid libanki was already checking if the imported image had another file with the same name and checksum. If it was a different file, it was renamed and imported. I only updated the naming pattern on it to follow upstream (file (index).extension to file-checksum). But due to a lack of code communication after importing the file, the <img> tags were not updated, showing the first image with that name.

TL;DR
Neither Ankidroid, nor Anki desktop treated same filename == same content, it was just a lack of tag updating after importing file.

@BrayanDSO
Copy link
Member Author

A demo:

Screen_Recording_20220217-071849_AnkiDroid.mp4

@BrayanDSO
Copy link
Member Author

Audios have this problem of making copies when reattaching the same file. It should behave like Anki desktop does:

  • Keep the original filename
  • Check its checksum when another file has the same name to see if they are equal. If not, append its checksum
Screen_Recording_20220228-190702_Files.mp4

Anki desktop behavior:
(reuploaded the same audio 3 times and it does not make any unnecessary copies. Tried with a different audio and its checksum was added)
2022_02_28-19_08_01

If this should be fixed in this PR or in another one, let me know

Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

I like this and it's a great improvement. Thanks!

All my comments are nitpicks.

@david-allison david-allison added Needs Second Approval Has one approval, one more approval to merge and removed Needs Review labels Mar 1, 2022
Append checksum instead in filename of index as Anki Desktop does
Do not create temp names
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.

Nice! Looks like a solid test by my read as well, this will be a nice user improvement, quite a few complaints came in about current behavior

@mikehardy mikehardy added Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) and removed Needs Second Approval Has one approval, one more approval to merge labels Mar 1, 2022
@mikehardy mikehardy added this to the 2.16 release milestone Mar 1, 2022
@mikehardy mikehardy merged commit b079410 into ankidroid:master Mar 1, 2022
@mikehardy mikehardy removed the Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) label Mar 1, 2022
@BrayanDSO BrayanDSO deleted the keepImgNames branch March 1, 2022 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Keep the original image file name when it's attached
3 participants