image occlusion button in note editor

[krmanik]

Added a button in note editor for adding/editing image occlusion notes.

The current implementation will be updated with option have mask editor and note editor in same window not in separate window.

[dae]

A few tweaks are required to get this to a minimum viable level:

• Older notetypes won't have that key:

    setOriginalStockKind({json.dumps(self.note.note_type()["originalStockKind"])});
KeyError: 'originalStockKind'

• Notetype will be missing:

diff --git a/pylib/anki/collection.py b/pylib/anki/collection.py
index 4b924fe17..ae1dd7a54 100644
--- a/pylib/anki/collection.py
+++ b/pylib/anki/collection.py
@@ -466,6 +466,10 @@ class Collection(DeprecatedNamesMixin):
     def get_image_for_occlusion(self, path: str | None) -> GetImageForOcclusionResponse:
         return self._backend.get_image_for_occlusion(path=path)

+    def add_image_occlusion_notetype(self) -> None:
+        "Add if missing."
+        self._backend.add_image_occlusion_notetype()
+
     def add_image_occlusion_note(
         self,
         notetype_id: int,
diff --git a/qt/aqt/addcards.py b/qt/aqt/addcards.py
index ef8a11500..1d0124320 100644
--- a/qt/aqt/addcards.py
+++ b/qt/aqt/addcards.py
@@ -55,6 +55,7 @@ class AddCards(QMainWindow):
         self._last_added_note: Optional[Note] = None
         gui_hooks.operation_did_execute.append(self.on_operation_did_execute)
         restoreGeom(self, "add")
+        self.col.add_image_occlusion_notetype()
         gui_hooks.add_cards_did_init(self)
         self.show()

• Image occlusion window should be closed after user adds/updates an occlusion

The current implementation will be updated with option have mask editor and note editor in same window not in separate window.

Does that mean you plan to change the code to avoid showing an extra window? If so, did you intend for this PR to be a draft at the moment, or did you want the current approach merged in and plan to make those changes in a future update?

[krmanik]

Will be implemented in next PR

Thanks, I have used that to implement the mask editor in note editor, but it needs more improvement. I will check again later.

TODO

• Get notes data from fields
• Handle image replacement if input button selected image again
• Hide Add button, link IO button and hide IO button when notetypes change
• Implementation for Edit mode
• Show Fields when notetypes change
• Load i18n in mask editor

Does that mean you plan to change the code to avoid showing an extra window? If so, did you intend for this PR to be a draft at the moment, or did you want the current approach merged in and plan to make those changes in a future update?

I think it will be better if the mask editor included in note editor, because we still need to do it later. So, I have changed the implementation to include the IO page in note editor.

[krmanik]

We should go with current implementation.
The inclusion of image occlusion in note editor may need more time to implement and I will implement.

[glutanimate]

Hmm, I would strongly prefer if we went with the editor-integrated variant and held off until that is ready, for all of the reasons discussed in the previous PRs. To reiterate, from a UX standpoint, separate windows have multiple negative implications on usability, could be a major source of confusion to users, and will likely result in otherwise avoidable support requests due to accidental note edits, people pressing the wrong "add" button, etc.

From an add-on support standpoint, it would also make it difficult for me to write add-on APIs, not knowing which IO code paths are still bound to fundamentally change and how they will be affected by the editor lifecycle. Chances are that, whatever APIs are introduced now might need to be updated or reworked as we move back to the editor-integrated variant of IO.

We will also be in a position where, knowing that a major UX rework of the feature is still pending, we will not be able to properly follow up on a lot of user feedback until that work is complete.

@dae, you are the best judge on this, but I don't see us critically pressured for time here. I think the most important and time-sensitive thing was to introduce IO to AnkiMobile and satisfy the long-standing feature request behind that. As native IO goes through testing there and eventually reaches the wider AnkiMobile user base, I don't think there will be a lack of user feedback to act on and improve native IO.

Meanwhile on desktop we have the advantage that IOE exists and I think that should give us the luxury to hold off a bit longer. Not for ages of course, but if we're talking about a few weeks, I really see no reason to go with a multiple window solution now – especially as when I gave b6bb864 a try a few days ago, it felt like you had already made a lot of progress in the single-window direction @krmanik

[dae]

Some of your previously-mentioned concerns have already been ameliorated by the approach AnkiMobile uses and Mani has copied here - you can try it out in a fresh profile by opening the add screen and selecting the I/O notetype. It's not perfect, and the necessity to switch to the Note tab to be able cards is still a bit difficult to discover, but there's less room for accidents.

I don't see us critically pressured for time here. I think the most important and time-sensitive thing was to introduce IO to AnkiMobile and satisfy the long-standing feature request behind that.

Yes, that was my primary concern. There would be advantages to having support in the desktop version as well - a larger testing base for one, and allowing users to make edits/new cards on whatever platform they happen to be on at the time, but those things are not super urgent from my perspective.

How do you feel about things Mani? Are you happy to hold off until the editor integration is done, or are you keen to get this into the hands of desktop users sooner rather than later? If you don't want to wait, maybe we could reach some compromise where this requires a debug console command to opt into, with a warning that it's only a temporary solution?

[krmanik]

If it can wait, then I will complete the editor integration. I have viewed the forum and thought that this feature is needed to be merged but it can be wait because on desktop IOE can be used for now.

So, I will complete the integration and push the changes in few weeks. The integration will be better, instead of implementing two features.

[khesed]

I hope it is okay to throw a feature request in here while development is ongoing.

Would it be possible to prompt the user with the masked area and then reveal that masked area? It would be the opposite image occlusion; it'd be image reveal/find. This way, information recall is more active.

Example:

• Prompt the user at the top of the card for
• On the same side, at the bottom of the card, render the hide-all occlusion to the user:
• On flip/answer side, render just the answer of the one occlusion to the user:

[dae]

We're currently focused on an MVP; feature requests are best posted in the suggestions section of the forums (and will probably get more traction after we get the MVP into the hands of users).

[krmanik]

I have integrated the mask editor in note editor, next I will add update button in edit mode and hide occlusion & image field.

@JeremiaWilliams
In the feature request answer will be on question side, a user have to guess the label on occlusion, it can be done. It may be implemented after current feature integration and discussion on forum.

[dae]

The integration seems to work well! Two issues I noticed:

• The first is likely the fault of my recent changes: the image is constrained to the original dimensions of the window, so if you start occluding in a small window and then resize it to become larger, the image is stuck at the smaller size. The size to fit button seems to force that size, instead of adjusting to the new window dimensions.
• Instead of note/mask buttons/labels, what if we had a single icon that acts a a toggle? In the add screen, clicking it would prompt to select an image, switch to the mask mode, and the button would be depressed. If users press it again, it switches back to notes mode and becomes undepressed. Press it again and it switches back to masks / is depressed again. WDYT?

[dae]

Rebasing over the main branch should fix the compile error

[krmanik]

What could be possible fix for this? I have rebased and tried to fix.

./tools/web-watch 
033cn    Finished dev [unoptimized + debuginfo] target(s) in 0.15s
ninja: error: unknown target 'qt/aqt', did you mean 'qt_aqt'?
    Finished dev [unoptimized + debuginfo] target(s) in 0.14s
     Running `out/rust/debug/configure`
ninja: error: unknown target 'qt/aqt', did you mean 'qt_aqt'?

Edit: fixed using

cargo clean
rm -rf out/ target/ node_modules/

[dae]

I've pushed a fix for web-watch; I forgot to update it when I made related changes.

[krmanik]

The integration is almost completed.

[dae]

Thanks Mani, it seems to work well. I needed to make a change to be able to open the add screen from an older collection; I've pushed a fix for that.

The integration in the editor could be a bit cleaner from a code perspective (e.g. global styling and a somewhat fragile adjustMaskEditorPosition()), but I presume you've done it that way to continue supporting the use outside of the editor case for now.

Our Python/TS editor bridge is a bit of a mess at the moment, and is going to need some serious attention in the near future to tidy it up and make it easier for the mobile clients to integrate. Hopefully I'll find time for that in the not-too-distant future.

Also in case you hadn't seen yet, a new AnkiMobile release came out yesterday, and it includes the new I/O support. It's mentioned in the change notes, but I'll mention it here too - thank you again for all the hard work you've put into this :-) I'm sure users are going to love it.

[krmanik]

I have used CSS for adjusting position for maskeditor, during resize the position does not update so I have moved it to JS. The code can also be moved to image-occlusion ts dir.

I've seen AnkiMobile's IO feature update. Users will love this awaited mobile client feature.

[dae]

Once ankidroid/Anki-Android-Backend#294 gets merged, I/O should be ready for exposing in AnkiDroid as well, though they'll need to get through their scoped storage update first.

The code can also be moved to image-occlusion ts dir.

To confirm, are you saying this is a further change you'd like to make, or that you've already made? Are you ready for this to be considered for merging, or would you like to make further changes?

@glutanimate heads up

[krmanik]

To confirm, are you saying this is a further change you'd like to make, or that you've already made? Are you ready for this to be considered for merging, or would you like to make further changes?

It is completed.
I want to change some UI related stuff and check if more code can be moved from note editor svelte (related to io) to io svelte then push the changes.

[krmanik]

It can be merged now.

[dae]

Having played with it a bit more, I'm not sure about hiding the image by default in editing mode. I suspect many image occlusions do not have any associated text, and the information is only contained in the image. Currently when the user clicks on a new note in the browser, they just see the blank fields, and have to manually click on the image occlusion button to see the image. Perhaps it should either show the image occlusion editor by default, or not hide the image in the fields list?

[dae]

A few more issues I'm afraid - mainly just cleaning things up to make it more maintainable. When working on these, it would be great if you could use new commits rather than updating the old ones, as that makes it easier to provide further feedback.

[dae]

Just a reminder this still needs json.dumps

[krmanik]

I have read all the reviewed message and will implement considering the suggested changes. Then push the changes in new commit. Thanks for the review.

[dae]

Thanks as always Mani :-)

[krmanik]

The last commit has changes for recent reviews. In the commit two thing not considered, and I will implement in next commit.

• If user want to change hide all to hide one notetype for current note
• If user want to change/replace image for current note

[dae]

Do we need showMaskEditor and the set call here? Why not just directly refer to $isShowMaskEditor?

[krmanik]

This is called on button click and handle the switch between mask editor and note editor.

[dae]

Why not remove the alias and access/set the store directly?

diff --git a/ts/editor/NoteEditor.svelte b/ts/editor/NoteEditor.svelte
index 9ff14bcae..dd5b96237 100644
--- a/ts/editor/NoteEditor.svelte
+++ b/ts/editor/NoteEditor.svelte
@@ -390,14 +390,13 @@ License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html
     import ImageOcclusionPage from "image-occlusion/ImageOcclusionPage.svelte";
     import type { IOMode } from "image-occlusion/lib";
     import { exportShapesToClozeDeletions } from "image-occlusion/shapes/to-cloze";
-    import { ioMaskEditorVisibleStore } from "image-occlusion/store";
+    import { ioMaskEditorVisible } from "image-occlusion/store";
 
     import { mathjaxConfig } from "../editable/mathjax-element";
     import CollapseLabel from "./CollapseLabel.svelte";
     import * as oldEditorAdapter from "./old-editor-adapter";
 
     let isIOImageLoaded = false;
-    const ioMaskEditorVisible = ioMaskEditorVisibleStore;
     let imageOcclusionMode: IOMode | undefined;
     async function setupMaskEditor(options: { html: string; mode: IOMode }) {
         imageOcclusionMode = options.mode;
diff --git a/ts/editor/editor-toolbar/ImageOcclusionButton.svelte b/ts/editor/editor-toolbar/ImageOcclusionButton.svelte
index 550f86d1b..3b17ad121 100644
--- a/ts/editor/editor-toolbar/ImageOcclusionButton.svelte
+++ b/ts/editor/editor-toolbar/ImageOcclusionButton.svelte
@@ -6,7 +6,7 @@ License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html
     import ButtonGroup from "components/ButtonGroup.svelte";
     import DynamicallySlottable from "components/DynamicallySlottable.svelte";
     import IconButton from "components/IconButton.svelte";
-    import { ioMaskEditorVisibleStore } from "image-occlusion/store";
+    import { ioMaskEditorVisible } from "image-occlusion/store";
 
     import ButtonGroupItem, {
         createProps,
@@ -16,7 +16,6 @@ License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html
     import { mdiViewDashboard } from "./icons";
 
     export let api = {};
-    const ioMaskEditorVisible = ioMaskEditorVisibleStore;
 </script>
 
 <ButtonGroup>
@@ -32,7 +31,7 @@ License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html
                 id="io-mask-btn"
                 class={$ioMaskEditorVisible ? "active-io-btn" : ""}
                 on:click={() => {
-                    ioMaskEditorVisibleStore.set(!$ioMaskEditorVisible);
+                    $ioMaskEditorVisible = !$ioMaskEditorVisible;
                 }}
             >
                 {@html mdiViewDashboard}
diff --git a/ts/image-occlusion/store.ts b/ts/image-occlusion/store.ts
index c37736d43..bf56b688f 100644
--- a/ts/image-occlusion/store.ts
+++ b/ts/image-occlusion/store.ts
@@ -10,4 +10,4 @@ export const zoomResetValue = writable(1);
 // it stores the tags for the note in note editor
 export const tagsWritable = writable([""]);
 // it stores the visibility of mask editor
-export const ioMaskEditorVisibleStore = writable(true);
+export const ioMaskEditorVisible = writable(true);

[dae]

Thanks Mani, looking good.

[dae]

Just one comment left above; rest looks good.

[dae]

Thanks for all your work on this Mani. I think it's about time we merge this in! 🎉 I expect to build beta 2 in around 2 weeks if everything goes smoothly. If any issues/tweaks are required, follow-up discussions or PRs are welcome.

[glutanimate]

@dae hmm, I was thinking Mani still wanted to look into these points and was waiting on that before doing a final review / testing pass. I do think there are a few issues left that we need to tackle before this goes out to users as a beta, but opening up separate issues & PRs also works, of course.