-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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: allow pasting images as png #17877
base: main
Are you sure you want to change the base?
Conversation
Important Maintainers: This PR contains Strings changes
|
fbf67bd
to
aad3c14
Compare
aad3c14
to
7ccc171
Compare
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 harder to review than it should. There is only one commit, it has no description at all, and it does a lot of different stuff that could be split. Also, the PR's description Approach
field was removed.
I suggest reading Arthur's guide of splitting into atomic commits. Here are some stuff that is happening in the single commit that could be split:
- The new
Editing
preference category (although I'd remove it) - Moving the
paste_png_key
constant - Making
MediaRegistration an
object` - Adding the
MediaError
enum - Changing MediaRegistration docs
Probably there are more stuff to split. Also add description to your commits to justify why you are doing X and Y
I was not expecting this comment 😅, thought the changes where self explanatory, purposely kept a single commit. But thanks for it I don't want to waste reviewers time so can do it |
That's what everyone tend to think (including myself) because they were "living" their code, but most stuff aren't. |
I'll admit I scanned this but it was kind of a big blob of jumbled things so I noped out 😅 - will appreciate the split |
- Remove summary from the paste png category as Anki doesn't have it, (images are pasted with original ext) - Shift the pastePNG under editing preference
- Remove context from constructor and converted the class to Object as we don't memory leaks - Avoids tight coupling
7ccc171
to
9337c43
Compare
I have updated the approach in my PR as well as split the commits, thanks! |
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.
Not all commits can be compiled, so this should be squashed
Splitting the PR in commits helped a lot in reviewing. Thanks for that |
ec04ef1
to
8747c93
Compare
): String = | ||
when (this) { | ||
GENERIC_ERROR -> context.getString(R.string.multimedia_editor_something_wrong) | ||
CONVERSION_ERROR -> context.getString(R.string.multimedia_editor_png_paste_error, message) |
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 message be provided to the constructor, rather than the method?
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.
it would be better to pass the message as an argument only one places uses that
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.
Implementer's choice, this reads better to me
Subject: [PATCH]
---
Index: AnkiDroid/src/main/java/com/ichi2/anki/MediaRegistration.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/MediaRegistration.kt b/AnkiDroid/src/main/java/com/ichi2/anki/MediaRegistration.kt
--- a/AnkiDroid/src/main/java/com/ichi2/anki/MediaRegistration.kt (revision b30f4ca8b2d075bb38933d8f32420cc793bf5410)
+++ b/AnkiDroid/src/main/java/com/ichi2/anki/MediaRegistration.kt (date 1739070568019)
@@ -35,6 +35,8 @@
import java.io.IOException
import java.io.InputStream
+private typealias DisplayMediaError = (MediaRegistration.MediaError) -> Unit
+
/**
* Utility class for media registration and handling errors during media paste actions.
*/
@@ -42,23 +44,22 @@
/**
* Represents different types of media errors.
*/
- enum class MediaError {
- GENERIC_ERROR,
- CONVERSION_ERROR,
- IMAGE_TOO_LARGE,
- VIDEO_TO_LARGE,
- AUDIO_TOO_LARGE, ;
+ sealed class MediaError {
+ data object GenericError : MediaError()
+ class ConversionError(val message: String) : MediaError()
+ data object ImageTooLarge : MediaError()
+ data object VideoTooLarge : MediaError()
+ data object AudioTooLarge : MediaError()
fun toHumanReadableString(
context: Context,
- message: String,
): String =
when (this) {
- GENERIC_ERROR -> context.getString(R.string.multimedia_editor_something_wrong)
- CONVERSION_ERROR -> context.getString(R.string.multimedia_editor_png_paste_error, message)
- IMAGE_TOO_LARGE -> context.getString(R.string.note_editor_image_too_large)
- VIDEO_TO_LARGE -> context.getString(R.string.note_editor_video_too_large)
- AUDIO_TOO_LARGE -> context.getString(R.string.note_editor_audio_too_large)
+ is GenericError -> context.getString(R.string.multimedia_editor_something_wrong)
+ is ConversionError -> context.getString(R.string.multimedia_editor_png_paste_error, message)
+ is ImageTooLarge -> context.getString(R.string.note_editor_image_too_large)
+ is VideoTooLarge -> context.getString(R.string.note_editor_video_too_large)
+ is AudioTooLarge -> context.getString(R.string.note_editor_audio_too_large)
}
}
@@ -83,7 +84,7 @@
uri: Uri,
description: ClipDescription,
pasteAsPng: Boolean,
- showError: (type: MediaError, message: String?) -> Unit,
+ showError: DisplayMediaError,
): String? =
try {
loadMediaIntoCollection(context, uri, description, pasteAsPng, showError)
@@ -95,17 +96,17 @@
// org.chromium.chrome.browser.util.ChromeFileProvider from ProcessRecord{80125c 11262:com.ichi2.anki/u0a455}
// (pid=11262, uid=10455) that is not exported from UID 10057
Timber.w(ex, "Failed to paste media")
- showError(MediaError.GENERIC_ERROR, null)
+ showError(MediaError.GenericError)
null
} catch (ex: SecurityException) {
Timber.w(ex, "Failed to paste media")
- showError(MediaError.GENERIC_ERROR, null)
+ showError(MediaError.GenericError)
null
} catch (e: Exception) {
// NOTE: This is happy path coding which works on Android 9.
CrashReportService.sendExceptionReport("File is invalid issue:8880", "RegisterMediaForWebView:onImagePaste URI of file:$uri")
Timber.w(e, "Failed to paste media")
- showError(MediaError.GENERIC_ERROR, null)
+ showError(MediaError.GenericError)
null
}
@@ -123,14 +124,14 @@
bytesWritten: Long,
isImage: Boolean,
isVideo: Boolean,
- showError: (type: MediaError) -> Unit,
+ showError: DisplayMediaError,
): Boolean {
if (bytesWritten <= MEDIA_MAX_SIZE_BYTES) return true
when {
- isImage -> showError(MediaError.IMAGE_TOO_LARGE)
- isVideo -> showError(MediaError.VIDEO_TO_LARGE)
- else -> showError(MediaError.AUDIO_TOO_LARGE)
+ isImage -> showError(MediaError.ImageTooLarge)
+ isVideo -> showError(MediaError.VideoTooLarge)
+ else -> showError(MediaError.AudioTooLarge)
}
return false
}
@@ -146,7 +147,7 @@
uri: Uri,
description: ClipDescription,
pasteAsPng: Boolean,
- showError: (type: MediaError, message: String?) -> Unit,
+ showError: DisplayMediaError,
): String? {
val filename = getFileName(context.contentResolver, uri)
val fd = openInputStreamWithURI(context, uri)
@@ -179,10 +180,7 @@
}
Timber.d("File was %d bytes", bytesWritten)
- val checkMediaSize =
- checkMediaSize(bytesWritten, isImage, isVideo) { errorType ->
- showError(errorType, null)
- }
+ val checkMediaSize = checkMediaSize(bytesWritten, isImage, isVideo, showError)
if (!checkMediaSize) {
File(tempFilePath).delete()
@@ -204,7 +202,7 @@
private fun convertToPNG(
file: File,
- showError: (type: MediaError, message: String?) -> Unit,
+ showError: DisplayMediaError,
): Boolean {
val bm = BitmapFactory.decodeFile(file.absolutePath)
try {
@@ -215,7 +213,7 @@
} catch (e: IOException) {
Timber.w("MediaRegistration : Unable to convert file to png format")
CrashReportService.sendExceptionReport(e, "Unable to convert file to png format")
- showError(MediaError.CONVERSION_ERROR, e.message)
+ showError(MediaError.ConversionError(e.message ?: ""))
return false
}
return true
Index: AnkiDroid/src/main/java/com/ichi2/anki/NoteEditor.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/NoteEditor.kt b/AnkiDroid/src/main/java/com/ichi2/anki/NoteEditor.kt
--- a/AnkiDroid/src/main/java/com/ichi2/anki/NoteEditor.kt (revision b30f4ca8b2d075bb38933d8f32420cc793bf5410)
+++ b/AnkiDroid/src/main/java/com/ichi2/anki/NoteEditor.kt (date 1739070600643)
@@ -2025,9 +2025,7 @@
uri,
description,
pasteAsPng,
- showError = { type, message ->
- showSnackbar(type.toHumanReadableString(requireContext(), message ?: ""))
- },
+ showError = { type -> showSnackbar(type.toHumanReadableString(requireContext())) },
) ?: return false
insertStringInField(editText, mediaTag)
- presernce the original extension of the images unless pasteAsPng is true - use the new MediaError enum class for errors - remove shouldConvertToJPG method as we don't want this check, new pasteAsPng parameter handles it
8747c93
to
ac56aba
Compare
- We get the Boolean via PASTE_IMAGES_AS_PNG from collection to see if we want to preserve the image extension or not and proceed to paste the image
ac56aba
to
b30f4ca
Compare
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.
Deeper review, a couple of bugs in the 'load into collection' method
): String? { | ||
val filename = getFileName(context.contentResolver, uri) | ||
val fd = openInputStreamWithURI(uri) | ||
val fd = openInputStreamWithURI(context, uri) |
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.
I worry this isn't closed
): String = | ||
when (this) { | ||
GENERIC_ERROR -> context.getString(R.string.multimedia_editor_something_wrong) | ||
CONVERSION_ERROR -> context.getString(R.string.multimedia_editor_png_paste_error, message) |
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.
Implementer's choice, this reads better to me
Subject: [PATCH]
---
Index: AnkiDroid/src/main/java/com/ichi2/anki/MediaRegistration.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/MediaRegistration.kt b/AnkiDroid/src/main/java/com/ichi2/anki/MediaRegistration.kt
--- a/AnkiDroid/src/main/java/com/ichi2/anki/MediaRegistration.kt (revision b30f4ca8b2d075bb38933d8f32420cc793bf5410)
+++ b/AnkiDroid/src/main/java/com/ichi2/anki/MediaRegistration.kt (date 1739070568019)
@@ -35,6 +35,8 @@
import java.io.IOException
import java.io.InputStream
+private typealias DisplayMediaError = (MediaRegistration.MediaError) -> Unit
+
/**
* Utility class for media registration and handling errors during media paste actions.
*/
@@ -42,23 +44,22 @@
/**
* Represents different types of media errors.
*/
- enum class MediaError {
- GENERIC_ERROR,
- CONVERSION_ERROR,
- IMAGE_TOO_LARGE,
- VIDEO_TO_LARGE,
- AUDIO_TOO_LARGE, ;
+ sealed class MediaError {
+ data object GenericError : MediaError()
+ class ConversionError(val message: String) : MediaError()
+ data object ImageTooLarge : MediaError()
+ data object VideoTooLarge : MediaError()
+ data object AudioTooLarge : MediaError()
fun toHumanReadableString(
context: Context,
- message: String,
): String =
when (this) {
- GENERIC_ERROR -> context.getString(R.string.multimedia_editor_something_wrong)
- CONVERSION_ERROR -> context.getString(R.string.multimedia_editor_png_paste_error, message)
- IMAGE_TOO_LARGE -> context.getString(R.string.note_editor_image_too_large)
- VIDEO_TO_LARGE -> context.getString(R.string.note_editor_video_too_large)
- AUDIO_TOO_LARGE -> context.getString(R.string.note_editor_audio_too_large)
+ is GenericError -> context.getString(R.string.multimedia_editor_something_wrong)
+ is ConversionError -> context.getString(R.string.multimedia_editor_png_paste_error, message)
+ is ImageTooLarge -> context.getString(R.string.note_editor_image_too_large)
+ is VideoTooLarge -> context.getString(R.string.note_editor_video_too_large)
+ is AudioTooLarge -> context.getString(R.string.note_editor_audio_too_large)
}
}
@@ -83,7 +84,7 @@
uri: Uri,
description: ClipDescription,
pasteAsPng: Boolean,
- showError: (type: MediaError, message: String?) -> Unit,
+ showError: DisplayMediaError,
): String? =
try {
loadMediaIntoCollection(context, uri, description, pasteAsPng, showError)
@@ -95,17 +96,17 @@
// org.chromium.chrome.browser.util.ChromeFileProvider from ProcessRecord{80125c 11262:com.ichi2.anki/u0a455}
// (pid=11262, uid=10455) that is not exported from UID 10057
Timber.w(ex, "Failed to paste media")
- showError(MediaError.GENERIC_ERROR, null)
+ showError(MediaError.GenericError)
null
} catch (ex: SecurityException) {
Timber.w(ex, "Failed to paste media")
- showError(MediaError.GENERIC_ERROR, null)
+ showError(MediaError.GenericError)
null
} catch (e: Exception) {
// NOTE: This is happy path coding which works on Android 9.
CrashReportService.sendExceptionReport("File is invalid issue:8880", "RegisterMediaForWebView:onImagePaste URI of file:$uri")
Timber.w(e, "Failed to paste media")
- showError(MediaError.GENERIC_ERROR, null)
+ showError(MediaError.GenericError)
null
}
@@ -123,14 +124,14 @@
bytesWritten: Long,
isImage: Boolean,
isVideo: Boolean,
- showError: (type: MediaError) -> Unit,
+ showError: DisplayMediaError,
): Boolean {
if (bytesWritten <= MEDIA_MAX_SIZE_BYTES) return true
when {
- isImage -> showError(MediaError.IMAGE_TOO_LARGE)
- isVideo -> showError(MediaError.VIDEO_TO_LARGE)
- else -> showError(MediaError.AUDIO_TOO_LARGE)
+ isImage -> showError(MediaError.ImageTooLarge)
+ isVideo -> showError(MediaError.VideoTooLarge)
+ else -> showError(MediaError.AudioTooLarge)
}
return false
}
@@ -146,7 +147,7 @@
uri: Uri,
description: ClipDescription,
pasteAsPng: Boolean,
- showError: (type: MediaError, message: String?) -> Unit,
+ showError: DisplayMediaError,
): String? {
val filename = getFileName(context.contentResolver, uri)
val fd = openInputStreamWithURI(context, uri)
@@ -179,10 +180,7 @@
}
Timber.d("File was %d bytes", bytesWritten)
- val checkMediaSize =
- checkMediaSize(bytesWritten, isImage, isVideo) { errorType ->
- showError(errorType, null)
- }
+ val checkMediaSize = checkMediaSize(bytesWritten, isImage, isVideo, showError)
if (!checkMediaSize) {
File(tempFilePath).delete()
@@ -204,7 +202,7 @@
private fun convertToPNG(
file: File,
- showError: (type: MediaError, message: String?) -> Unit,
+ showError: DisplayMediaError,
): Boolean {
val bm = BitmapFactory.decodeFile(file.absolutePath)
try {
@@ -215,7 +213,7 @@
} catch (e: IOException) {
Timber.w("MediaRegistration : Unable to convert file to png format")
CrashReportService.sendExceptionReport(e, "Unable to convert file to png format")
- showError(MediaError.CONVERSION_ERROR, e.message)
+ showError(MediaError.ConversionError(e.message ?: ""))
return false
}
return true
Index: AnkiDroid/src/main/java/com/ichi2/anki/NoteEditor.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/NoteEditor.kt b/AnkiDroid/src/main/java/com/ichi2/anki/NoteEditor.kt
--- a/AnkiDroid/src/main/java/com/ichi2/anki/NoteEditor.kt (revision b30f4ca8b2d075bb38933d8f32420cc793bf5410)
+++ b/AnkiDroid/src/main/java/com/ichi2/anki/NoteEditor.kt (date 1739070600643)
@@ -2025,9 +2025,7 @@
uri,
description,
pasteAsPng,
- showError = { type, message ->
- showSnackbar(type.toHumanReadableString(requireContext(), message ?: ""))
- },
+ showError = { type -> showSnackbar(type.toHumanReadableString(requireContext())) },
) ?: return false
insertStringInField(editText, mediaTag)
showError(errorType, null) | ||
} | ||
|
||
if (!checkMediaSize) { |
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.
registerMediaForWebView
, the file is already in the collection, and this doesn't delete it
@@ -54,11 +142,14 @@ class MediaRegistration( | |||
*/ | |||
@Throws(IOException::class) | |||
fun loadMediaIntoCollection( |
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.
I think this is a candidate for some refactoring, given the bugs below:
- remove the Android dependencies (
Context
/Uri
/ClipDescription
), and handle them outside the method loadMediaIntoCollection
should accept a validatedMediaFile
- It should add it into the collection, where
registerMediaForWebView
is inlined - It should return a
Field
representing the
bytesWritten = CompatHelper.compat.copyFile(fd, clipCopy.absolutePath) | ||
// return null if jpg conversion false. | ||
if (!convertToJPG(clipCopy)) { | ||
if (!convertToPNG(clipCopy, showError)) { |
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.
bytesWritten
is used below to validate the size, but this measures the copied file, not the converted file
Purpose / Description
Anki preserves the image format unless
Paste as png
is turned on, this PR allow the user to preserve the image format unless the option is turned onFixes
How Has This Been Tested?
Tested on API 35 -> Google Emulator
Approach
Object
onPaste
ran a check to see if we want to convert a image to JPG or not, but Anki doesn't do that and preserves the image extension and in case paste as PNG is turned on then pastes the images as png, hence I updated the code to remove these JPG checks and proceed with original extension unless the paste as PNG is turned oncol
to see if we want to paste the image as PNG in NoteEditorChecklist
Please, go through these checks before submitting the PR.