-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: Warn users when adding media files > 100MiB (#19580) #19582
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
feat: Warn users when adding media files > 100MiB (#19580) #19582
Conversation
|
Important Maintainers: This PR contains Strings changes
|
david-allison
left a comment
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!!
As I said an hour ago
It's best if we reach a quick consensus on this one before continuing
Drafting until then
| * Files exceeding this limit cannot be synced to AnkiWeb. | ||
| * @see <a href="https://faqs.ankiweb.net/are-there-limits-on-file-sizes-on-ankiweb.html">AnkiWeb FAQ</a> | ||
| */ | ||
| const val ANKIWEB_MAX_MEDIA_FILE_SIZE = 100 * 1024 * 1024L // 100 MiB = 104,857,600 bytes |
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 will need exposing in Anki-Android-Backend
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.
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.
Should I keep the constant here or wait for #609? ?
aslo Happy to work on ankidroid/Anki-Android-Backend#609 if needed.
| } catch (e: IllegalArgumentException) { | ||
| // Check if this is a media size limit exception | ||
| val message = e.message ?: "" | ||
| if (message.startsWith("MEDIA_SIZE_LIMIT_EXCEEDED:")) { |
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.
make a different exception type if you're doing this.
| val message = e.message ?: "" | ||
| if (message.startsWith("MEDIA_SIZE_LIMIT_EXCEEDED:")) { | ||
| val parts = message.substringAfter("MEDIA_SIZE_LIMIT_EXCEEDED: ").split("|") | ||
| if (parts.size == 3) { |
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 really risky, store the data you need in an exception
| // backend text ends with dot | ||
| val successMessage = TR.importingCardsAdded(count).replace(".", "") | ||
| showSnackbar(successMessage, Snackbar.LENGTH_SHORT) | ||
| showSnackbar(successMessage) |
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.
what's the reasoning here
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.
reverting this
Purpose / Description
AnkiWeb enforces a 100MiB limit on media files. Currently, AnkiDroid allows users to add files larger than this limit, which leads to sync failures and potential OutOfMemoryErrors later on. This PR implements a warning system to alert users when they attempt to add a file exceeding this limit.
Fixes
Approach
ANKIWEB_MAX_MEDIA_FILE_SIZEconstant (100 MiB) to Media.kt and implemented a size check inMedia.addFile(). This ensures all entry points for adding media are covered.Media.addFile()throws anIllegalArgumentExceptionwith a specific message format (MEDIA_SIZE_LIMIT_EXCEEDED:...) when the limit is breached.AlertDialogwarning the user.How Has This Been Tested?
Tested manually on an Android emulator (Pixel 4 API 30) and physical device:
Checklist
Please, go through these checks before submitting the PR.