-
-
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
[GSoC'24] Enhancement of Media Drop Feature in NoteEditor #16749
Conversation
Message to maintainers, this PR contains strings changes.
Read more about updating strings on the wiki, |
This comment was marked as resolved.
This comment was marked as resolved.
Thanks! I'll say this is blocked on #16750 EDIT: Unblocked after a few hours thanks to Arthur |
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.
Looks great!
Asking for testing [unit/integration] as we'll certainly want to support extensions here, and want to guard against regressions
This would be easier to read if you performed the rename from image
to media
in a preceding commit
PS: I really appreciate you retaining my copyright header
private val IMAGE_MIME_TYPES = arrayOf("image/*") | ||
private val AUDIO_MIME_TYPES = arrayOf("audio/*") | ||
private val VIDEO_MIME_TYPES = arrayOf("video/*") |
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'd want to see a couple of unit tests around this code
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
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.
Looks really good!
One logic error (I believe) - please consider adding a test for this
Then good to go
c60c67c
to
33ae2a2
Compare
33ae2a2
to
b1a9561
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.
Only nits. Please feel free to let me know you disagree, or you want to leave change for another PR. Also, one question for @david-allison : Are you okay with your homepath bieng in the codebase?
I admit there were parts which were still hard for me to understand, but it's still clearly easier than the first PR you gave me, thanks
@@ -96,8 +96,8 @@ class FieldEditText : FixedEditText, NoteService.NoteField { | |||
setDefaultStyle() | |||
} | |||
|
|||
fun setImagePasteListener(imageListener: ImagePasteListener?) { | |||
this.imageListener = imageListener | |||
fun setPasteListener(pasteListener: PasteListener?) { |
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.
While you're in this part of the code, I'd consider (as a last followup PR, so that you don't lose David's approval) , ensure that the type of the input is not nullable. As far as I can see, we never use it with null
@@ -81,13 +81,13 @@ class MediaRegistration(private val context: Context) { | |||
Timber.d("File was %d bytes", bytesWritten) | |||
if (bytesWritten > MEDIA_MAX_SIZE) { | |||
Timber.w("File was too large: %d bytes", bytesWritten) | |||
showThemedToast(context, context.getString(R.string.note_editor_paste_too_large), false) | |||
showThemedToast(context, context.getString(R.string.note_editor_media_too_large), false) |
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.
Do you know how to reproduce this case ? I tried to paste a 15 mb image and it was resized.
If we can't reproduce it, it would be nice to delete the code.
If we can, then please see my comment on the string
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.
To reproduce try pasting video or audio
@@ -240,7 +240,7 @@ | |||
<string name="toolbar_item_explain_edit_or_remove">Enter HTML to be inserted before and after the selected text\n\nLong press a toolbar item to edit or remove it</string> | |||
<string name="remove_toolbar_item">Remove Toolbar Item?</string> | |||
|
|||
<string name="note_editor_paste_too_large">The image is too large to paste, please insert the image manually</string> | |||
<string name="note_editor_media_too_large">The media is too large, please insert the media manually</string> |
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 wonder whether "media" is very clear, as a word. I think it would be better to have "image", "video", or "audio file".
I know we can't always detect which kind of file one file is, but we can still try heuristic. And maybe keep "media" in the case where we don't know whether we have audio or video.
@@ -337,6 +337,7 @@ dependencies { | |||
implementation libs.androidx.appcompat | |||
implementation libs.androidx.browser | |||
implementation libs.androidx.core.ktx | |||
implementation libs.androidx.draganddrop |
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.
Regarding the commit message of the second commit, I'd suggest using >
on the start of the lines, so that markdown displays it as a quote
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 you need a space after > for it to work in markdown
@@ -193,8 +144,8 @@ class FieldEditText : FixedEditText, NoteService.NoteField { | |||
override fun onTextContextMenuItem(id: Int): Boolean { | |||
// This handles both CTRL+V and "Paste" |
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.
Do you think you could replace "this" by what is referred.
I assume it means "The current function is called both by ctrl+v and pasting from the context menu"
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.
Also, note if it also deal with drag and drop
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.
Also, note if it also deal with drag and drop
No
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.
Sorry, I was not clear.
I didn't mean "If it deals with drag and drop, please write it down.
I meant "Indicates whether or not it deals with drag and drop"
My understanding is that drag/drop and copy/paste use similar mechanism, so I thought it migh be a natural question, and so it would be nice to state that this is not used for drag/drop.
Honestly, given the name, I would not even have guessed it was dealing with pasting
|
||
fun getDescription(clipboard: ClipboardManager?): ClipDescription? { | ||
return clipboard | ||
?.takeIf { it.hasPrimaryClip() } |
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 don't understand why you removed the takeIf
above and put it here. I'd expect if there is no primary clip, it returns null
, in which case this line can be removed without change in behavior
val IMAGE_MIME_TYPES = arrayOf("image/*") | ||
val AUDIO_MIME_TYPES = arrayOf("audio/*") | ||
val VIDEO_MIME_TYPES = arrayOf("video/*") | ||
val MEDIA_MIME_TYPES = arrayOf(*IMAGE_MIME_TYPES, *AUDIO_MIME_TYPES, *VIDEO_MIME_TYPES) | ||
|
||
fun hasImage(clipboard: ClipboardManager?): Boolean { |
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.
Please, on all methods considering "image", add some documentation stating that it may return false on svg.
If you're certain it always return "false", then state "it returns false on svg". But given that, according to mdn, the mime type of svg should be image/svg+xml
, I assume it may returns true
on some device.
Anyway, it seems important that any caller think about svg explicitly
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 appreciate you added the comment on hasImage
below. But this should be here too.
More importantly: I realize that I assumed that it may return "false", but maybe i got it wrong. I assumed it becaume, previously, this function only returned true for gif, png and jpg. But it seems not to be the case anymore, given you changed IMAGE_MIME_TYPES
value.
I also assumed that because you had a function specifically for SVG. And so I assumed they were distinct case.
I now suspect that I may have misunderstood what you're doing.
So my question is: where you able to make this function return false on SVG?
If so, can you please document how you did it. And what was the mime type.
Otherwise, I guess that I should ask you to remove the comment, given that I was wrong
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.
where you able to make this function return false on SVG?
It returns true on SVG too
private fun getFirstItem(clipboard: ClipboardManager?) = clipboard | ||
?.takeIf { it.hasPrimaryClip() } | ||
?.primaryClip | ||
private fun getFirstItem(clipboard: ClipboardManager?) = clipboard?.primaryClip |
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.
While touching this method/object, I'd consider rewriting it as ClipboardManager?.getFirstItem() = ...
And similarly everywhere. I would find it nicer.
And similarly everywhere in the file
Note that you can do an automated rewrite with "ctrl+f6" and move a parameter as receive of the method, so it does the change for you.
Not mandatory, I won't block on it
testlib/src/main/AndroidManifest.xml
Outdated
<manifest xmlns:tools="http://schemas.android.com/tools"> | ||
<!-- | ||
overrideLibrary shouldn't be necessary as it should be handled in the AnkiDroid manifest |
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 should be handled in AnkiDroid manifest? I don't know what "it" refers to here.
In particular, what is not required? On first read, I'd expect that you mean that you don't need to override, and could just use the standard uses-feature android:name=
.
By looking more at the code, I understand you probably meant that we should not have to add anything, given that the current manifest otherwise implicitly use everything from AnkiDroid/src/main/AndroidManifest.xml
If I understand correctly, it would be rephrase as:
Testlib's manifest should implicitly imports everything from
AnkiDroid/src/main/AndroidManifest.xml
, except the imports done withoverrideLibrary
it seems. So we repeat the overriden imports below. Otherwise, here is the error message we receive:
330cf7f
to
2411415
Compare
In preparation for 'Drag & Drop' work, which will allow importing of media
2411415
to
675bc90
Compare
@@ -240,7 +240,8 @@ | |||
<string name="toolbar_item_explain_edit_or_remove">Enter HTML to be inserted before and after the selected text\n\nLong press a toolbar item to edit or remove it</string> | |||
<string name="remove_toolbar_item">Remove Toolbar Item?</string> | |||
|
|||
<string name="note_editor_paste_too_large">The image is too large to paste, please insert the image manually</string> | |||
<string name="note_editor_media_too_large">The %1$s is too large, please insert the %2$s manually</string> |
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.
Nope.
I understand what you tried to do here, but we'll go with just multiple full sentences.
Firstly, if we were to do that, then you'd really need to add a comment explaining what kind of value you expect those variables to be for the translator.
In French, which is my mother tongue, every noun is gendered. An image is femine while a sound is masculine. Don't ask why. Adjectives are gendered.
So we'd need to be able to do either:
L'image est trop grande
or
Le son est trop grand
Both the translation of "the" would change (in this case "l'" is similar to the English "a", while "le" would be similar to "an". It depends on whether there is a vowel or not as the next letter), and of "large" would need to change.
So this would not be translatable.
Plus, we would have no guarantee that the translator translating note_editor_media_too_large would also be the one translating "image".
I assume that, on the long term, the goal is to be able to have "audio" and "video" instead of "image" as a parameter.
If so, I'm not even fan of this sentence. I don't know whether all users would know what it means for a sound to be too large. An image, you can more or less see the size of the image as a physical object, as a picture or a drawing. I think it'd be clearer if we state that "the audio file is too large"
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'd be clearer if we state that "the audio file is too large"
@Arthur-Milchior Similarly for video as "the audio file is too large"?
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.
Do we need the string parameter?
The file is too large to paste. Please insert it manually
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.
@david-allison whcih string parameter are you referring to ?
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.
- In the original we had 1 string with a parameter (bad)
- In the update, we have 3 strings:
<string name="note_editor_image_too_large">The image is too large, please insert the image manually</string>
<string name="note_editor_video_too_large">The video file is too large, please insert the video manually</string>
<string name="note_editor_audio_too_large">The audio file is too large, please insert the audio manually</string>
I don't see why we need 3 strings if the context is obvious to the user. In all cases it's a file which is too large to paste.
- This is a shorter string and more comprehensible
- This is less work for our translators
} | ||
|
||
@Test | ||
fun hasMediaWithImageMimeTypeTest() { |
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.
Would you mind doing the same with svg please?
private fun getFirstItem(clipboard: ClipboardManager?) = clipboard | ||
?.takeIf { it.hasPrimaryClip() } | ||
?.primaryClip | ||
private fun ClipboardManager?.getFirstItem() = this?.primaryClip |
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 mentioned "similarly, everywhere in the file".
I prefer consistency.
Either you agree that having the clipboard manager/description has a receiver seems nice, and you put all such objects as receiver.
Or you think it's nicer to have them as argument. In this case, don't change getFirstItem
In any case, having only one changed makes no sense to me
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.
Also, in this case, I'd consider doing ClipboardManager.getFirstItem() = primaryClip?.takeIf { it.itemCount > 0 } ?.getItemAt(0)
And then any place the manager could be null, you'd call this function with manager?.getFirstItem()
.
Personally, I find it slightly ugly when you call a method with null as a receiver.
@@ -337,6 +337,7 @@ dependencies { | |||
implementation libs.androidx.appcompat | |||
implementation libs.androidx.browser | |||
implementation libs.androidx.core.ktx | |||
implementation libs.androidx.draganddrop |
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 you need a space after > for it to work in markdown
val IMAGE_MIME_TYPES = arrayOf("image/*") | ||
val AUDIO_MIME_TYPES = arrayOf("audio/*") | ||
val VIDEO_MIME_TYPES = arrayOf("video/*") | ||
val MEDIA_MIME_TYPES = arrayOf(*IMAGE_MIME_TYPES, *AUDIO_MIME_TYPES, *VIDEO_MIME_TYPES) | ||
|
||
fun hasImage(clipboard: ClipboardManager?): Boolean { |
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 appreciate you added the comment on hasImage
below. But this should be here too.
More importantly: I realize that I assumed that it may return "false", but maybe i got it wrong. I assumed it becaume, previously, this function only returned true for gif, png and jpg. But it seems not to be the case anymore, given you changed IMAGE_MIME_TYPES
value.
I also assumed that because you had a function specifically for SVG. And so I assumed they were distinct case.
I now suspect that I may have misunderstood what you're doing.
So my question is: where you able to make this function return false on SVG?
If so, can you please document how you did it. And what was the mime type.
Otherwise, I guess that I should ask you to remove the comment, given that I was wrong
@@ -32,40 +32,69 @@ import timber.log.Timber | |||
|
|||
object ClipboardUtil { | |||
// JPEG is sent via pasted content |
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 we can remove this comment. It may have been useful, but it seems strange here when we use so many media type
context.getString(R.string.image) | ||
) | ||
} else { | ||
context.getString( |
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'd consider having one sentence for video and one for audio if you can
openInputStreamWithURI(uri).use { copyFd -> | ||
// no conversion to jpg in cases of gif and jpg and if png image with alpha channel | ||
if (shouldConvertToJPG(fileNameAndExtension.value, copyFd)) { | ||
if (!isSVG && isImage && shouldConvertToJPG(fileNameAndExtension.value, copyFd)) { |
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 feel like shouldConvertToJPG
should handle the case of svg and checking that the media is an image.
Maybe it means giving it more arguments, such as the description. But that would allow us to simplify this part of the code.
675bc90
to
b88b703
Compare
@Arthur-Milchior still not |
This commit ensures that we can drop files (photos, videos, and audio) in NoteEditor (FixedEditText). Fix: Handle minSdkVersion conflict with DropHelper and enable file drop in NoteEditor Added DropHelperCompat to handle the incompatibility issue with DropHelper and minSdkVersion 23. The manifest merger failed with the following error: > Manifest merger failed: uses-sdk:minSdkVersion 23 cannot be smaller than version 24 declared in library [androidx.draganddrop:draganddrop:1.0.0] /Users/davidallison/.gradle/cache> s/8.8/transforms/0a1688833d368c1b9b07d2054911030e/transformed/draganddrop-1.0.0/AndroidManifest.xml as the library might be using APIs not available in 23 > Suggestion: use a compatible library with a minSdk of at most 23, > or increase this project's minSdk version to at least 24, > or use tools:overrideLibrary="androidx.draganddrop" to force usage > (may lead to runtime failures) To resolve this, the DropHelperCompat class is used to conditionally configure the view for drag and drop operations only when the SDK version is 24 or higher.
b88b703
to
33498e8
Compare
openInputStreamWithURI(uri).use { copyFd -> | ||
// no conversion to jpg in cases of gif and jpg and if png image with alpha channel | ||
if (shouldConvertToJPG(fileNameAndExtension.value, copyFd)) { | ||
if (shouldConvertToJPG(fileNameAndExtension.value, copyFd, isImage)) { |
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'd have considered to send the description
directly to the function. So that you could use the mime type to check for SVG.
@@ -112,7 +129,13 @@ class MediaRegistration(private val context: Context) { | |||
return true // successful conversion to jpg. | |||
} | |||
|
|||
private fun shouldConvertToJPG(fileNameExtension: String, fileStream: InputStream): Boolean { | |||
private fun shouldConvertToJPG(fileNameExtension: String, fileStream: InputStream, isImage: Boolean): Boolean { | |||
if (!isImage) { |
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.
While you are touching this file, would you mind:
- setting fileNameExtension to lowercase
- add jpeg, which is also a valid for jpg
- maybe simplify to:
return when (fileNameExtension.lowercase()) {
".svg", ".jpg", ".jpeg", ".gif" -> false
".png" -> !doesInputStreamContainTransparency(fileStream)
else -> true
}
Very small nit. If you're willing to address them in a different PR it's great, otherwise, no worries. |
Maintainers: Please Sync Translations to produce a commit with only the automated changes from this PR. Read more about updating strings on the wiki, |
Purpose / Description
This PR enhances the existing functionality of dropping media files in the NoteEditor (FixedEditText). Previously, only photos could be dropped. With this enhancement, users can now also drop video and audio files into the FixedEditText.
Approach
To implement this feature, I utilized the
DropHelper
class instead of the traditional drag-and-drop mechanisms.Why DropHelper?
Simplified Implementation: DropHelper provides a straightforward and high-level API for managing drop actions, reducing the complexity of handling multiple types of media.
How Has This Been Tested?
HP Chromebook
Screen recording 2024-07-17 10.30.17 PM.webm
Learning (optional, can help others)
https://medium.com/androiddevelopers/simplifying-drag-and-drop-3713d6ef526e
https://developer.android.com/reference/androidx/draganddrop/DropHelper
Checklist
Please, go through these checks before submitting the PR.