Skip to content

fix(image-occlusion): support updating a note's deck#19607

Merged
mikehardy merged 6 commits intoankidroid:mainfrom
david-allison:19605-image-occlusion-edit
Nov 30, 2025
Merged

fix(image-occlusion): support updating a note's deck#19607
mikehardy merged 6 commits intoankidroid:mainfrom
david-allison:19605-image-occlusion-edit

Conversation

@david-allison
Copy link
Member

@david-allison david-allison commented Nov 24, 2025

Purpose / Description

From the Reviewer, I could not edit the deck of an image occlusion note

This functionality was not implemented on ImageOcclusion. Only editing the deck of 'add' was supported, and the 'edit deck' functionality of NoteEditorFragment was hidden

In future, we can handle this inImageOcclusion, BUT:

There are 3 cases:

  • A user wants to edit a card when studying
  • A user wants to edit a selection of cards from the same note [Card Browser - CARDS]
  • A user wants to edit a note [Card Browser - NOTES]
    • What does it mean to edit the deck of a note, is a user aware of this?

And a user can remove the cards they are editing (by removing masks).

Since we don't yet have full control over the process of adding an occlusion note, I decided it was a risk to take the time to implement this, and went for a simple fix

Fixes

Approach

Beforehand, I refactored the Image Occlusion screen:

  • Moving to a discriminated union, to make invalid states non-representable
  • Moving to flows, to reduce

Then:

  • Enable editing the deck in NoteEditorFragment if in edit mode
  • Disable editing the deck in ImageOcclusion if in edit mode

How Has This Been Tested?

Pixel 9 API 35 emulator:

  • Add Image Occlusion note by 'share' & change deck from default
    • ✅ Deck is set on note
    • ✅ Current deck is not changed
  • Add Image Occlusion note by 'add'
    • ✅ Deck is set on note
    • ✅ Current deck is not changed
  • Edit Image Occlusion note in 'Review'
    • ✅ Deck is settable in Note Editor
    • ✅ Deck is not settable in Image Occlusion Editor
  • Edit Image Occlusion note in 'Review' - removing current mask
    • ✅ 'No cloze 1 found on card ...'
  • Edit Image Occlusion card in 'Card Browser'
    • ✅ Deck is changeable in Note Editor
  • Edit Image Occlusion note in 'Card Browser'
Screenshot 2025-11-24 at 20 52 55 Screenshot 2025-11-24 at 20 53 03

Checklist

  • 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

@david-allison david-allison force-pushed the 19605-image-occlusion-edit branch 3 times, most recently from a306e55 to fa05db9 Compare November 24, 2025 20:55
@david-allison david-allison force-pushed the 19605-image-occlusion-edit branch 2 times, most recently from 6073000 to decbc8d Compare November 24, 2025 21:02
@david-allison david-allison added the Needs Author Reply Waiting for a reply from the original author label Nov 25, 2025
@david-allison david-allison added this to the 2.23 release milestone Nov 25, 2025
@david-allison david-allison force-pushed the 19605-image-occlusion-edit branch from decbc8d to b372f5a Compare November 25, 2025 14:51
@david-allison david-allison added Review High Priority Request for high priority review Needs Review and removed Needs Author Reply Waiting for a reply from the original author labels Nov 25, 2025
@david-allison david-allison marked this pull request as ready for review November 25, 2025 14:58
@david-allison david-allison force-pushed the 19605-image-occlusion-edit branch 2 times, most recently from fd4bdbb to a90022f Compare November 25, 2025 15:57
@lukstbit
Copy link
Member

From the Reviewer, I could not edit the deck of an image occlusion note

Unless I'm missing something this isn't possible on desktop either(we can only modify the masks). If this is the case I don't think we should diverge.

We don't quite follow desktop as we don't allow the user to save without any occlusions set but on desktop it just results in an empty card.

@david-allison
Copy link
Member Author

david-allison commented Nov 25, 2025

We supported editing the deck in 2.22, I'd rather avoid the regression

As far as I understand the code, It's the backend rejecting the note with 0 occlusions.

Copy link
Member

@BrayanDSO BrayanDSO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

partial review

@david-allison david-allison force-pushed the 19605-image-occlusion-edit branch 2 times, most recently from 25467bf to 5009b6d Compare November 27, 2025 15:11
BrayanDSO

This comment was marked as resolved.

* Removes nullable properties
* Strongly types 'id' + 'deckId'
* Properties are only available if the mode ('Add'/'Edit') is checked

For issue 19605, we will want to know whether the page is adding
 or editing a note, this will read better if we are not
 assuming existence of properties
I want to make the deck selection conditional on 'ADD' mode
fixes: `1` and `1L` in the JSONObject are output to the same string

improves:

* add JSON syntax highlighting
* fix call: no need to call JSONObject( ) at the call site
Once a note had been created, the deck couldn't be updated.

Fixed by:

* Enable editing the deck in NoteEditorFragment if in edit mode
* Disable editing the deck in ImageOcclusion if in edit mode

In future, we can handle this in ImageOcclusion, BUT:

There are 3 cases:
* A user wants to edit a card when studying
* A user wants to edit a selection of cards from the same note [Card Browser - CARDS]
* A user wants to edit a note [Card Browser - NOTES]
  * What does it mean to edit the deck of a note, is a user aware of this?

And a user can remove the cards they are editing (by removing masks).

Since we don't yet have full control over the process of adding an occlusion note,
 I decided it was a risk to take the time to implement this

Fixes 19605
@david-allison
Copy link
Member Author

Thanks so much for the patch!!

@david-allison david-allison force-pushed the 19605-image-occlusion-edit branch from 5009b6d to afc5248 Compare November 27, 2025 19:14
@BrayanDSO BrayanDSO added Needs Second Approval Has one approval, one more approval to merge and removed Needs Review labels Nov 27, 2025
Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't see anything that looks off, let's go

@mikehardy mikehardy added this pull request to the merge queue Nov 30, 2025
Merged via the queue into ankidroid:main with commit fc7bb43 Nov 30, 2025
15 checks passed
@github-actions github-actions bot removed Needs Second Approval Has one approval, one more approval to merge Review High Priority Request for high priority review labels Nov 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

4 participants