Skip to content

Warn user when adding media files exceeding the AnkiWeb size limit #20193

Open
ujjol1234 wants to merge 8 commits intoankidroid:mainfrom
ujjol1234:feature/large-media-file-warning
Open

Warn user when adding media files exceeding the AnkiWeb size limit #20193
ujjol1234 wants to merge 8 commits intoankidroid:mainfrom
ujjol1234:feature/large-media-file-warning

Conversation

@ujjol1234
Copy link
Contributor

Purpose / Description

This PR implements a warning dialog when a user attempts to add a media file that exceeds the AnkiWeb synchronization limit (100MB).

Fixes

  • 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

@welcome
Copy link

welcome bot commented Jan 23, 2026

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.

@github-actions
Copy link
Contributor

Important

Maintainers: This PR contains Strings changes

  1. Sync Translations before merging this PR and wait for the action to complete
  2. Review and merge the auto-generated PR in order to sync all user-submitted translations
  3. Sync Translations again and merge the PR so the huge automated string changes caused by merging this PR are by themselves and easy to review

android.text.format.Formatter
.formatShortFileSize(requireContext(), fileSize)

MaterialAlertDialogBuilder(requireContext())
Copy link
Member

Choose a reason for hiding this comment

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

Use the .show extension

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@david-allison david-allison added the Needs Author Reply Waiting for a reply from the original author label Jan 23, 2026
@ujjol1234
Copy link
Contributor Author

Hi @david-allison, I have implemented your suggested changes. Could you please check them out?

@ujjol1234 ujjol1234 closed this Jan 24, 2026
@ujjol1234 ujjol1234 reopened this Jan 24, 2026
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.

Quick review on my phone, I think this is almost there. Thanks!!

@ujjol1234
Copy link
Contributor Author

Hi @david-allison, hope you are doing well!
Could you review the new changes I have made?

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.

Cheers! Sorry for the delay. Also needs a de-conflcit

}
changed = true
} catch (e: Exception) {
Timber.e(e, "Failed to add large media file after warning")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Timber.e(e, "Failed to add large media file after warning")
Timber.w(e, "Failed to add large media file after warning")

@ujjol1234 ujjol1234 force-pushed the feature/large-media-file-warning branch from 850d129 to 909effe Compare February 2, 2026 19:40
@david-allison david-allison removed Needs Author Reply Waiting for a reply from the original author Has Conflicts labels Feb 3, 2026
@ujjol1234
Copy link
Contributor Author

Hi @david-allison I have made the required changed, could you please review them?

index: Int,
field: IField,
) {
// Initial call checks size (skipSizeCheck = false)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Initial call checks size (skipSizeCheck = false)

field: IField,
skipSizeCheck: Boolean = false,
) {
val file = field.mediaFile
Copy link
Member

@david-allison david-allison Feb 5, 2026

Choose a reason for hiding this comment

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

It doesn't make sense to me that we continue execution if the file is null.

I may have missed something

index: Int,
field: IField,
) {
// Initial call checks size (skipSizeCheck = false)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Initial call checks size (skipSizeCheck = false)

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.

This is great!

I think removing the null check will massively improve typing throughout the change

Optional extension: define a value class for something like fileSize to produce a nicer looking string format

Here's something similar I wrote last night which might be of use

@JvmInline
value class Bytes(val bytes: Long) {
    fun toShortString(context: Context): String = Formatter.formatShortFileSize(context, this.bytes)
}

val Int.bytes get() = Bytes(this.toLong())

@ujjol1234
Copy link
Contributor Author

You are absolutely right @david-allison. I see now how removing the null check significantly cleaned up the types in both the exception class and the fragment. Also, that Bytes value class is super helpful. I have implemented it as you shared.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Warn users on attaching media files which breach the AnkiWeb limits (100MB+)

2 participants