[GSoC'24] New Multimedia UI #16673

criticalAY · 2024-06-29T07:38:40Z

Purpose / Description

This PR sets the new multimedia UI as developer option and handles the image options - Gallery and Camera

Fixes

Partially Note Editor: Wrong field editing screen opened #13064

How Has This Been Tested?

Tested on Oneplus Nord CE

Checklist

Please, go through these checks before submitting the PR.

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

github-actions · 2024-06-29T07:38:51Z

Message to maintainers, this PR contains strings changes.

Before merging this PR, it is best to run the "Sync Translations" GitHub action, then make and merge a PR from the i18n_sync branch to get translations cleaned out.
Then merge this PR, and immediately do another translation PR so the huge change made by this PR's key changes are all by themselves.

Read more about updating strings on the wiki,

lukstbit

Had a quick look and left some notes.
Also:

versus the previous implementation(tested on the emulator), I choose gallery -> got to the gallery screen -> get stuck on it even if I click up(toolbar) or BACK. Seems to exit if I click fast enough. Does it work for you?
in the above scenario, after I come back, the UI is not responsive: clicking up in the toolbar or done doesn't work and doesn't close the screen(BACK seems to work however). I also don't see any menu items.
choosing camera doesn't work as well. I create the picture but when I want to go back I again get sent to the camera app and the app crashes as there's a leak for crop dialog:

android.view.WindowLeaked: Activity com.ichi2.anki.multimedia.MultimediaActivity has leaked window DecorView@c071bb8[MultimediaActivity] that was originally added here
        at android.view.ViewRootImpl.<init>(ViewRootImpl.java:1009)
        at android.view.ViewRootImpl.<init>(ViewRootImpl.java:997)
        at android.view.WindowManagerGlobal.addView(WindowManagerGlobal.java:397)
        at android.view.WindowManagerImpl.addView(WindowManagerImpl.java:150)
        at android.app.Dialog.show(Dialog.java:352)
        at androidx.appcompat.app.AlertDialog$Builder.show(AlertDialog.java:1008)
        at com.ichi2.anki.multimedia.MultimediaImageFragment.showCropDialog(MultimediaImageFragment.kt:451)
        at com.ichi2.anki.multimedia.MultimediaImageFragment.handleTakePictureResult(MultimediaImageFragment.kt:209)
        at com.ichi2.anki.multimedia.MultimediaImageFragment.registerLauncher$lambda$3(MultimediaImageFragment.kt:153)

AnkiDroid/src/main/java/com/ichi2/anki/multimedia/MultimediaViewModel.kt

AnkiDroid/src/main/AndroidManifest.xml

AnkiDroid/src/main/java/com/ichi2/anki/multimedia/MultimediaBottomSheet.kt

AnkiDroid/src/main/res/layout/bottomsheet_multimedia.xml

criticalAY · 2024-06-30T14:38:44Z

Had a quick look and left some notes.

@lukstbit I tested it on my own device and emulator both but I can't reproduce the started issue of the menu not respoding

AnkiDroid/src/main/java/com/ichi2/anki/NoteEditor.kt

AnkiDroid/src/main/java/com/ichi2/anki/multimedia/MultimediaActivity.kt

AnkiDroid/src/main/java/com/ichi2/anki/NoteEditor.kt

david-allison · 2024-07-02T14:13:04Z

AnkiDroid/src/main/res/values/16-multimedia-editor.xml

@@ -73,4 +73,7 @@
    <string name="activity_result_unexpected">App returned an unexpected value. You may need to use a different app</string>
    <string name="audio_recording_field_list" comment="Displays a list of the field data in the note editor while audio recording is taking place">Field Contents</string>

+    <string name="no_image_preview">No image selected. Click restart to add an image</string>


Change this to either accept 'restart' as a parameter, or reword

I have reworded the sentence, /open to discussion/

AnkiDroid/src/main/java/com/ichi2/anki/NoteEditor.kt

ShridharGoel · 2024-07-03T19:38:20Z

AnkiDroid/src/main/java/com/ichi2/anki/NoteEditor.kt

+     * @param field The `IField` object representing the media file and its details.
+     * @throws IllegalStateException if the currently edited multimedia note cannot be retrieved.
+     */
+    private fun addMediaFileToField(index: Int, field: IField) = lifecycleScope.launch {


Do you want addMediaFileToField to return a job?

Doing it as we are using withCol instead of getColUnsafe

But why does it need to return the job?

Will change it

ShridharGoel · 2024-07-03T19:39:22Z

AnkiDroid/src/main/java/com/ichi2/anki/multimedia/MultimediaBottomSheet.kt

+        return view
+    }
+
+    private fun setMultimediaAction(action: MultimediaAction) = lifecycleScope.launch {


Do you want setMultimediaAction to return a job?

AnkiDroid/src/main/java/com/ichi2/anki/multimedia/MultimediaBottomSheet.kt

ShridharGoel · 2024-07-05T07:24:50Z

AnkiDroid/src/main/java/com/ichi2/anki/NoteEditor.kt

                    val imageIntent = MultimediaImageFragment.getIntent(
                        this@NoteEditor,
-                        MultimediaEditFieldActivityExtra(index, field, note),
+                        MultimediaEditFieldActivityExtra(index, ImageField(), note),


This would lead to a new instance of ImageField. Is that what you expect?

AnkiDroid/src/main/java/com/ichi2/anki/NoteEditor.kt

david-allison

Initial impression: looks good, thanks!

AnkiDroid/src/main/java/com/ichi2/anki/NoteEditor.kt

david-allison · 2024-07-05T15:28:17Z

AnkiDroid/src/main/java/com/ichi2/anki/NoteEditor.kt

+        multimediaViewModel.multimediaAction.take(1).onEach { action ->
+            when (action) {
+                MultimediaBottomSheet.MultimediaAction.SELECT_IMAGE_FILE -> {
+                    note.setField(index, ImageField())


Are you planning on moving away from ImageField etc...? I don't think anyone in the project likes this implementation, and we can do better here

It is not directly connected to to the multimedia activity hence I will do it at last, it concerns with the data that is being sent to the noteeditor i.e. tell it what to do

AnkiDroid/src/main/java/com/ichi2/anki/NoteEditor.kt

david-allison · 2024-07-05T15:29:12Z

AnkiDroid/src/main/java/com/ichi2/anki/NoteEditor.kt

+
+        val note: MultimediaEditableNote = getCurrentMultimediaEditableNote() ?: return
+
+        multimediaViewModel.multimediaAction.take(1).onEach { action ->


multimediaViewModel.multimediaAction.take(1).onEach doesn't read well, what are you trying to do here?

It will make the selected option to be executed only once, when the user clicks it otherwise if we observe it it will recursively launch the media option after successfully adding media to the field.

AnkiDroid/src/main/java/com/ichi2/anki/NoteEditor.kt

david-allison · 2024-07-05T16:07:24Z

AnkiDroid/src/main/java/com/ichi2/anki/multimedia/MultimediaImageFragment.kt

+            ImageOptions.GALLERY -> {
+                Timber.d("Registering image result launcher")
+                pickImageLauncher =
+                    registerForActivityResult(ActivityResultContracts.StartActivityForResult()) { result ->


registerForActivityResult

This must be called unconditionally, as part of initialization path, typically as a field initializer of an Activity or Fragment.

https://developer.android.com/reference/androidx/activity/result/ActivityResultCaller#public-methods_1

Meaning it should not be placed in when?

Ideally in onCreate, make it obvious that registration for all activities is guaranteed to be called on all paths

I misspoke - just register them as variables

AnkiDroid/src/main/java/com/ichi2/anki/multimedia/MultimediaImageFragment.kt

AnkiDroid/src/main/java/com/ichi2/anki/NoteEditor.kt

AnkiDroid/src/main/java/com/ichi2/anki/multimedia/MultimediaActivity.kt

david-allison

Initial impression: LGTM

AnkiDroid/src/main/java/com/ichi2/anki/NoteEditor.kt

AnkiDroid/src/main/java/com/ichi2/anki/multimedia/MultimediaImageFragment.kt

AnkiDroid/src/main/java/com/ichi2/anki/NoteEditor.kt

AnkiDroid/src/main/java/com/ichi2/anki/multimedia/MultimediaActivity.kt

AnkiDroid/src/main/java/com/ichi2/anki/multimedia/MultimediaBottomSheet.kt

AnkiDroid/src/main/java/com/ichi2/anki/multimedia/MultimediaImageFragment.kt

david-allison · 2024-07-12T17:48:31Z

AnkiDroid/src/main/java/com/ichi2/anki/multimedia/MultimediaImageFragment.kt

+            ImageOptions.GALLERY -> {
+                Timber.d("Registering image result launcher")
+                pickImageLauncher =
+                    registerForActivityResult(ActivityResultContracts.StartActivityForResult()) { result ->


I misspoke - just register them as variables

AnkiDroid/src/main/java/com/ichi2/anki/multimedia/MultimediaImageFragment.kt

david-allison

I'm happy, I don't feel any of my comments are blocking, especially as this is behind a dev setting

This will want a rebase onto main once #16754 is in

refactor(note-editor): improve nullability #16754

david-allison · 2024-07-18T13:01:49Z

AnkiDroid/src/main/java/com/ichi2/anki/NoteEditor.kt

+     * Handles an intent containing an image from the user's gallery or the internet,
+     * specifically for creating a new card.


Maybe this wants to be renamed, I feel the description should list the outcome (opening a screen) more explicitly

AnkiDroid/src/main/java/com/ichi2/anki/NoteEditor.kt

AnkiDroid/src/main/java/com/ichi2/anki/multimedia/MultimediaFragment.kt

AnkiDroid/src/main/java/com/ichi2/anki/multimedia/MultimediaImageFragment.kt

* feat: added multimedia activity and base fragment * enhancement: setup the gallery and camera options

lukstbit · 2024-07-20T07:58:23Z

Everything(that's implemented) seems to work and I don't see the issue I mentioned on another emulator so we can merge this.

github-actions · 2024-07-20T08:14:38Z

Maintainers: Please Sync Translations to produce a commit with only the automated changes from this PR.

Read more about updating strings on the wiki,

github-actions bot added the Strings label Jun 29, 2024

criticalAY force-pushed the multimedia-ui branch 2 times, most recently from c8ca7ed to b24d818 Compare June 29, 2024 07:43

criticalAY added Needs Review GSoC Pull requests authored by a Google Summer of Code participant [Candidate/Selected], for GSoC mentors labels Jun 29, 2024

criticalAY force-pushed the multimedia-ui branch from cceb1b1 to 4509c70 Compare June 29, 2024 08:04

lukstbit reviewed Jun 30, 2024

View reviewed changes

david-allison reviewed Jul 2, 2024

View reviewed changes

lukstbit added Needs Author Reply Waiting for a reply from the original author and removed Needs Review labels Jul 3, 2024

ShridharGoel reviewed Jul 3, 2024

View reviewed changes

AnkiDroid/src/main/java/com/ichi2/anki/NoteEditor.kt Outdated Show resolved Hide resolved

ShridharGoel reviewed Jul 3, 2024

View reviewed changes

AnkiDroid/src/main/java/com/ichi2/anki/multimedia/MultimediaBottomSheet.kt Outdated Show resolved Hide resolved

ShridharGoel reviewed Jul 5, 2024

View reviewed changes

AnkiDroid/src/main/java/com/ichi2/anki/NoteEditor.kt Outdated Show resolved Hide resolved

david-allison reviewed Jul 5, 2024

View reviewed changes

david-allison reviewed Jul 8, 2024

View reviewed changes

AnkiDroid/src/main/java/com/ichi2/anki/NoteEditor.kt Outdated Show resolved Hide resolved

david-allison reviewed Jul 8, 2024

View reviewed changes

david-allison reviewed Jul 9, 2024

View reviewed changes

AnkiDroid/src/main/java/com/ichi2/anki/NoteEditor.kt Outdated Show resolved Hide resolved

AnkiDroid/src/main/java/com/ichi2/anki/multimedia/MultimediaImageFragment.kt Outdated Show resolved Hide resolved

david-allison added Needs Review and removed Needs Author Reply Waiting for a reply from the original author labels Jul 9, 2024

criticalAY force-pushed the multimedia-ui branch from 1b7697d to c4eda8b Compare July 9, 2024 20:06

criticalAY requested review from lukstbit, ShridharGoel and david-allison and removed request for lukstbit July 9, 2024 20:11

david-allison reviewed Jul 12, 2024

View reviewed changes

criticalAY force-pushed the multimedia-ui branch from d154ba9 to 3bff668 Compare July 17, 2024 15:47

criticalAY removed the Has Conflicts label Jul 17, 2024

david-allison mentioned this pull request Jul 18, 2024

refactor(note-editor): improve nullability #16754

Merged

5 tasks

david-allison approved these changes Jul 18, 2024

View reviewed changes

github-actions bot added the Has Conflicts label Jul 18, 2024

criticalAY force-pushed the multimedia-ui branch 2 times, most recently from 33bbf51 to f04fb6d Compare July 18, 2024 16:33

github-actions bot added Has Conflicts and removed Has Conflicts labels Jul 18, 2024

ShridharGoel approved these changes Jul 19, 2024

View reviewed changes

AnkiDroid/src/main/java/com/ichi2/anki/multimedia/MultimediaImageFragment.kt Show resolved Hide resolved

feat: add multimedia UI dev option

2732e63

criticalAY force-pushed the multimedia-ui branch 2 times, most recently from add8542 to 3913667 Compare July 19, 2024 17:19

criticalAY added Needs Second Approval Has one approval, one more approval to merge and removed Needs Review Has Conflicts labels Jul 19, 2024

feat: setup new multimedia bottom sheet

4a6b94a

* feat: added multimedia activity and base fragment * enhancement: setup the gallery and camera options

criticalAY force-pushed the multimedia-ui branch from 3913667 to 4a6b94a Compare July 19, 2024 17:49

david-allison added Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) and removed Needs Second Approval Has one approval, one more approval to merge labels Jul 19, 2024

lukstbit added this pull request to the merge queue Jul 20, 2024

Merged via the queue into ankidroid:main with commit bb85307 Jul 20, 2024
8 checks passed

github-actions bot removed the Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) label Jul 20, 2024

github-actions bot added this to the 2.19 release milestone Jul 20, 2024

criticalAY deleted the multimedia-ui branch July 20, 2024 08:15

lukstbit mentioned this pull request Jul 20, 2024

Updated strings from Crowdin 2024/07/20 08:20 #16767

Merged

snowtimeglass mentioned this pull request Jul 23, 2024

[GSoC'24] Enhacement: add Audio and Video multimedia options #16769

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GSoC'24] New Multimedia UI #16673

[GSoC'24] New Multimedia UI #16673

criticalAY commented Jun 29, 2024 •

edited

Loading

github-actions bot commented Jun 29, 2024

lukstbit left a comment

criticalAY commented Jun 30, 2024

david-allison Jul 2, 2024

criticalAY Jul 2, 2024

ShridharGoel Jul 3, 2024

criticalAY Jul 4, 2024

ShridharGoel Jul 4, 2024

criticalAY Jul 4, 2024

ShridharGoel Jul 3, 2024

ShridharGoel Jul 5, 2024

david-allison left a comment

david-allison Jul 5, 2024

criticalAY Jul 6, 2024

david-allison Jul 5, 2024

criticalAY Jul 6, 2024

david-allison Jul 5, 2024

criticalAY Jul 6, 2024

david-allison Jul 8, 2024

david-allison Jul 12, 2024

david-allison left a comment

david-allison Jul 12, 2024

david-allison left a comment

david-allison Jul 18, 2024

lukstbit commented Jul 20, 2024

github-actions bot commented Jul 20, 2024


		val note: MultimediaEditableNote = getCurrentMultimediaEditableNote() ?: return

		multimediaViewModel.multimediaAction.take(1).onEach { action ->

		* Handles an intent containing an image from the user's gallery or the internet,
		* specifically for creating a new card.

[GSoC'24] New Multimedia UI #16673

[GSoC'24] New Multimedia UI #16673

Conversation

criticalAY commented Jun 29, 2024 • edited Loading

Purpose / Description

Fixes

How Has This Been Tested?

Checklist

github-actions bot commented Jun 29, 2024

lukstbit left a comment

Choose a reason for hiding this comment

criticalAY commented Jun 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-allison left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-allison left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-allison left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukstbit commented Jul 20, 2024

github-actions bot commented Jul 20, 2024

criticalAY commented Jun 29, 2024 •

edited

Loading