-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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.
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
Fixed the empty regular deck check, now uses foreach to check all the… #18007
base: main
Are you sure you want to change the base?
Conversation
First PR! 🚀 We sincerely appreciate that you have taken the time to propose a change to AnkiDroid! Please have patience with us as we are all volunteers - we will get to this as soon as possible. |
I'd really appreciate any feedback on improving this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for your first contribution. It does exactly what I wanted, that's great!
I have a few suggestion in order to improve the codebase quality generally.
Indeed, I didn't really know the part of the codebase you touched and it's only while reviewing your PR that I saw some improvement potential in our current codebase.
In particular, I think we can remove CompletedDeckStatus
. When it was introduced, each status lead to a different action. Today, every status but the empty deck lead to the same action. Thus, the only thing that matter is "is the deck (and its subdeck) empty or not"
So we should have a function that answer this question (and write unit test to ensure it's answered properly) and then either display the snackbar or execute onDeckCompleted
Would you be willing to do all of those changes?
Thanks for the review and suggestions! I'll work on improving it |
… deck nodes in the tree to be empty
aaeb16e
to
2e33db4
Compare
@Arthur-Milchior really grateful for your reviews I was able to refactor this code. Can you have a look at it now? I'd appreciate if there are any improvements still possible in this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Left a comment. Thanks!
decks.isEmpty(did) | ||
} -> CompletedDeckStatus.EMPTY_REGULAR_DECK | ||
else -> CompletedDeckStatus.REGULAR_DECK_NO_MORE_CARDS_TODAY | ||
private suspend fun isDeckAndSubdeckEmpty(did: DeckId): Boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I believe that areDeckAndSubdecksEmpty
is more gramatically correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that the first subject in the phrase is not plural, so I used 'is', but what you said makes sense. Shall I create a new commit to this PR fixing this?
@Arthur-Milchior I'd really appreciate your review if possible. |
Purpose / Description
Creating a deck A::B, if B subdeck is empty and parent A is empty, clicking on the parent it should show 'this deck is empty', instead of the finished deck message.
Fixes
Approach
The earlier code just checked for the following conditions:
getNodeByDid(did).children.isEmpty()
decks.isEmpty(did)
This would fail at cases where the deck has subdecks, which are also empty.
Now the check is done to not just check subdecks, but the count of cards of these subdecks too.
forEach
recursive function ofDeckNode
to traverse the entire tree of subdecks to check if all of them are empty for triggering the empty deck snackbarHow Has This Been Tested?
ran all tests from
./gradlew jacocoTestReport
on emulator (android 15 VanillaIceCream)PC config:
processor: i5 12500H
RAM: 16GB
SDK: 21
results: ran with 0 failures
Also I tested with creation of "A::B::C", "A::B" decks, and adding the cards to the subdecks.
After fix
Update commit - 2e33db4
Now uses a new function
isDeckAndSubdeckEmpty
, and DeckNode implements Iterable, and hence the entire tree empty check is done using deckNode.all{}, which efficiently returns true or false.Also refactored the previous query function that returned enums.
Checklist
Please, go through these checks before submitting the PR.