Integrate image occlusion editor by abdnh · Pull Request #14884 · ankidroid/Anki-Android

abdnh · 2023-12-06T14:18:34Z

Purpose / Description

This integrates the image occlusion editor with NoteEditor.

Fixes

Fixes #6980

Approach

The IO editor is accessed from the normal editor via a button.

How Has This Been Tested?

Manual testing in the emulator. Android 14.0, API level 34

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

Issues

Clipboard support
Clicking the save button in the normal button in IO edit mode triggers NullPointerException
IO editor should be closed when Add/Update button is clicked. The normal editor should probably also be closed in edit mode
Add the "Reset Image Occlusion" button
No way to edit the Comments field since it's not exposed in the IO editor. @dae is that because it's not a protected field?
The "x cards added" string counts text labels and other non-mask shapes - looks like an upstream issue

github-actions · 2023-12-06T14:18:49Z

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,

abdnh · 2023-12-06T14:21:02Z

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

            else -> {}
        }

+        col.backend.addImageOcclusionNotetype()


I assume it's fine not to call this in a background task. The computer version doesn't bother using background task for this.

Technically, we probably should be using a background task for the desktop as well. If the user has a large number of notetypes + slow disk, the operation to fetch all the notetypes could potentially block for some time.

For methods that access the collection, standard practice is to write a little wrapper function in the Collection class and call that inside withCol, instead of using the backend directly. We only access the backend directly for operations that don't require/want the collection lock (such as checking progress). One problem this avoids is the collection is guaranteed not to be closed in the middle of an operation running inside withCol.

dae

Thanks for looking into this Abdo!

dae · 2023-12-07T01:43:39Z

AnkiDroid/src/main/java/com/ichi2/anki/pages/ImageOcclusion.kt

+
+class ImageOcclusion : PageFragment() {
+
+    override val title = R.string.image_occlusion


You can use TR.notetypesImageOcclusionName() to avoid having to add a duplicate string.

dae · 2023-12-07T01:50:36Z

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

            else -> {}
        }

+        col.backend.addImageOcclusionNotetype()


Technically, we probably should be using a background task for the desktop as well. If the user has a large number of notetypes + slow disk, the operation to fetch all the notetypes could potentially block for some time.

For methods that access the collection, standard practice is to write a little wrapper function in the Collection class and call that inside withCol, instead of using the backend directly. We only access the backend directly for operations that don't require/want the collection lock (such as checking progress). One problem this avoids is the collection is guaranteed not to be closed in the middle of an operation running inside withCol.

dae · 2023-12-07T01:51:24Z

AnkiDroid/src/main/java/com/ichi2/anki/pages/AnkiServer.kt

+            "getImageOcclusionNote" -> CollectionManager.getBackend().getImageOcclusionNoteRaw(bytes)
+            "getImageForOcclusionFields" -> CollectionManager.getBackend().getImageOcclusionFieldsRaw(bytes)
+            "addImageOcclusionNote" -> CollectionManager.getBackend().addImageOcclusionNoteRaw(bytes)
+            "updateImageOcclusionNote" -> CollectionManager.getBackend().updateImageOcclusionNoteRaw(bytes)


These would be better implemented as wrappers inside a withCol block.

dae · 2023-12-10T00:12:52Z

No way to edit the Comments field since it's not exposed in the IO editor. @dae is that because it's not a protected field?

Sorry, missed that comment. I think when @krmanik originally introduced this feature, we didn't have a comments field in the notetype. Presumably when it was later added, we forgot to update the mobile-only entrypoint. We should probably keep it hidden until we can transition to the desktop editor implementation, as I think the mobile-only entrypoint will fail if that field is missing, and we don't protect it. Once we've migrated all clients to the desktop editor, we'll be able to remove the protection on header/back extra as well, so users can remove them if they wish.

abdnh · 2023-12-10T01:18:01Z

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

+            }
+            mPasteImaegOcclusionImageButton?.text = TR.notetypesIoPasteImageFromClipboard()
+            mPasteImaegOcclusionImageButton?.setOnClickListener {
+                if (ClipboardUtil.hasImage(clipboard)) {


Clipboard support is not properly tested yet. I wrote this referencing similar features across the codebase. I've not managed to copy images to the clipboard for some reason (tested copying from Chrome).

Just confirmed this is working. But maybe hasImage() should be extended to support all accepted types? https://github.com/ankitects/anki/blob/6f3550464d37aee1b8b784e431cbfce8382d3ce7/rslib/src/image_occlusion/imagedata.rs#L154

This can be left as a TODO if you want

abdnh · 2023-12-10T01:19:20Z

AnkiDroid/src/main/java/com/ichi2/anki/pages/AnkiServer.kt

+            "getImageForOcclusionFields" -> withCol { getImageOcclusionFieldsRaw(bytes) }
+            "addImageOcclusionNote" -> withCol { addImageOcclusionNoteRaw(bytes) }.also {
+                activity.launchCatchingTask {
+                    // Allow time for toast message to appear before closing editor


@dae does AnkiMobile show the toast messages? If so, how is this handled there?

It 'cheats': the screen is closed immediately, and it prints something else:

showPopup(tr.importingNoteAdded(count: 1))

This may be somewhat related - a crash showing JS dialogs/confirms when AnkiPages tried to open an AlertDialog after the AnkiPages object was destroyed (if my hypothesis there is correct) #14920 - I see some other discussion in this area and I haven't read all comments so this comment should be considered as an isolated bit of context just in case, and it may not be useful

david-allison

Comments from a few days ago, things may have moved on

david-allison · 2023-12-08T13:03:30Z

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

                // nothing was changed by the note editor so just redraw the card
                redrawCard()
+            } else if (resultCode == NoteEditor.RESULT_UPDATED_IO_NOTE) {
+                displayCardQuestion(true)


(question, not a request)

Is an OpChanges exposed which we can use inside an undoableOp instead?

Both adding and updating I/O notes return OpChanges, so that should be possible, but as the calls are currently of the Raw variant, the return value would need to be decoded as OpChangesOnly.

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

david-allison · 2023-12-08T13:07:27Z

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

        get() = mEditorNote!!.items().map { it.requireNoNulls() }.toTypedArray()

+    private fun currentNotetypeIsImageOcclusion(): Boolean {
+        println("currentNotetypeIsImageOcclusion: ${currentlySelectedNotetype?.fieldsNames}")


Prefer Timber.d

(if this is something you feel would be useful moving forward, and not temporary debug info that should be removed)

david-allison · 2023-12-08T13:08:36Z

AnkiDroid/src/main/java/com/ichi2/anki/pages/AnkiServer.kt

+            "getImageForOcclusionFields" -> withCol { getImageOcclusionFieldsRaw(bytes) }
+            "addImageOcclusionNote" -> withCol { addImageOcclusionNoteRaw(bytes) }.also {
+                activity.launchCatchingTask {
+                    // Allow time for toast message to appear before closing editor


Android toasts persist globally, as a future extension, we can move this to a native call

For context David, this current screen is a 'temporary' one: the desktop editor integrates the I/O editor directly, and this separate screen is provided just for the mobile clients. Hopefully they'll be able to switch to the desktop editor in the future, making this entrypoint obsolete.

david-allison · 2023-12-08T13:09:57Z

AnkiDroid/src/main/java/com/ichi2/anki/pages/ImageOcclusion.kt

+        private const val ARG_KEY_PATH = "path"
+
+        fun getIntent(context: Context, kind: String, noteOrNotetypeId: Long, imagePath: String?): Intent {
+            val arguments = Bundle().apply {


bundleOf exists in Kotlin

david-allison · 2023-12-08T13:12:16Z

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

                // nothing was changed by the note editor so just redraw the card
                redrawCard()
+            } else if (resultCode == NoteEditor.RESULT_UPDATED_IO_NOTE) {
+                displayCardQuestion(true)


This should be rebased on main, this line conflicts (parameter was removed in API changes)

david-allison · 2023-12-08T13:23:08Z

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

    )

+    private val requestIOEditorLauncher = registerForActivityResult(
+        ActivityResultContracts.StartActivityForResult(),


I believe this can use
https://developer.android.com/reference/androidx/activity/result/contract/ActivityResultContracts.GetContent

abdnh · 2023-12-10T12:02:22Z

@dae Does AnkiMobile have the "Reset Image Occlusion" button? It probably makes more sense as part of the IO page on mobile.

dae · 2023-12-11T01:48:27Z

It doesn't - you need to close and re-open the window if you wish to select a different image.

krmanik · 2023-12-11T05:07:17Z

we didn't have a comments field in the notetype. Presumably when it was later added, we forgot to update the mobile-only entrypoint.

The comments field added later, the header and back extra fields are currently basic implementation and enough for most users.

abdnh · 2023-12-11T20:48:53Z

It doesn't - you need to close and re-open the window if you wish to select a different image.

OK, will skip that then.

This PR is ready for another review if someone has the time.

dae · 2023-12-12T01:13:03Z

This will update the interface to show the change in undo status, and avoids the need for manually refreshing the review screen:

diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/AbstractFlashcardViewer.kt b/AnkiDroid/src/main/java/com/ichi2/anki/AbstractFlashcardViewer.kt
index a56e7ac178..aad2eb588c 100644
--- a/AnkiDroid/src/main/java/com/ichi2/anki/AbstractFlashcardViewer.kt
+++ b/AnkiDroid/src/main/java/com/ichi2/anki/AbstractFlashcardViewer.kt
@@ -297,9 +297,6 @@ abstract class AbstractFlashcardViewer :
             } else if (result.resultCode == RESULT_CANCELED && !reloadRequired) {
                 // nothing was changed by the note editor so just redraw the card
                 redrawCard()
-            } else if (result.resultCode == NoteEditor.RESULT_UPDATED_IO_NOTE) {
-                reloadWebViewContent()
-                onEditedNoteChanged()
             }
         }
     )
diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/pages/AnkiServer.kt b/AnkiDroid/src/main/java/com/ichi2/anki/pages/AnkiServer.kt
index 183867c0cc..b75925708a 100644
--- a/AnkiDroid/src/main/java/com/ichi2/anki/pages/AnkiServer.kt
+++ b/AnkiDroid/src/main/java/com/ichi2/anki/pages/AnkiServer.kt
@@ -19,6 +19,7 @@ package com.ichi2.anki.pages
 
 import android.app.Activity
 import androidx.fragment.app.FragmentActivity
+import anki.collection.OpChanges
 import com.ichi2.anki.CollectionManager
 import com.ichi2.anki.CollectionManager.withCol
 import com.ichi2.anki.NoteEditor
@@ -100,21 +101,31 @@ open class AnkiServer(
             "getImageForOcclusion" -> withCol { getImageForOcclusionRaw(bytes) }
             "getImageOcclusionNote" -> withCol { getImageOcclusionNoteRaw(bytes) }
             "getImageForOcclusionFields" -> withCol { getImageOcclusionFieldsRaw(bytes) }
-            "addImageOcclusionNote" -> withCol { addImageOcclusionNoteRaw(bytes) }.also {
+            "addImageOcclusionNote" -> {
+                val data = withCol {
+                    addImageOcclusionNoteRaw(bytes)
+                }
+                undoableOp { OpChanges.parseFrom(data) }
                 activity.launchCatchingTask {
                     // Allow time for toast message to appear before closing editor
                     delay(1000)
                     activity.setResult(Activity.RESULT_OK)
                     activity.finish()
                 }
+                data
             }
-            "updateImageOcclusionNote" -> withCol { updateImageOcclusionNoteRaw(bytes) }.also {
+            "updateImageOcclusionNote" -> {
+                val data = withCol {
+                    updateImageOcclusionNoteRaw(bytes)
+                }
+                undoableOp { OpChanges.parseFrom(data) }
                 activity.launchCatchingTask {
                     // Allow time for toast message to appear before closing editor
                     delay(1000)
                     activity.setResult(NoteEditor.RESULT_UPDATED_IO_NOTE)
                     activity.finish()
                 }
+                data
             }
             "congratsInfo" -> withCol { congratsInfoRaw(bytes) }
             else -> { throw Exception("unhandled request: $methodName") }

Apart from that, adding and editing seems to work well. The other AD devs are more qualified to comment on the Android-related changes here, but from my backend-focused perspective, it looks good. For your first non-documentation PR, you've done well to get up to speed with Android development so quickly!

ankidroid#14884 (comment)

abdnh · 2023-12-12T23:58:40Z

For your first non-documentation PR, you've done well to get up to speed with Android development so quickly!

Thanks. I worked briefly with Android and Kotlin on a project a couple of years ago.

david-allison

Looks good. Only optional nitpicks

Additional feedback: this could do with maybe 8dp spacing between the button and the 'tags' picker

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

BrayanDSO

LGTM. Really great having IO in AnkIDroid

david-allison · 2023-12-16T13:38:59Z

Pending Strings & squash-merge A squash & force push is required. The PR author may do this to speed up the merge process.

https://github.com/ankidroid/Anki-Android/actions/runs/7232283098 => Updated strings from Crowdin 2023/12/16 13:40 #15003

david-allison · 2023-12-16T17:35:02Z

Really glad to see this one in, thank you 🙏🙏

mikehardy · 2023-12-16T19:06:24Z

This is ridiculous, we have image occlusion in AnkiDroid !?!?! @abdnh - champion work 👑 thank you

I literally never thought I'd see the day

* Integrate image occlusion editor * Remove duplicate translation * Reuse translation from upstream * Wrap in withCol * Run addImageOcclusionNotetype in background * Close IO editor after adding/updating note * Do not load field/tag changes for IO note * Add 'Paste Image from Clipboard' button * Use .apply * Remove print * Use bundleOf() * Use ActivityResultContracts.GetContent() * Fix Edit Occlusions button * Fix reviewer note not being updated after editing IO note * Use undoableOp ankidroid#14884 (comment) * Prefer structural equality * Avoid Hungarian notation * Add bottom padding to IO buttons container * Add TODO about image formats * Replace START animation

github-actions · 2024-04-13T13:29:59Z

Hi there @abdnh! This is the OpenCollective Notice for PRs merged from 2023-12-01 through 2023-12-31

If you are interested in compensation for this work, the process with details is here:

https://github.com/ankidroid/Anki-Android/wiki/OpenCollective-Payment-Process#how-to-get-paid

We only post one comment per person per month to avoid spamming you, regardless of the number of PRs merged, but this note applies to all PRs merged for this month

Please understand that our monthly budget is never guaranteed to cover all claims - the cap on payments-per-person may be lower, but we try to make our process as fair and transparent as possible, we just need your understanding.

Thanks!

Integrate image occlusion editor

0a794d6

abdnh marked this pull request as draft December 6, 2023 14:18

github-actions bot added the Strings label Dec 6, 2023

abdnh commented Dec 6, 2023

View reviewed changes

Remove duplicate translation

8b58483

dae reviewed Dec 7, 2023

View reviewed changes

abdnh added 4 commits December 8, 2023 02:27

Reuse translation from upstream

474b077

Wrap in withCol

8e084e4

Run addImageOcclusionNotetype in background

d10c1e3

Close IO editor after adding/updating note

834ec12

abdnh force-pushed the image-occlusion branch from 2a2cffd to 834ec12 Compare December 8, 2023 09:09

Do not load field/tag changes for IO note

1fcaf64

abdnh mentioned this pull request Dec 9, 2023

Fix IO note count ankitects/anki#2884

Merged

abdnh added 2 commits December 10, 2023 04:13

Add 'Paste Image from Clipboard' button

d8027cd

Use .apply

8facba1

abdnh commented Dec 10, 2023

View reviewed changes

david-allison reviewed Dec 10, 2023

View reviewed changes

abdnh added 6 commits December 10, 2023 13:20

Remove print

ba94433

Use bundleOf()

47b1247

Use ActivityResultContracts.GetContent()

c09efa4

Merge remote-tracking branch 'origin' into image-occlusion

2014063

Fix Edit Occlusions button

385f59e

Fix reviewer note not being updated after editing IO note

6ba377f

abdnh marked this pull request as ready for review December 10, 2023 12:02

Use undoableOp

d92941d

ankidroid#14884 (comment)

david-allison added the Review High Priority Request for high priority review label Dec 13, 2023

david-allison approved these changes Dec 13, 2023

View reviewed changes

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

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

abdnh added 3 commits December 14, 2023 15:34

Prefer structural equality

c2fd7f0

Avoid Hungarian notation

e3615ff

Add bottom padding to IO buttons container

d1de5e9

BrayanDSO reviewed Dec 16, 2023

View reviewed changes

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

BrayanDSO approved these changes Dec 16, 2023

View reviewed changes

BrayanDSO added Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) squash-merge A squash & force push is required. The PR author may do this to speed up the merge process. labels Dec 16, 2023

Add TODO about image formats

84ba4b7

Replace START animation

3e2047e

david-allison merged commit b0d66d6 into ankidroid:main Dec 16, 2023

github-actions bot added this to the 2.17 release milestone Dec 16, 2023

abdnh deleted the image-occlusion branch December 16, 2023 23:51

This comment has been minimized.

Sign in to view

abdnh mentioned this pull request Feb 21, 2024

Can't see nor edit Comments field in Image Occlusion note #15610

Closed

4 tasks

david-allison mentioned this pull request May 14, 2024

[BUG]: image occlusion type reappears when a card is added #16389

Closed

5 tasks

Haz3-jolt mentioned this pull request May 14, 2024

fix: IO type reappears when card is added #16398

Merged

5 tasks


		class ImageOcclusion : PageFragment() {

		override val title = R.string.image_occlusion

Uh oh!

Conversation

abdnh commented Dec 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose / Description

Fixes

Approach

How Has This Been Tested?

Checklist

Issues

Uh oh!

github-actions bot commented Dec 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dae left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dae commented Dec 10, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-allison left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abdnh commented Dec 10, 2023

Uh oh!

dae commented Dec 11, 2023

Uh oh!

krmanik commented Dec 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abdnh commented Dec 11, 2023

Uh oh!

dae commented Dec 12, 2023

Uh oh!

abdnh commented Dec 12, 2023

Uh oh!

david-allison left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

abdnh commented Dec 6, 2023 •

edited

Loading

krmanik commented Dec 11, 2023 •

edited

Loading

david-allison left a comment •

edited

Loading

david-allison commented Dec 16, 2023 •

edited

Loading