Skip to content

Conversation

@david-allison
Copy link
Member

Purpose / Description

Creating the CardTemplateEditor was unusual, as it creates an intent, then further modifies it.

I found this unclear when working on

From first glance, the NoteId was unset (as it used a hardcoded string constant rather than EDITOR_NOTE_ID

Approach

  • Define CardOrdinal, as I didn't want to document one Int parameter on getIntent and ignore the rest
  • Introduce CardTemplateEditor.getIntent

How Has This Been Tested?

  • Pixel 9 Pro: both with 'Manage Note Types' and with the Note Editor, on a non-1 ord card

Logcat: Built intent for CardTemplateEditor; ntid: 1381831066614; nid: 1767661996450; ord: 1

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 added Needs Review cleanup Non functional change that would improve the code readability labels Jan 8, 2026
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.

TemplatePreviewerViewModel could use CardOrdinal as well

@BrayanDSO BrayanDSO added Needs Second Approval Has one approval, one more approval to merge and removed Needs Review labels Jan 9, 2026
@david-allison
Copy link
Member Author

Re-requested a review as that added a number more CardOrdinal usages

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.

Still LGTM. Thanks!

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.

good to clarify a confusing concept, I remember when I was first learning about note types and ordinals and it took me quite a while to suss out. Likely reflected (still) in the code. Making everything as explicit as possible can't hurt

I was curious what this looked like for ephemeral items or partially specified items and it seems to make sense, e.g. if you just edit a note type but not on a particular card:

01-09 08:33:41.530 4025 4025 D CardTemplateEditor$Companion: Built intent for CardTemplateEditor; ntid: 1767965588557; nid: null; ord: null

(but everything works as it should)

As I'd expect. Let's go

@mikehardy mikehardy added this pull request to the merge queue Jan 9, 2026
@mikehardy mikehardy 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 Jan 9, 2026
Merged via the queue into ankidroid:main with commit e4ceaa0 Jan 9, 2026
15 checks passed
@github-actions github-actions bot added this to the 2.24 release milestone Jan 9, 2026
@github-actions 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 Jan 9, 2026
@david-allison david-allison deleted the cte-get-intent branch January 9, 2026 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cleanup Non functional change that would improve the code readability

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants