This repository was archived by the owner on Dec 21, 2025. It is now read-only.
ADD CUSTOM CARDS BABY #83
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 adds full support for “custom cards” by extending the data model (Param, MiddleParam, AnswerParam), the database schema (NullableCustomCard entity, new TypeConverters, migration to v32), and all related repositories, view models, and UI. In parallel it refactors state handling (moving CDetails state into a FieldParamRepository), replaces SharedPreferences with Jetpack DataStore for app settings, introduces ordering controls for decks, simplifies LaTeX keyboard integration (StepListView composable), and cleans up legacy code (text‐range state, duplicate logic). Sequence diagram for adding a custom card (View → ViewModel → Repository → Database)sequenceDiagram
actor User
participant View as AddCustomCardView
participant VM as AddCardViewModel
participant Repo as FlashCardRepository
participant DB as Database
User->>View: Fill custom card fields and submit
View->>VM: addCustomCard(deck, question, middle, answer, type)
VM->>Repo: insertCustomCard(deck, CustomCD(question, middle, answer), type, isOwner)
Repo->>DB: Save NullableCustomCard entity
DB-->>Repo: Success
Repo-->>VM: Success
VM-->>View: Success
View-->>User: Show success message
Class diagram for new and updated card data types (CustomCard, NullableCustomCard, CardDetails, CDetails)classDiagram
class Param
class MiddleParam
class AnswerParam
class CardDetails {
<<sealed>>
+BasicCD
+ThreeCD
+HintCD
+MultiCD
+NotationCD
+CustomCD
}
class CDetails {
+String question
+String middle
+String answer
+List~String~ choices
+Char correct
+List~String~ steps
+PartOfQorA isQOrA
+Param customQuestion
+MiddleParam customMiddle
+AnswerParam customAnswer
}
class NullableCustomCard {
+Int cardId
+Param question
+MiddleParam? middle
+AnswerParam answer
}
class CustomCard {
+Int cardId
+Param question
+MiddleParam middle
+AnswerParam answer
}
CardDetails <|-- CardDetails.BasicCD
CardDetails <|-- CardDetails.ThreeCD
CardDetails <|-- CardDetails.HintCD
CardDetails <|-- CardDetails.MultiCD
CardDetails <|-- CardDetails.NotationCD
CardDetails <|-- CardDetails.CustomCD
CardDetails.CustomCD o-- Param
CardDetails.CustomCD o-- MiddleParam
CardDetails.CustomCD o-- AnswerParam
NullableCustomCard o-- Param
NullableCustomCard o-- MiddleParam
NullableCustomCard o-- AnswerParam
CustomCard o-- Param
CustomCard o-- MiddleParam
CustomCard o-- AnswerParam
CDetails o-- Param
CDetails o-- MiddleParam
CDetails o-- AnswerParam
Class diagram for FieldParamRepository and CDetails state managementclassDiagram
class FieldParamRepository {
+StateFlow~CDetails~ fields
+updateQ(q: String|Param)
+updateA(a: String|AnswerParam)
+updateM(m: String|MiddleParam)
+updateCh(c: String, idx: Int)
+updateCor(c: Char)
+addStep()
+removeStep()
+updateStep(s: String, idx: Int)
+updateQA(qa: PartOfQorA)
+resetFields()
+updateCustomFields(ti: TypeInfo)
+createFields(init: CDetails)
}
class CDetails
FieldParamRepository o-- CDetails
Class diagram for PreferencesManager refactor (SharedPreferences → DataStore)classDiagram
class PreferencesManager {
-Context context
-CoroutineScope scope
-SharedPreferences sharedPrefs
+StateFlow~Boolean~ darkTheme
+StateFlow~Boolean~ dynamicTheme
+StateFlow~Boolean~ cuteTheme
+StateFlow~Int~ cardAmount
+StateFlow~Int~ reviewAmount
+StateFlow~Int~ height
+StateFlow~Int~ width
+saveDynamicTheme()
+saveDarkTheme()
+saveCuteTheme(value: Boolean)
+saveCardAmount(amount: Int)
+saveReviewAmount(amount: Int)
+saveKatexMenuHeight(h: Int)
+saveKatexMenuWidth(w: Int)
+setDarkTheme(isDarkTheme: Boolean)
+getIsFirstTime(): Boolean
+setIsFirstTime()
}
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:
- The LocalCTToSBCT converter currently skips CT.Custom types (logging and returning null), which will drop custom cards on export—please implement proper mapping for custom cards or handle this case explicitly.
- There’s considerable duplication between AddCustomCard and AddSavedTypeCard (and their file-save logic); consider extracting shared behavior (param rendering, file saving) into reusable helper functions/composables to reduce boilerplate.
- This PR spans data-model migrations, new DataStore flows, custom card layers, and UI changes in one go—it would be much easier to review and validate if broken into smaller, focused PRs (e.g. persistence, core models, UI).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The LocalCTToSBCT converter currently skips CT.Custom types (logging and returning null), which will drop custom cards on export—please implement proper mapping for custom cards or handle this case explicitly.
- There’s considerable duplication between AddCustomCard and AddSavedTypeCard (and their file-save logic); consider extracting shared behavior (param rendering, file saving) into reusable helper functions/composables to reduce boilerplate.
- This PR spans data-model migrations, new DataStore flows, custom card layers, and UI changes in one go—it would be much easier to review and validate if broken into smaller, focused PRs (e.g. persistence, core models, UI).
## Individual Comments
### Comment 1
<location> `app/src/main/java/com/belmontCrest/cardCrafter/model/application/PreferencesManager.kt:101` </location>
<code_context>
+ )
+
+ val width = context.dataStore.data.map { preferences ->
+ preferences[WIDTH] ?: Int.MIN_VALUE
+ }.stateIn(
+ scope = scope,
</code_context>
<issue_to_address>
Default value for width is Int.MIN_VALUE, which may be problematic.
Consider using a default value like 200 for width to align with the height default and prevent potential layout issues.
Suggested implementation:
```
val width = context.dataStore.data.map { preferences ->
preferences[WIDTH] ?: 200
}.stateIn(
scope = scope,
```
```
started = SharingStarted.WhileSubscribed(TIMEOUT_MILLIS),
initialValue = 200
)
```
</issue_to_address>
### Comment 2
<location> `app/src/main/java/com/belmontCrest/cardCrafter/views/cardViews/addCardViews/AddNotationCard.kt:121` </location>
<code_context>
+ horizontalAlignment = Alignment.CenterHorizontally,
+ modifier = Modifier
+ .verticalScroll(scrollState)
+ .zIndex(-1f)
+ ) {
+ ParamChooser(
</code_context>
<issue_to_address>
Setting zIndex to -1f on the main Column may cause layering issues.
A negative zIndex may cause the main content to be obscured or unresponsive. Please confirm this is intentional and does not affect user interactions.
</issue_to_address>
### Comment 3
<location> `app/src/main/java/com/belmontCrest/cardCrafter/views/miscFunctions/Functions.kt:214` </location>
<code_context>
+ val targetWidth: Int
+ val targetHeight: Int
+
+ if (width > height) {
+ targetWidth = maxWidth
+ targetHeight = (maxWidth / aspectRatio).toInt()
</code_context>
<issue_to_address>
Bitmap resizing logic may distort images if aspect ratio is not handled carefully.
Using min(maxWidth / width, maxHeight / height) as the scaling factor will ensure neither dimension exceeds the bounds, even for extreme aspect ratios.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
val aspectRatio = width.toFloat() / height
val targetWidth: Int
val targetHeight: Int
if (width > height) {
targetWidth = maxWidth
targetHeight = (maxWidth / aspectRatio).toInt()
} else {
targetHeight = maxHeight
targetWidth = (maxHeight * aspectRatio).toInt()
}
return bitmap.scale(targetWidth, targetHeight)
=======
val widthF = width.toFloat()
val heightF = height.toFloat()
val scale = minOf(maxWidth / widthF, maxHeight / heightF)
val targetWidth = (widthF * scale).toInt()
val targetHeight = (heightF * scale).toInt()
return bitmap.scale(targetWidth, targetHeight)
>>>>>>> REPLACE
</suggested_fix>
### Comment 4
<location> `app/src/main/java/com/belmontCrest/cardCrafter/views/cardViews/editCardViews/EditNotationCard.kt:44` </location>
<code_context>
val coroutineScope = rememberCoroutineScope()
val (fields, showKB, selectedKB) = collectNotationFieldsAsStates(vm)
- val (selection, composition) = collectTextRangesAsStates(vm)
- var selectedQSymbol by rememberSaveable {
+ var selectedSymbol by rememberSaveable {
mutableStateOf(KaTeXMenu(null, SelectedAnnotation.Idle))
}
- var selectedASymbol by rememberSaveable {
- mutableStateOf(KaTeXMenu(null, SelectedAnnotation.Idle))
- }
</code_context>
<issue_to_address>
Redundant state variables for selectedQSymbol and selectedASymbol removed.
If the UI needs to track different symbols for question and answer fields independently, consider keeping separate state variables.
</issue_to_address>
### Comment 5
<location> `app/src/main/java/com/belmontCrest/cardCrafter/views/cardViews/cardDeckViews/cardTypeView/customCardParams/CustomCardParams.kt:180` </location>
<code_context>
+ val ci = ContentIcons(getUIStyle)
+ val context = LocalContext.current
+ // Remember and prepare a MediaPlayer for this URI
+ val mediaPlayer = remember(uriString) {
+ MediaPlayer().apply {
+ setDataSource(context, uriString.toUri())
+ setAudioAttributes(
+ AudioAttributes.Builder()
+ .setContentType(AudioAttributes.CONTENT_TYPE_UNKNOWN)
+ .build()
+ )
+ prepare()
+ }
+ }
+ var isPlaying by rememberSaveable { mutableStateOf(false) }
</code_context>
<issue_to_address>
MediaPlayer is prepared synchronously in a Composable, which may block the UI thread.
Preparing MediaPlayer synchronously in the remember block can block the UI thread, especially with large or slow-loading audio files. Use asynchronous preparation to prevent UI jank.
</issue_to_address>
### Comment 6
<location> `app/src/main/java/com/belmontCrest/cardCrafter/views/cardViews/addCardViews/AddCustomCard.kt:76` </location>
<code_context>
) {
var enabled by rememberSaveable { mutableStateOf(true) }
var description by rememberSaveable { mutableStateOf("") }
- var failed = remember { mutableStateOf(false) }
</code_context>
<issue_to_address>
Potential race condition with 'enabled' state in coroutine.
Disabling the button before starting the coroutine will help prevent multiple coroutines from being launched due to rapid clicks.
</issue_to_address>
### Comment 7
<location> `app/src/main/java/com/belmontCrest/cardCrafter/views/cardViews/addCardViews/AddCustomCard.kt:250` </location>
<code_context>
+ scrollState.animateScrollTo(0)
+ }
+ }
+ LaunchedEffect(errorMessage) {
+ delay(1500)
+ vm.clearErrorMessage()
</code_context>
<issue_to_address>
Error message is cleared after a fixed delay regardless of user interaction.
Consider letting users dismiss the error manually or increasing the delay to improve accessibility and ensure users have enough time to read the message.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
LaunchedEffect(errorMessage) {
delay(1500)
vm.clearErrorMessage()
}
=======
// Show error message with a manual dismiss button
if (errorMessage.isNotEmpty()) {
Row(
modifier = Modifier
.fillMaxWidth()
.padding(8.dp)
.background(Color.Red.copy(alpha = 0.1f)),
verticalAlignment = Alignment.CenterVertically
) {
Text(
text = errorMessage,
color = Color.Red,
modifier = Modifier.weight(1f)
)
Spacer(modifier = Modifier.width(8.dp))
Button(
onClick = { vm.clearErrorMessage() },
colors = ButtonDefaults.buttonColors(backgroundColor = Color.Transparent),
elevation = ButtonDefaults.elevation(0.dp, 0.dp)
) {
Text("Dismiss", color = Color.Red)
}
}
// Optionally, also auto-dismiss after a longer delay for accessibility
LaunchedEffect(errorMessage) {
delay(4000) // Increased delay for accessibility
vm.clearErrorMessage()
}
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 8
<location> `app/src/main/java/com/belmontCrest/cardCrafter/model/daoHelpers/DeckHelper.kt:188` </location>
<code_context>
+ LEFT JOIN RankedCards rc ON d.id = rc.deckId
+ ORDER BY d.cardsLeft DESC"""
+ )
+ fun getCCOrderedByCardsLefDesc(currentTime: Long): Flow<List<Int>>
+
+ @Query("SELECT * from decks ORDER BY cardsLeft DESC")
</code_context>
<issue_to_address>
Typo in function name: 'getCCOrderedByCardsLefDesc'.
Rename the function to 'getCCOrderedByCardsLeftDesc' to correct the typo.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
fun getCCOrderedByCardsLefDesc(currentTime: Long): Flow<List<Int>>
=======
fun getCCOrderedByCardsLeftDesc(currentTime: Long): Flow<List<Int>>
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| ) | ||
|
|
||
| val width = context.dataStore.data.map { preferences -> | ||
| preferences[WIDTH] ?: Int.MIN_VALUE |
There was a problem hiding this comment.
suggestion (bug_risk): Default value for width is Int.MIN_VALUE, which may be problematic.
Consider using a default value like 200 for width to align with the height default and prevent potential layout issues.
Suggested implementation:
val width = context.dataStore.data.map { preferences ->
preferences[WIDTH] ?: 200
}.stateIn(
scope = scope,
started = SharingStarted.WhileSubscribed(TIMEOUT_MILLIS),
initialValue = 200
)
| horizontalAlignment = Alignment.CenterHorizontally, | ||
| modifier = Modifier | ||
| .verticalScroll(scrollState) | ||
| .zIndex(-1f) |
There was a problem hiding this comment.
question (bug_risk): Setting zIndex to -1f on the main Column may cause layering issues.
A negative zIndex may cause the main content to be obscured or unresponsive. Please confirm this is intentional and does not affect user interactions.
Comment on lines
+210
to
+222
| val aspectRatio = width.toFloat() / height | ||
| val targetWidth: Int | ||
| val targetHeight: Int | ||
|
|
||
| if (width > height) { | ||
| targetWidth = maxWidth | ||
| targetHeight = (maxWidth / aspectRatio).toInt() | ||
| } else { | ||
| targetHeight = maxHeight | ||
| targetWidth = (maxHeight * aspectRatio).toInt() | ||
| } | ||
|
|
||
| return bitmap.scale(targetWidth, targetHeight) |
There was a problem hiding this comment.
suggestion (bug_risk): Bitmap resizing logic may distort images if aspect ratio is not handled carefully.
Using min(maxWidth / width, maxHeight / height) as the scaling factor will ensure neither dimension exceeds the bounds, even for extreme aspect ratios.
Suggested change
| val aspectRatio = width.toFloat() / height | |
| val targetWidth: Int | |
| val targetHeight: Int | |
| if (width > height) { | |
| targetWidth = maxWidth | |
| targetHeight = (maxWidth / aspectRatio).toInt() | |
| } else { | |
| targetHeight = maxHeight | |
| targetWidth = (maxHeight * aspectRatio).toInt() | |
| } | |
| return bitmap.scale(targetWidth, targetHeight) | |
| val widthF = width.toFloat() | |
| val heightF = height.toFloat() | |
| val scale = minOf(maxWidth / widthF, maxHeight / heightF) | |
| val targetWidth = (widthF * scale).toInt() | |
| val targetHeight = (heightF * scale).toInt() | |
| return bitmap.scale(targetWidth, targetHeight) |
Comment on lines
-44
to
-47
| var selectedQSymbol by rememberSaveable { | ||
| var selectedSymbol by rememberSaveable { | ||
| mutableStateOf(KaTeXMenu(null, SelectedAnnotation.Idle)) | ||
| } | ||
| var selectedASymbol by rememberSaveable { |
There was a problem hiding this comment.
issue: Redundant state variables for selectedQSymbol and selectedASymbol removed.
If the UI needs to track different symbols for question and answer fields independently, consider keeping separate state variables.
| modifier = modifier | ||
| .fillMaxWidth() | ||
| ) | ||
| } |
There was a problem hiding this comment.
issue (performance): MediaPlayer is prepared synchronously in a Composable, which may block the UI thread.
Preparing MediaPlayer synchronously in the remember block can block the UI thread, especially with large or slow-loading audio files. Use asynchronous preparation to prevent UI jank.
| var selectedSymbol by rememberSaveable { | ||
| mutableStateOf(KaTeXMenu(null, SelectedAnnotation.Idle)) | ||
| } | ||
| var enabled by rememberSaveable { mutableStateOf(true) } |
There was a problem hiding this comment.
issue (bug_risk): Potential race condition with 'enabled' state in coroutine.
Disabling the button before starting the coroutine will help prevent multiple coroutines from being launched due to rapid clicks.
Comment on lines
+250
to
+253
| LaunchedEffect(errorMessage) { | ||
| delay(1500) | ||
| vm.clearErrorMessage() | ||
| } |
There was a problem hiding this comment.
suggestion: Error message is cleared after a fixed delay regardless of user interaction.
Consider letting users dismiss the error manually or increasing the delay to improve accessibility and ensure users have enough time to read the message.
Suggested change
| LaunchedEffect(errorMessage) { | |
| delay(1500) | |
| vm.clearErrorMessage() | |
| } | |
| // Show error message with a manual dismiss button | |
| if (errorMessage.isNotEmpty()) { | |
| Row( | |
| modifier = Modifier | |
| .fillMaxWidth() | |
| .padding(8.dp) | |
| .background(Color.Red.copy(alpha = 0.1f)), | |
| verticalAlignment = Alignment.CenterVertically | |
| ) { | |
| Text( | |
| text = errorMessage, | |
| color = Color.Red, | |
| modifier = Modifier.weight(1f) | |
| ) | |
| Spacer(modifier = Modifier.width(8.dp)) | |
| Button( | |
| onClick = { vm.clearErrorMessage() }, | |
| colors = ButtonDefaults.buttonColors(backgroundColor = Color.Transparent), | |
| elevation = ButtonDefaults.elevation(0.dp, 0.dp) | |
| ) { | |
| Text("Dismiss", color = Color.Red) | |
| } | |
| } | |
| // Optionally, also auto-dismiss after a longer delay for accessibility | |
| LaunchedEffect(errorMessage) { | |
| delay(4000) // Increased delay for accessibility | |
| vm.clearErrorMessage() | |
| } | |
| } |
| LEFT JOIN RankedCards rc ON d.id = rc.deckId | ||
| ORDER BY d.cardsLeft DESC""" | ||
| ) | ||
| fun getCCOrderedByCardsLefDesc(currentTime: Long): Flow<List<Int>> |
There was a problem hiding this comment.
issue (typo): Typo in function name: 'getCCOrderedByCardsLefDesc'.
Rename the function to 'getCCOrderedByCardsLeftDesc' to correct the typo.
Suggested change
| fun getCCOrderedByCardsLefDesc(currentTime: Long): Flow<List<Int>> | |
| fun getCCOrderedByCardsLeftDesc(currentTime: Long): Flow<List<Int>> |
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
Add full support for custom card types allowing users to define cards with arbitrary fields (text, notation, image, audio, pairs), introduce a new parameter model (Param, MiddleParam, AnswerParam), and extend UI, view models, repositories, DAOs, and database schema (including migrations) to handle creating, saving, editing, and storing custom cards. Switch user preferences to DataStore, add deck ordering controls, and streamline keyboard/KaTeX handling across card views.
New Features:
Enhancements:
Build:
Chores: