This repository was archived by the owner on Dec 21, 2025. It is now read-only.
katex-menu #80
Merged
katex-menu #80
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR adds multi-select and bulk delete capabilities to the edit-cards screen using new composables and repository flows, centralizes common button components into a shared package, renames the model.uiModels package to model.ui, and simplifies CardTypeRepository and DAO APIs by exposing new Flow- and suspend-based methods. Sequence Diagram for Entering Selection Mode and Selecting CardssequenceDiagram
actor User
participant ECL as EditCardsList
participant ECLVM as EditingCardListViewModel
participant NVM as NavViewModel
participant OCTR as OfflineCardTypeRepository
User->>+ECL: Long-presses CardItem
ECL->>+ECLVM: toggleCard(ct)
ECLVM->>+OCTR: toggleCard(ct)
OCTR-->>-ECLVM:
ECLVM-->>-ECL:
ECL->>+NVM: isSelectingTrue()
NVM-->>-ECL:
ECL-->>-User: UI updates (selection mode active, card selected)
User->>+ECL: Taps another CardItem
ECL->>+ECLVM: toggleCard(ct_other)
ECLVM->>+OCTR: toggleCard(ct_other)
OCTR-->>-ECLVM:
ECLVM-->>-ECL:
ECL-->>-User: UI updates (card selection toggled)
Sequence Diagram for Bulk Deleting Selected CardssequenceDiagram
actor User
participant CLO as CardListOptions
participant NVM as NavViewModel
participant OCTR as OfflineCardTypeRepository
participant CTD as CardTypesDao
User->>+CLO: Clicks "Delete" option (in menu)
CLO->>CLO: Shows DeleteCards dialog
User->>+CLO: Confirms deletion
CLO->>+NVM: deleteCardList()
NVM->>+OCTR: deleteCTs()
OCTR->>+CTD: deleteCardList(selectedCards)
CTD-->>-OCTR:
OCTR-->>-NVM: Returns success/failure
NVM-->>-CLO:
CLO-->>-User: UI updates (cards deleted, dialog dismissed)
Class Diagram: Core Logic for Multi-Select and Bulk DeleteclassDiagram
direction LR
class NavViewModel {
+isSelecting: StateFlow~Boolean~
+isSelectingTrue()
+selectAll()
+clearSelection()
+resetSelection()
+deleteCardList(): Boolean
+resetSearchQuery()
}
class EditingCardListViewModel {
+selectedCards: StateFlow~List~CT~~
+searchQuery: StateFlow~String~
+toggleCard(CT)
}
class OfflineCardTypeRepository {
+selectedCards: StateFlow~List~CT~~
+searchQuery: StateFlow~String~
+toggleCard(CT)
+toggleAllCards(Int)
+clearSelection()
+updateQuery(String)
+resetQuery()
+deleteCTs()
+getAllCardTypesStream(deckId: Int): Flow~List~CT~~
+getAllCardTypes(deckId: Int): List~CT~
}
class CardTypeRepository {
<<Interface>>
+selectedCards: StateFlow~List~CT~~
+searchQuery: StateFlow~String~
+toggleCard(CT)
+toggleAllCards(Int)
+clearSelection()
+updateQuery(String)
+resetQuery()
+deleteCTs()
+getAllCardTypesStream(deckId: Int): Flow~List~CT~~
+getAllCardTypes(deckId: Int): List~CT~
}
class CardTypesDao {
<<DAO>>
+deleteCardList(List~CT~)
+getAllCardTypesStream(deckId: Int): Flow~List~AllCardTypes~~
+getAllCardTypes(deckId: Int): List~AllCardTypes~
}
NavViewModel ..> CardTypeRepository : Uses
EditingCardListViewModel ..> CardTypeRepository : Uses
OfflineCardTypeRepository ..|> CardTypeRepository
OfflineCardTypeRepository ..> CardTypesDao : Uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @xanderlmk - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `app/src/main/java/com/belmontCrest/cardCrafter/views/cardViews/editCardViews/EditingCardsList.kt:105` </location>
<code_context>
- CardSelector(filtered, index)
- }
+ items(filtered.size, key = { index -> filtered[index].getCardId() }) { index ->
+ val selected = selectedCTL.any { it.getCardId() == filtered[index].getCardId() }
+
+ CardItem(
</code_context>
<issue_to_address>
Potential performance issue with linear search for selection.
Using selectedCTL.any for each item results in O(n) lookup per item. Switching to a Set of selected IDs would improve lookup to O(1).
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
items(filtered.size, key = { index -> filtered[index].getCardId() }) { index ->
val selected = selectedCTL.any { it.getCardId() == filtered[index].getCardId() }
CardItem(
=======
val selectedCardIds = selectedCTL.map { it.getCardId() }.toSet()
items(filtered.size, key = { index -> filtered[index].getCardId() }) { index ->
val selected = filtered[index].getCardId() in selectedCardIds
CardItem(
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `app/src/main/java/com/belmontCrest/cardCrafter/localDatabase/dbInterface/repositories/OfflineCardTypeRepository.kt:51` </location>
<code_context>
+ }
+ }
+
+ override suspend fun toggleAllCards(deckId: Int) {
+ _selectedCards.update {
+ val cts = getAllCardTypes(deckId)
</code_context>
<issue_to_address>
toggleAllCards does not handle deselecting all when all are already selected.
Consider implementing a toggle-all/deselect-all feature for improved UX, so users can also deselect all cards when all are selected.
</issue_to_address>
### Comment 3
<location> `app/src/main/java/com/belmontCrest/cardCrafter/navigation/NavViewModel.kt:158` </location>
<code_context>
+ viewModelScope.launch { flashCardRepository.deleteCard(card) }
+ }
+
+ suspend fun deleteCardList(): Boolean {
+ return withContext(Dispatchers.IO) {
+ try {
</code_context>
<issue_to_address>
deleteCardList swallows exceptions and only logs them.
Consider propagating the exception or providing user feedback instead of just logging the error and returning false.
Suggested implementation:
```
suspend fun deleteCardList() {
withContext(Dispatchers.IO) {
cardTypeRepository.deleteCTs()
}
}
```
- Update all usages of `deleteCardList()` to handle exceptions and provide user feedback as appropriate, since it no longer returns a Boolean or swallows exceptions.
- If you want to provide user feedback, catch the exception at the call site and display an error message to the user.
</issue_to_address>
### Comment 4
<location> `app/src/main/java/com/belmontCrest/cardCrafter/uiFunctions/buttons/Buttons.kt:59` </location>
<code_context>
-}
-
-@Composable
-fun SmallAddButton(
- onClick: () -> Unit, iconSize: Int = 45,
- getUIStyle: GetUIStyle, modifier: Modifier
</code_context>
<issue_to_address>
Use Dp type for iconSize instead of Int
Changing the parameter to `Dp` allows callers to pass dp values directly and removes the need for internal `.dp` conversions.
Suggested implementation:
```
fun SmallAddButton(
onClick: () -> Unit, iconSize: Dp = 45.dp,
getUIStyle: GetUIStyle, modifier: Modifier
) {
```
If there are any usages of `iconSize.dp` within the body of `SmallAddButton`, remove the `.dp` as `iconSize` is now already a `Dp`. Also, update all call sites of `SmallAddButton` to pass a `Dp` value for `iconSize` if they specify it.
</issue_to_address>
### Comment 5
<location> `app/src/main/java/com/belmontCrest/cardCrafter/uiFunctions/buttons/Buttons.kt:52` </location>
<code_context>
- ) {
- Icon(
- painter = painterResource(R.drawable.merge),
- contentDescription = "Merge deck",
- )
- Text("Merge deck")
</code_context>
<issue_to_address>
Hardcoded contentDescription string
Use a string resource for the contentDescription to support accessibility and localization.
Suggested implementation:
```
Icon(
painter = painterResource(R.drawable.merge),
contentDescription = stringResource(R.string.merge_deck),
)
```
You also need to add the following to your `res/values/strings.xml` file:
```xml
<string name="merge_deck">Merge deck</string>
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+104
to
+107
| items(filtered.size, key = { index -> filtered[index].getCardId() }) { index -> | ||
| val selected = selectedCTL.any { it.getCardId() == filtered[index].getCardId() } | ||
|
|
||
| CardItem( |
There was a problem hiding this comment.
suggestion (performance): Potential performance issue with linear search for selection.
Using selectedCTL.any for each item results in O(n) lookup per item. Switching to a Set of selected IDs would improve lookup to O(1).
Suggested change
| items(filtered.size, key = { index -> filtered[index].getCardId() }) { index -> | |
| val selected = selectedCTL.any { it.getCardId() == filtered[index].getCardId() } | |
| CardItem( | |
| val selectedCardIds = selectedCTL.map { it.getCardId() }.toSet() | |
| items(filtered.size, key = { index -> filtered[index].getCardId() }) { index -> | |
| val selected = filtered[index].getCardId() in selectedCardIds | |
| CardItem( |
...belmontCrest/cardCrafter/localDatabase/dbInterface/repositories/OfflineCardTypeRepository.kt
Show resolved
Hide resolved
| viewModelScope.launch { flashCardRepository.deleteCard(card) } | ||
| } | ||
|
|
||
| suspend fun deleteCardList(): Boolean { |
There was a problem hiding this comment.
suggestion: deleteCardList swallows exceptions and only logs them.
Consider propagating the exception or providing user feedback instead of just logging the error and returning false.
Suggested implementation:
suspend fun deleteCardList() {
withContext(Dispatchers.IO) {
cardTypeRepository.deleteCTs()
}
}
- Update all usages of
deleteCardList()to handle exceptions and provide user feedback as appropriate, since it no longer returns a Boolean or swallows exceptions. - If you want to provide user feedback, catch the exception at the call site and display an error message to the user.
| } | ||
|
|
||
| @Composable | ||
| fun SmallAddButton( |
There was a problem hiding this comment.
suggestion: Use Dp type for iconSize instead of Int
Changing the parameter to Dp allows callers to pass dp values directly and removes the need for internal .dp conversions.
Suggested implementation:
fun SmallAddButton(
onClick: () -> Unit, iconSize: Dp = 45.dp,
getUIStyle: GetUIStyle, modifier: Modifier
) {
If there are any usages of iconSize.dp within the body of SmallAddButton, remove the .dp as iconSize is now already a Dp. Also, update all call sites of SmallAddButton to pass a Dp value for iconSize if they specify it.
| ) { | ||
| Icon( | ||
| painter = painterResource(R.drawable.merge), | ||
| contentDescription = "Merge deck", |
There was a problem hiding this comment.
suggestion: Hardcoded contentDescription string
Use a string resource for the contentDescription to support accessibility and localization.
Suggested implementation:
Icon(
painter = painterResource(R.drawable.merge),
contentDescription = stringResource(R.string.merge_deck),
)
You also need to add the following to your res/values/strings.xml file:
<string name="merge_deck">Merge deck</string>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Enable multi-select and batch deletion of cards in the editing list, refactor UI components and data flows to support selection state, and streamline model, repository, and navigation logic.
New Features:
Enhancements:
Documentation:
Chores: