Closed mikehardy closed 2 years ago
Video is supported, but via "add audio clip" - we should fix this with an additional icon and split out the functionality.
We should also test our video player. It's one of the few aspects of the code that I haven't touched (to be honest, cards with video seem like a niche feature - you typically only want a very short "clip" rather than a video, and it would lead to larger storage on AnkiWeb)
Note: Anki natively supports video via the [sound:]
tag, which is why it was considered to be an audio clip
2.16 has an advanced preference to allow any file imports in "add audio clip": https://github.com/ankidroid/Anki-Android/pull/9643
Proposed implementation: Initially we should add a "Add video clip" to the Note Editor menu, from here, we can later split out the functionality between video and audio. @mikehardy I feel this is a "good first issue" if you're happy with this initial change
Sounds good yes
Hey @mikehardy I would like to work on this issue.
Sounds good to me!
@david-allison-1 Hey! I see that this issue has not been updated in a while. I'm new to the community and would love to contribute. Is this issue still being worked on by others?
@david-allison-1 Hey! I see that this issue has not been updated in a while. I'm new to the community and would love to contribute. Is this issue still being worked on by others?
@bishalbar77 are you still working on this?
Otherwise, feel free to take this on if there's no reply in a couple of days. Ping me and I'll assign it
Hey @david-allison-1 ! Since there is no reply, I assume no work is being done on it currently, and would like to be assigned!
Firstly, I was successfully able to reproduce this issue(?) on my device, and can attach a video through the Add Audio Clip
selection.
I'm new to AnkiDroid's development but did take my time to poke around the code in Android Studio, and I think I have found the starting point of what I'm supposed to do (do correct me if i'm wrong :p).
In src/main/java/ichi2/anki/multimediacard/fields
I found two things of interest:
AudioClipField.kt
file (which i assume, is the underlying code behind Add Audio Clip
)EFieldType.kt
file, which has the enum classFrom what I got from the discussion, and my own (limited) understanding, what I would have to do is:
EFieldType.kt
for VIDEO_CLIP
AudioClipField.kt
code into another file VideoClipField.kt
and make the necessary changes (Audio --> Video). Let me know if there are any gaps in my understanding of the issue, I would love to get any amount of help and feedback I can get on my first issue! Thank you!
@viciousAegis that's really thorough and I'm impressed. I'm not at my computer right now, but that all sounds correct.
I'm more than happy for you to start. I'll be in Discord if you need anything, and it's expected that a PR will have a few comments for review.
Regarding the UI, there should be an XML layout file. You'll want an icon, and can get that from Android studio: right click - add - vector icon.
The development guide has a section on adding strings. Only modify the root ".XML", and not any of the language-specific files.
Be bold and submit a PR, or add comments here with questions. We'll walk you through getting things submitted.
If you're still having trouble with the UI, give me or one of the team a ping here or in Discord
@david-allison-1
(might be very long, apologies :p; any help appreciated!)
I started by creating a new entry in src/main/java/com/ichi2/anki/multimediacard/fields/EFieldType.kt
for video clips:
enum class EFieldType {
TEXT, // Just text
IMAGE, // Just image
AUDIO_RECORDING, // Just audio
AUDIO_CLIP, // Just audio clip
VIDEO_CLIP // Just video clip (part of issue #10212)
}
After this, I went to src/main/java/com/ichi2/anki/multimediacard/fields/BasicControllerFactory.kt
and copied the code for AUDIO_CLIP
for VIDEO_CLIP
(as functionality is same for now):
class BasicControllerFactory private constructor() : IControllerFactory {
override fun createControllerForField(field: IField): IFieldController? {
return when (field.type) {
EFieldType.TEXT -> BasicTextFieldController()
EFieldType.IMAGE -> BasicImageFieldController()
EFieldType.AUDIO_RECORDING -> BasicAudioRecordingFieldController()
EFieldType.AUDIO_CLIP -> BasicAudioClipFieldController()
EFieldType.VIDEO_CLIP -> BasicAudioClipFieldController() /* created for issue #10212: explicit video clips */
else -> null
}
}
Next, in the src/main/java/com/ichi2/anki/multimediacard/fields
folder, I duplicated AudioClipField.kt
to create VideoClipField.kt
which has the same code as the former, bar the getType() function, which was changed as follows:
/**
* Implementation of Video Clip field type (issue #10212)
*/
class VideoClipField : AudioField() {
override fun getType(): EFieldType {
return EFieldType.VIDEO_CLIP
}
I have my first doubt here: What do i do about serialVersionUID
in the following?
/**
* Ask about the companion object
*/
companion object {
private const val serialVersionUID = 2937641017832762987L
}
This UID is of the AudioClipField.kt
file, so I assume it should be different, but I'm not sure what to do about it.
For the UI part,
I started off by creating a new vector icon for the video clip option in the menu, for which i created src/main/res/drawable/ic_popup_menu_item_video.xml
The Icon:
Then, I went to src/main/res/values/16-multimedia-editor.xml
and added a string for video clip
following the same convention used for audio clip
:
<resources>
<!-- Main editor popup menu items -->
<string name="multimedia_editor_popup_image">Add image</string>
<string name="multimedia_editor_popup_audio_clip">Add audio clip</string>
<string name="multimedia_editor_popup_video_clip">Add video clip</string> <!--issue #10212 (explicitly adding video clips)-->
<string name="multimedia_editor_popup_audio">Record audio</string>
<string name="multimedia_editor_popup_text">Advanced editor</string>
<string name="multimedia_editor_popup_cloze">Cloze deletion</string>
<string name="multimedia_editor_popup_clear_field">Clear field</string>
Now, the following changes crashed the Anki app in the simulator as soon as it tried to open
Then I went to src/main/res/menu/popupmenu_multimedia_options.xml
file, which contained the popup menu, and made the following changes:
<!-- issue #10212: creating "add video clip" item -->
<item
android:id="@+id/menu_multimedia_video_clip"
android:icon="@drawable/ic_popup_menu_item_video"
android:title="@string/multimedia_editor_popup_video_clip" />
Design:
Finally, I went to src/main/java/com/ichi2/anki/NoteEditor.java
and in the function private void setMMButtonListener(ImageButton mediaButton, final int index)
added an else if()
statement for video clip
private void setMMButtonListener(ImageButton mediaButton, final int index) {
mediaButton.setOnClickListener(v -> {
Timber.i("NoteEditor:: Multimedia button pressed for field %d", index);
if (mEditorNote.items()[index][1].length() > 0) {
final Collection col = CollectionHelper.getInstance().getCol(NoteEditor.this);
// If the field already exists then we start the field editor, which figures out the type
// automatically
IMultimediaEditableNote note = getCurrentMultimediaEditableNote(col);
startMultimediaFieldEditor(index, note);
} else {
// Otherwise we make a popup menu allowing the user to choose between audio/image/text field
// TODO: Update the icons for dark material theme, then can set 3rd argument to true
PopupMenuWithIcons popup = new PopupMenuWithIcons(NoteEditor.this, v, true);
MenuInflater inflater = popup.getMenuInflater();
inflater.inflate(R.menu.popupmenu_multimedia_options, popup.getMenu());
popup.setOnMenuItemClickListener(item -> {
int itemId = item.getItemId();
if (itemId == R.id.menu_multimedia_audio) {
Timber.i("NoteEditor:: Record audio button pressed");
startMultimediaFieldEditorForField(index, new AudioRecordingField());
return true;
} else if (itemId == R.id.menu_multimedia_audio_clip) {
Timber.i("NoteEditor:: Add audio clip button pressed");
startMultimediaFieldEditorForField(index, new AudioClipField());
return true;
} else if (itemId == R.id.menu_multimedia_video_clip) {
Timber.i("NoteEditor:: Add video clip button pressed");
startMultimediaFieldEditorForField(index, new VideoClipField()); /* Done as Part of issue 10212: add explicit video file*/
return true;
@viciousAegis
serialVersionUID
Remove it, I don't believe we need to bother with this as we don't serialize the exception.
A "crash" should be visible in the "logcat" window.
You'll need to provide one of:
if you need further help.
I'd advise that you run the relevant unit tests to see if the problem is picked up there, that reduces the cycle time to determine what the problem is
I'd highly advise pushing to a branch on your repo if you want to provide large amounts of sample code, it saves time for you, and makes it much easier for me to pull the diff and check it out
2) Yes, same screen
@viciousAegis
serialVersionUID
Remove it, I don't believe we need to bother with this as we don't serialize the exception.
A "crash" should be visible in the "logcat" window.
You'll need to provide one of:
push to a branch in your repo that I can pull (recommended)
a stack trace
a patch file
if you need further help.
I'd advise that you run the relevant unit tests to see if the problem is picked up there, that reduces the cycle time to determine what the problem is
I'd highly advise pushing to a branch on your repo if you want to provide large amounts of sample code, it saves time for you, and makes it much easier for me to pull the diff and check it out
2) Yes, same screen
Okay, will do all of this within a few days and update again. Thanks for the feedback and guidance!
@david-allison-1 created a PR with appropriate documentation. Ready for review!
Proposed implementation: We should add a "Add video clip" to the Note Editor "attach" menu, which does the same thing as
add audio clip
from here, we can later split out the functionality between video and audio.Edit by @david-allison-1
This came in from a play store review comment / interaction with a user
It appears that you can only attach images in the note editor.
Methods tried:
I think this is possible with Anki Desktop (and likely Anki Mobile?) as such I believe this is an anki ecosystem compatibility issue
I suppose using the advanced editor to relax the mime type filter to either images or videos would be acceptable? If I understand this correctly and it has never been possible, and someone is only asking now, then a couple more taps to use it should be okay, and it avoids the need to re-translate the attach image string to be attach image or video etc