Skip to content

fix(split-cardbrowser): list is empty when toggling to notes only mode #20027

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
sanjaysargam wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(split-cardbrowser): list is empty when toggling to notes only mode #20027

sanjaysargam wants to merge 1 commit into from
+88 −26

Conversation

@sanjaysargam
Copy link
Member

@sanjaysargam sanjaysargam commented Jan 5, 2026

Purpose / Description

java.util.NoSuchElementException: List is empty when toggling Card/Notes

Fixes

Approach

Moved logic to ViewModel with safe handling

How Has This Been Tested?

Chromebook

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

david-allison
david-allison requested changes Jan 5, 2026
View reviewed changes
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

This doesn't seem to be handling the outcome of #19805

In addition, I feel we should prefer null over 0 here.

'No row/no selected row' is a valid state for the browser (for example: if a query produces no rows) and we should handle this.

AnkiDroid/src/main/java/com/ichi2/anki/browser/CardBrowserViewModel.kt Outdated
/**
* Safely determines the current card ID to display in the note editor.
*/
suspend fun safeGetCurrentCardId(): CardId {
Copy link
Member

@david-allison david-allison Jan 5, 2026

Choose a reason for hiding this comment

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

Why use 0 instead of null?

Copy link
Member Author

@sanjaysargam sanjaysargam Jan 6, 2026

Choose a reason for hiding this comment

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

The code expects a CardId (which is a Long), and 0 lets us avoid changing everything to nullable types. 0 serves as our 'no card' sentinel that maintains non-null types

Copy link
Member

@david-allison david-allison Jan 7, 2026

Choose a reason for hiding this comment

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

Use 'null' if it can be null

@david-allison david-allison added the Needs Author Reply Waiting for a reply from the original author label Jan 5, 2026
@sanjaysargam
Copy link
Member Author

sanjaysargam commented Jan 6, 2026

This doesn't seem to be handling the outcome of #19805

Could you elaborate it?

@sanjaysargam
Copy link
Member Author

sanjaysargam commented Jan 6, 2026

'No row/no selected row' is a valid state for the browser (for example: if a query produces no rows) and we should handle this.

I already handles right? or am I missed anything?

david-allison
david-allison requested changes Jan 7, 2026
View reviewed changes
Copy link
Member

@david-allison david-allison left a comment •
edited
Loading

Choose a reason for hiding this comment

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

currentCardId being non-null is a bad abstraction.

It was added in Java in:

and should be nullable now we're in Kotlin (if it even needs to continue to exist)

@sanjaysargam sanjaysargam added Needs Review and removed Needs Author Reply Waiting for a reply from the original author labels Jan 7, 2026
david-allison
david-allison requested changes Jan 8, 2026
View reviewed changes
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

So much better, thanks!

@david-allison david-allison added the Needs Author Reply Waiting for a reply from the original author label Jan 8, 2026
@sanjaysargam sanjaysargam removed the Needs Author Reply Waiting for a reply from the original author label Jan 8, 2026
david-allison
david-allison requested changes Jan 12, 2026
View reviewed changes
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

SO MUCH BETTER!

One method name/implementation issue.

A unit test on the ViewModel side would be ideal.

I'm questioning the domain modelling assumptions that a note may not have cards (we should probably expose this as a warning that the user needs to check database).

Probably add in a TODO to think about this.

@david-allison david-allison added the Needs Author Reply Waiting for a reply from the original author label Jan 12, 2026
@sanjaysargam sanjaysargam removed the Needs Author Reply Waiting for a reply from the original author label Jan 12, 2026
david-allison
david-allison reviewed Jan 12, 2026
View reviewed changes
AnkiDroid/src/main/java/com/ichi2/anki/noteeditor/NoteEditorLauncher.kt Outdated
Comment on lines 168 to 169
* @property cardId The ID of the card to edit.
* @property animation The animation direction.
Copy link
Member

@david-allison david-allison Jan 12, 2026

Choose a reason for hiding this comment

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

This needs a name change, or a new class.

Logically, passing in a null card to edit a card doesn't sit well with me

david-allison
david-allison requested changes Jan 12, 2026
View reviewed changes
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

One issue on EditCard no longer accepting a CardId

libanki/src/main/java/com/ichi2/anki/libanki/Collection.kt Outdated
Comment on lines 696 to 698
* TODO: Notes without cards likely indicate an invalid or corrupted collection state.
* We currently handle this gracefully by returning an empty list,
* we may want to surface this as a warning or integrity check for the user.
Copy link
Member

@david-allison david-allison Jan 12, 2026

Choose a reason for hiding this comment

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

Move this TODO out of libanki,

I missed that this was library code. Handing the issue is an application-level concern

@david-allison david-allison added the Needs Author Reply Waiting for a reply from the original author label Jan 12, 2026
@sanjaysargam sanjaysargam removed the Needs Author Reply Waiting for a reply from the original author label Jan 12, 2026
david-allison
david-allison approved these changes Jan 12, 2026
View reviewed changes
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

Cheers!

@david-allison david-allison added Needs Second Approval Has one approval, one more approval to merge and removed Needs Review labels Jan 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@david-allison david-allison david-allison approved these changes

Assignees

No one assigned

Labels

Needs Second Approval Has one approval, one more approval to merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Split Browser] : java.util.NoSuchElementException: List is empty when toggling Card/Notes

2 participants

@sanjaysargam @david-allison