This repository was archived by the owner on Dec 21, 2025. It is now read-only.
more-card-options #82
Merged
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 overhauls state management and UI options across card creation/editing flows, introducing SavedStateHandle persistence in ViewModels, migrating away from the old Fields model to a structured CDetails/flow-based state, centralizing keyboard selection logic, and consolidating preference handling and UI components for consistency and extensibility. Class diagram for new and updated ViewModels and state typesclassDiagram
class AddCardViewModel {
-flashCardRepository: FlashCardRepository
-isOwnerOrCoOwnerRepo: IsOwnerOrCoOwnerRepo
-kbRepository: KeyboardSelectionRepository
-savedStateHandle: SavedStateHandle
-deckUUID: String
+fields: StateFlow<CDetails>
+selection: StateFlow<MyTextRange>
+composition: StateFlow<MyTextRange?>
+showKatexKeyboard: StateFlow<Boolean>
+selectedKB: StateFlow<SelectedKeyboard?>
+resetOffset: StateFlow<Boolean>
+updateQ(q: String)
+updateA(a: String)
+updateM(m: String)
+updateCh(c: String, idx: Int)
+updateCor(c: Char)
+addStep()
+removeStep()
+updateStep(s: String, idx: Int)
+updateQA(qa: PartOfQorA)
+resetDone()
+updateSelectedKB(selectedKeyboard: SelectedKeyboard)
+onCreate()
+toggleKeyboard()
+resetSelectedKB()
}
class EditCardViewModel {
-cardTypeRepository: CardTypeRepository
-sSRepository: ScienceSpecificRepository
-kbRepository: KeyboardSelectionRepository
-savedStateHandle: SavedStateHandle
-cardId: Int
+fields: StateFlow<CDetails>
+selection: StateFlow<MyTextRange>
+composition: StateFlow<MyTextRange?>
+showKatexKeyboard: StateFlow<Boolean>
+selectedKB: StateFlow<SelectedKeyboard?>
+resetOffset: StateFlow<Boolean>
+selectedSLSymbols: StateFlow<List<KaTeXMenu>>
+updateQ(q: String)
+updateA(a: String)
+updateM(m: String)
+updateCh(c: String, idx: Int)
+updateCor(c: Char)
+addStep()
+removeStep()
+updateStep(s: String, idx: Int)
+updateQA(qa: PartOfQorA)
+resetDone()
+updateSelectedKB(selectedKeyboard: SelectedKeyboard)
+onCreate()
+toggleKeyboard()
+resetSelectedKB()
}
class CDetails {
<<sealed class>>
+question: String
+answer: String
+middle: String
+choices: List<String>
+correct: Char
+steps: List<String>
+isQOrA: PartOfQorA
+updateQuestion(q: String): CDetails
+updateAnswer(a: String): CDetails
+updateMiddle(m: String): CDetails
+updateChoices(c: String, idx: Int): CDetails
+updateCorrect(c: Char): CDetails
+addStep(): CDetails
+removeStep(): CDetails
+updateStep(s: String, idx: Int): CDetails
+updateQOrA(qa: PartOfQorA): CDetails
}
class SelectedKeyboard
class MyTextRange
class KaTeXMenu
class ListOfKatexMenu {
+km: List<KaTeXMenu>
}
AddCardViewModel --> CDetails
EditCardViewModel --> CDetails
EditCardViewModel --> ListOfKatexMenu
ListOfKatexMenu --> KaTeXMenu
AddCardViewModel --> SelectedKeyboard
EditCardViewModel --> SelectedKeyboard
AddCardViewModel --> MyTextRange
EditCardViewModel --> MyTextRange
EditCardViewModel --> KaTeXMenu
Class diagram for CDetails sealed class structureclassDiagram
class CDetails {
<<sealed class>>
}
class BasicCD {
+question: String
+answer: String
}
class HintCD {
+question: String
+hint: String
+answer: String
}
class ThreeCD {
+question: String
+middle: String
+answer: String
+isQOrA: PartOfQorA
}
class MultiCD {
+question: String
+choiceA: String
+choiceB: String
+choiceC: String
+choiceD: String
+correct: Char
}
class NotationCD {
+question: String
+steps: List<String>
+answer: String
}
CDetails <|-- BasicCD
CDetails <|-- HintCD
CDetails <|-- ThreeCD
CDetails <|-- MultiCD
CDetails <|-- NotationCD
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 - here's some feedback:
- There’s a lot of duplicated serialization and SavedStateHandle logic in AddCardViewModel and EditCardViewModel—consider extracting the CDetails state‐saving and restoration into a shared helper or base class to DRY up the code.
- The new PreferencesManager manually syncs SharedPreferences with MutableStateFlows—migrating to Jetpack DataStore would give you a firstclass reactive preferences API and eliminate boilerplate.
- This PR touches UI, ViewModels, navigation, and preferences all at once—splitting it into focused changes (e.g. keyboard handling, ViewModel state persistence, preferences migration) would make review and maintenance much easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There’s a lot of duplicated serialization and SavedStateHandle logic in AddCardViewModel and EditCardViewModel—consider extracting the CDetails state‐saving and restoration into a shared helper or base class to DRY up the code.
- The new PreferencesManager manually syncs SharedPreferences with MutableStateFlows—migrating to Jetpack DataStore would give you a firstclass reactive preferences API and eliminate boilerplate.
- This PR touches UI, ViewModels, navigation, and preferences all at once—splitting it into focused changes (e.g. keyboard handling, ViewModel state persistence, preferences migration) would make review and maintenance much easier.
## Individual Comments
### Comment 1
<location> `app/src/main/java/com/belmontCrest/cardCrafter/views/mainViews/DefaultDeckOptions.kt:46` </location>
<code_context>
- clicked: Boolean,
getUIStyle: GetUIStyle
) {
- var expanded by remember { mutableStateOf(false) }
- if (clicked) {
- expanded = false
- }
</code_context>
<issue_to_address>
The 'clicked' parameter and related logic are now unused and can be removed.
Remove the 'clicked' parameter from the function signature and delete all related code to simplify the component.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
@Composable
fun DefaultDeckOptions(
cardAmount: MutableState<String>,
reviewSuccess: Boolean,
cardSuccess: Boolean,
getUIStyle: GetUIStyle
) {
=======
@Composable
fun DefaultDeckOptions(
cardAmount: MutableState<String>,
reviewSuccess: Boolean,
cardSuccess: Boolean,
getUIStyle: GetUIStyle
) {
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `app/src/main/java/com/belmontCrest/cardCrafter/controller/cardHandlers/CardTypeHandler.kt:35` </location>
<code_context>
+ vm: EditCardViewModel, getUIStyle: GetUIStyle, onUpdate: () -> KaTeXMenu
) {
-
- if (ct is CT.Basic) {
- if (!changed) {
- val cardDetails by remember { mutableStateOf(ct.toCardDetails()) }
- fields.question = rememberSaveable { mutableStateOf(cardDetails.question.value) }
- fields.answer = rememberSaveable { mutableStateOf(cardDetails.answer.value) }
- }
- EditBasicCard(fields)
- } else {
- EditBasicCard(fields)
</code_context>
<issue_to_address>
The CardTypeHandler implementations no longer use the CT argument for field initialization.
If CT is no longer required, please remove it from the interface and all related code to prevent confusion.
Suggested implementation:
```
interface CardTypeHandler {
@Composable
fun HandleCardEdit(
vm: EditCardViewModel, getUIStyle: GetUIStyle, onUpdate: () -> KaTeXMenu
)
}
```
```
class BasicCardTypeHandler : CardTypeHandler {
@Composable
override fun HandleCardEdit(
vm: EditCardViewModel, getUIStyle: GetUIStyle, onUpdate: () -> KaTeXMenu
) {
EditBasicCard(vm)
}
}
```
```
class ThreeCardTypeHandler : CardTypeHandler {
@Composable
override fun HandleCardEdit(
```
You will also need to:
1. Remove the `CT` parameter from all other `CardTypeHandler` implementations.
2. Update all calls to `HandleCardEdit` throughout the codebase to no longer pass a `CT` argument.
3. Remove any logic that depends on the `CT` parameter within those implementations.
</issue_to_address>
### Comment 3
<location> `app/src/main/java/com/belmontCrest/cardCrafter/localDatabase/dbInterface/daoInterfaces/deckAndCardDao/CardTypesDao.kt:104` </location>
<code_context>
insertBasicCard(
BasicCard(
cardId = cardId,
- question = fields.question.value,
- answer = fields.answer.value
+ question = fields.question,
+ answer = fields.answer
</code_context>
<issue_to_address>
All usages of fields.question.value, fields.answer.value, etc. are replaced with fields.question, fields.answer, etc.
Ensure all fields are properly initialized and non-null, since the previous MutableState may have provided defaults.
Suggested implementation:
```
// Ensure fields.question and fields.answer are non-null, provide defaults if necessary
val question = fields.question ?: ""
val answer = fields.answer ?: ""
insertBasicCard(
BasicCard(
cardId = cardId,
question = question,
answer = answer
)
)
```
- If there are other usages of `fields.*` (e.g., `fields.extra`, `fields.hint`, etc.) in this or related functions, apply the same null-check/default-value pattern.
- If `CDetails` is a data class you control, consider enforcing non-null types or providing default values at the data class level for even safer code.
</issue_to_address>
### Comment 4
<location> `app/src/main/java/com/belmontCrest/cardCrafter/model/application/PreferencesManager.kt:122` </location>
<code_context>
+ get() = sharedPreferences.getInt("menu_height", 200)
+ set(value) = sharedPreferences.edit { putInt("menu_height", value) }
+
+ private var katexMenuWidth: Int
+ get() = sharedPreferences.getInt("menu_width", Int.MIN_VALUE)
+ set(value) = sharedPreferences.edit { putInt("menu_width", value) }
+}
</code_context>
<issue_to_address>
Default value for katexMenuWidth may be problematic.
Int.MIN_VALUE as a default for menu_width may cause UI issues if unset. Recommend using a sensible default like 200 to prevent negative values in the UI.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
private var katexMenuWidth: Int
get() = sharedPreferences.getInt("menu_width", Int.MIN_VALUE)
set(value) = sharedPreferences.edit { putInt("menu_width", value) }
=======
private var katexMenuWidth: Int
get() = sharedPreferences.getInt("menu_width", 200)
set(value) = sharedPreferences.edit { putInt("menu_width", value) }
>>>>>>> REPLACE
</suggested_fix>
### Comment 5
<location> `app/src/main/java/com/belmontCrest/cardCrafter/model/application/PreferencesManager.kt:43` </location>
<code_context>
+ _width.update { katexMenuWidth }
+ }
+
+ fun saveDynamicTheme() {
+ _dynamicTheme.update { !it }; isDynamicThemeEnabled = _dynamicTheme.value
+ }
+
</code_context>
<issue_to_address>
saveDynamicTheme toggles the value rather than setting it explicitly.
Consider updating saveDynamicTheme to accept a Boolean parameter, aligning its behavior with saveCuteTheme and reducing potential confusion from unintended toggling.
Suggested implementation:
```
fun saveDynamicTheme(enabled: Boolean) {
_dynamicTheme.update { enabled }
isDynamicThemeEnabled = enabled
}
```
You will also need to update all usages of `saveDynamicTheme()` elsewhere in your codebase to pass a Boolean argument, e.g. `saveDynamicTheme(true)` or `saveDynamicTheme(false)`, to match the new function signature.
</issue_to_address>
### Comment 6
<location> `app/src/main/java/com/belmontCrest/cardCrafter/localDatabase/tables/ParcelableConstuctors.kt:58` </location>
<code_context>
+ correct = parcel.readString()!![0]
+ )
+
+fun toParcelableNotationCard(parcel: Parcel): NotationCard? =
+ NotationCard(
+ cardId = parcel.readInt(),
+ question = parcel.readString()!!,
+ steps = listOf(parcel.readString()!!),
+ answer = parcel.readString()!!
+ )
</code_context>
<issue_to_address>
steps field in toParcelableNotationCard only reads a single string.
Currently, only one step is read from the parcel. To support multiple steps, read the list size first and then read each step in a loop.
</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
21
to
36
| import androidx.compose.ui.text.style.TextAlign | ||
| import androidx.compose.ui.unit.dp | ||
| import androidx.compose.ui.unit.sp | ||
| import com.belmontCrest.cardCrafter.R | ||
| import com.belmontCrest.cardCrafter.ui.theme.GetUIStyle | ||
| import com.belmontCrest.cardCrafter.ui.theme.generalSettingsOptionsModifier | ||
| import com.belmontCrest.cardCrafter.uiFunctions.EditIntField | ||
|
|
||
| @Composable | ||
| fun DefaultDeckOptions( |
There was a problem hiding this comment.
suggestion: The 'clicked' parameter and related logic are now unused and can be removed.
Remove the 'clicked' parameter from the function signature and delete all related code to simplify the component.
Suggested change
| import androidx.compose.ui.text.style.TextAlign | |
| import androidx.compose.ui.unit.dp | |
| import androidx.compose.ui.unit.sp | |
| import com.belmontCrest.cardCrafter.R | |
| import com.belmontCrest.cardCrafter.ui.theme.GetUIStyle | |
| import com.belmontCrest.cardCrafter.ui.theme.generalSettingsOptionsModifier | |
| import com.belmontCrest.cardCrafter.uiFunctions.EditIntField | |
| @Composable | |
| fun DefaultDeckOptions( | |
| @Composable | |
| fun DefaultDeckOptions( | |
| cardAmount: MutableState<String>, | |
| reviewSuccess: Boolean, | |
| cardSuccess: Boolean, | |
| getUIStyle: GetUIStyle | |
| ) { |
Comment on lines
-35
to
-41
| if (ct is CT.Basic) { | ||
| if (!changed) { | ||
| val cardDetails by remember { mutableStateOf(ct.toCardDetails()) } | ||
| fields.question = rememberSaveable { mutableStateOf(cardDetails.question.value) } | ||
| fields.answer = rememberSaveable { mutableStateOf(cardDetails.answer.value) } | ||
| } | ||
| EditBasicCard(fields) |
There was a problem hiding this comment.
suggestion: The CardTypeHandler implementations no longer use the CT argument for field initialization.
If CT is no longer required, please remove it from the interface and all related code to prevent confusion.
Suggested implementation:
interface CardTypeHandler {
@Composable
fun HandleCardEdit(
vm: EditCardViewModel, getUIStyle: GetUIStyle, onUpdate: () -> KaTeXMenu
)
}
class BasicCardTypeHandler : CardTypeHandler {
@Composable
override fun HandleCardEdit(
vm: EditCardViewModel, getUIStyle: GetUIStyle, onUpdate: () -> KaTeXMenu
) {
EditBasicCard(vm)
}
}
class ThreeCardTypeHandler : CardTypeHandler {
@Composable
override fun HandleCardEdit(
You will also need to:
- Remove the
CTparameter from all otherCardTypeHandlerimplementations. - Update all calls to
HandleCardEditthroughout the codebase to no longer pass aCTargument. - Remove any logic that depends on the
CTparameter within those implementations.
Comment on lines
-104
to
-105
| question = fields.question.value, | ||
| answer = fields.answer.value |
There was a problem hiding this comment.
suggestion (bug_risk): All usages of fields.question.value, fields.answer.value, etc. are replaced with fields.question, fields.answer, etc.
Ensure all fields are properly initialized and non-null, since the previous MutableState may have provided defaults.
Suggested implementation:
// Ensure fields.question and fields.answer are non-null, provide defaults if necessary
val question = fields.question ?: ""
val answer = fields.answer ?: ""
insertBasicCard(
BasicCard(
cardId = cardId,
question = question,
answer = answer
)
)
- If there are other usages of
fields.*(e.g.,fields.extra,fields.hint, etc.) in this or related functions, apply the same null-check/default-value pattern. - If
CDetailsis a data class you control, consider enforcing non-null types or providing default values at the data class level for even safer code.
Comment on lines
+122
to
+124
| private var katexMenuWidth: Int | ||
| get() = sharedPreferences.getInt("menu_width", Int.MIN_VALUE) | ||
| set(value) = sharedPreferences.edit { putInt("menu_width", value) } |
There was a problem hiding this comment.
suggestion (bug_risk): Default value for katexMenuWidth may be problematic.
Int.MIN_VALUE as a default for menu_width may cause UI issues if unset. Recommend using a sensible default like 200 to prevent negative values in the UI.
Suggested change
| private var katexMenuWidth: Int | |
| get() = sharedPreferences.getInt("menu_width", Int.MIN_VALUE) | |
| set(value) = sharedPreferences.edit { putInt("menu_width", value) } | |
| private var katexMenuWidth: Int | |
| get() = sharedPreferences.getInt("menu_width", 200) | |
| set(value) = sharedPreferences.edit { putInt("menu_width", value) } |
Comment on lines
+43
to
+44
| fun saveDynamicTheme() { | ||
| _dynamicTheme.update { !it }; isDynamicThemeEnabled = _dynamicTheme.value |
There was a problem hiding this comment.
suggestion: saveDynamicTheme toggles the value rather than setting it explicitly.
Consider updating saveDynamicTheme to accept a Boolean parameter, aligning its behavior with saveCuteTheme and reducing potential confusion from unintended toggling.
Suggested implementation:
fun saveDynamicTheme(enabled: Boolean) {
_dynamicTheme.update { enabled }
isDynamicThemeEnabled = enabled
}
You will also need to update all usages of saveDynamicTheme() elsewhere in your codebase to pass a Boolean argument, e.g. saveDynamicTheme(true) or saveDynamicTheme(false), to match the new function signature.
Comment on lines
+58
to
+62
| fun toParcelableNotationCard(parcel: Parcel): NotationCard? = | ||
| NotationCard( | ||
| cardId = parcel.readInt(), | ||
| question = parcel.readString()!!, | ||
| steps = listOf(parcel.readString()!!), |
There was a problem hiding this comment.
issue (bug_risk): steps field in toParcelableNotationCard only reads a single string.
Currently, only one step is read from the parcel. To support multiple steps, read the list size first and then read each step in a loop.
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
Refactor the card creation and editing workflows to use structured UI state objects, SavedStateHandle and StateFlow, and introduce centralized preference management and dynamic theming. Migrate from ad-hoc Fields states to CDetails and MyTextRange models, streamline Compose components for add/edit cards (including notation cards with KaTeX keyboard support), and consolidate keyboard selection into a dedicated repository.
New Features:
Enhancements:
Build:
Documentation: