Conversation
Contributor
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds per-user default note color support: new preference, ViewModel sync, UI changes (star icon, long-press set-default, edit/delete flows), color types converted to ColorString across dialogs/adapters, DAO LiveData removal, layout/drawable/string resources added/updated, and TRANSLATIONS.md recomputed to 344 total translations. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as Color UI (Adapter / ColorVH)
participant Dialog as Color Dialog (ColorActivityExtensions)
participant Prefs as NotallyXPreferences
participant VM as ViewModel (BaseNoteModel / NotallyModel)
participant DB as Database (BaseNoteDao)
User->>UI: long-press color item
UI->>Dialog: request set-default confirmation
User->>Dialog: confirm set as default
Dialog->>Prefs: set defaultNoteColor.value(newColor)
Prefs-->>VM: notify observers (default changed)
VM->>DB: persist color/default updates if required
DB-->>VM: ack
VM-->>UI: refresh default indicator (star)
Dialog-->>UI: update dialog UI (star/icon)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/com/philkes/notallyx/presentation/view/main/ColorVH.kt (1)
35-58:⚠️ Potential issue | 🟠 MajorExpose the default-state on the swatch, not only via the star icon.
CardViewis the accessible click target here, but itscontentDescriptionis still justcolor. That means the new default marker is visual-only for TalkBack users. Please include the default state in the swatch announcement/state description and treat the icon itself as decorative.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/philkes/notallyx/presentation/view/main/ColorVH.kt` around lines 35 - 58, CardView's contentDescription currently only contains the color and doesn't announce the default state, leaving TalkBack users unaware; update the code that sets CardView.contentDescription to include the default marker when isDefault is true (e.g., append or build a localized string like "ColorName, default" using the same logic that reads isDefault), and make the DefaultColorIcon decorative for accessibility by clearing its contentDescription (or setting it as not important for accessibility) while keeping CardIcon behavior unchanged; reference CardView, DefaultColorIcon, CardIcon, contentDescription, isDefault and showAddIcon when making these changes.
🧹 Nitpick comments (3)
app/src/main/java/com/philkes/notallyx/presentation/activity/note/NoteActionHandler.kt (1)
273-281: Consider simplifying the intermediate set conversion.The inner
.toMutableSet()at line 279 is redundant since the+operator returns an immutableSetregardless of the input type, and the outer.toMutableSet()at line 281 handles the final conversion.♻️ Simplified version
val colors: MutableSet<ColorString> = (withContext(Dispatchers.IO) { NotallyDatabase.getDatabase(activity, observePreferences = false) .value .getBaseNoteDao() .getAllColors() - .toMutableSet() - } + activity.preferences.defaultNoteColor.value) - .toMutableSet() + }.toSet() + activity.preferences.defaultNoteColor.value) + .toMutableSet()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/philkes/notallyx/presentation/activity/note/NoteActionHandler.kt` around lines 273 - 281, The temporary call to .toMutableSet() inside the withContext block is redundant because the set concatenation using + returns an immutable Set which you then convert to mutable with the outer .toMutableSet(); update the code in NoteActionHandler (where the local val colors is built inside the withContext(Dispatchers.IO) block calling NotallyDatabase.getDatabase(...).getBaseNoteDao().getAllColors()) by removing the inner .toMutableSet() so only the final .toMutableSet() is used to produce the MutableSet<ColorString>.app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt (1)
631-637: Good synchronization of default color when a color is deleted/changed.The logic correctly updates the
defaultNoteColorpreference when the deleted color matches the current default. This prevents orphaned default color references.Minor consistency note: The direct
preferences.defaultNoteColor.save(newColor)call differs from thesavePreference()pattern used elsewhere (line 392-394) that wraps inDispatchers.IO. While SharedPreferences commits are typically fast, consider using the existing pattern for consistency:♻️ Optional refactor for consistency
fun changeColor(oldColor: String, newColor: String) { val defaultColor = preferences.defaultNoteColor.value if (oldColor == defaultColor) { - preferences.defaultNoteColor.save(newColor) + savePreference(preferences.defaultNoteColor, newColor) } viewModelScope.launch(Dispatchers.IO) { baseNoteDao.updateColor(oldColor, newColor) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt` around lines 631 - 637, changeColor in BaseNoteModel directly calls preferences.defaultNoteColor.save(newColor) on the calling thread; for consistency with other code (e.g., the savePreference(...) pattern used elsewhere) move this preference write onto the IO dispatcher — either call the existing savePreference(preferences.defaultNoteColor, newColor) helper or wrap the save(...) call in viewModelScope.launch(Dispatchers.IO) { preferences.defaultNoteColor.save(newColor) } so the default preference update matches the IO-dispatched pattern used in this class.app/src/main/java/com/philkes/notallyx/utils/ColorActivityExtensions.kt (1)
71-86: Consider whether long-pressing onCOLOR_DEFAULTshould behave differently.Currently, long-pressing the transparent/no-color option (
COLOR_DEFAULT) prompts to set it as the default color for new notes. While functionally correct, users might find it confusing that "no color" can be the "default color."If this is intentional (allowing users to reset default to "no color"), the UX is acceptable. Otherwise, consider showing a different message or skipping this dialog entirely for
COLOR_DEFAULT.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/philkes/notallyx/utils/ColorActivityExtensions.kt` around lines 71 - 86, Long-pressing the transparent option (BaseNote.COLOR_DEFAULT) currently shows the same "set as default color" dialog which can be confusing; update the long-press branch inside showColorSelectDialog to either skip showing a dialog for BaseNote.COLOR_DEFAULT or present a clearer confirmation that the default will be set to "no color" (use new string resources like R.string.clear_default_color_title and R.string.clear_default_color_message if needed). If you keep a dialog, call preferences.defaultNoteColor.save(BaseNote.COLOR_DEFAULT) and set colorAdapter.defaultColor = BaseNote.COLOR_DEFAULT then notify via colorAdapter.notifyDataSetChanged(); if you prefer skipping, simply return without showing MaterialAlertDialogBuilder when oldColor == BaseNote.COLOR_DEFAULT. Ensure you reference BaseNote.COLOR_DEFAULT, showColorSelectDialog, preferences.defaultNoteColor.save, and colorAdapter.defaultColor in the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/res/layout/dialog_color_picker.xml`:
- Around line 24-31: The DeleteColor ImageButton is visible by default but only
enabled in the old-color flow; change its default visibility to hidden so it
doesn't appear as a dead action in new-color flow: update the DeleteColor view
(android:id="@+id/DeleteColor") in the layout to use a non-visible default
(e.g., GONE or INVISIBLE) and let showEditColorDialog() continue to set it
visible inside the oldColor?.let { ... } block when appropriate.
In `@app/src/main/res/layout/recycler_color.xml`:
- Around line 29-37: The ImageView with id DefaultColorIcon in
recycler_color.xml uses a hardcoded contentDescription "DefaultNoteColor" which
prevents localization; replace the inline string by pointing contentDescription
to a string resource (e.g., `@string/default_note_color`) defined in strings.xml,
or if the parent swatch already exposes the default state to accessibility, make
the icon decorative by setting android:contentDescription="@null" and
android:importantForAccessibility="no" on DefaultColorIcon so it doesn't
duplicate the announcement.
---
Outside diff comments:
In `@app/src/main/java/com/philkes/notallyx/presentation/view/main/ColorVH.kt`:
- Around line 35-58: CardView's contentDescription currently only contains the
color and doesn't announce the default state, leaving TalkBack users unaware;
update the code that sets CardView.contentDescription to include the default
marker when isDefault is true (e.g., append or build a localized string like
"ColorName, default" using the same logic that reads isDefault), and make the
DefaultColorIcon decorative for accessibility by clearing its contentDescription
(or setting it as not important for accessibility) while keeping CardIcon
behavior unchanged; reference CardView, DefaultColorIcon, CardIcon,
contentDescription, isDefault and showAddIcon when making these changes.
---
Nitpick comments:
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/note/NoteActionHandler.kt`:
- Around line 273-281: The temporary call to .toMutableSet() inside the
withContext block is redundant because the set concatenation using + returns an
immutable Set which you then convert to mutable with the outer .toMutableSet();
update the code in NoteActionHandler (where the local val colors is built inside
the withContext(Dispatchers.IO) block calling
NotallyDatabase.getDatabase(...).getBaseNoteDao().getAllColors()) by removing
the inner .toMutableSet() so only the final .toMutableSet() is used to produce
the MutableSet<ColorString>.
In
`@app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt`:
- Around line 631-637: changeColor in BaseNoteModel directly calls
preferences.defaultNoteColor.save(newColor) on the calling thread; for
consistency with other code (e.g., the savePreference(...) pattern used
elsewhere) move this preference write onto the IO dispatcher — either call the
existing savePreference(preferences.defaultNoteColor, newColor) helper or wrap
the save(...) call in viewModelScope.launch(Dispatchers.IO) {
preferences.defaultNoteColor.save(newColor) } so the default preference update
matches the IO-dispatched pattern used in this class.
In `@app/src/main/java/com/philkes/notallyx/utils/ColorActivityExtensions.kt`:
- Around line 71-86: Long-pressing the transparent option
(BaseNote.COLOR_DEFAULT) currently shows the same "set as default color" dialog
which can be confusing; update the long-press branch inside
showColorSelectDialog to either skip showing a dialog for BaseNote.COLOR_DEFAULT
or present a clearer confirmation that the default will be set to "no color"
(use new string resources like R.string.clear_default_color_title and
R.string.clear_default_color_message if needed). If you keep a dialog, call
preferences.defaultNoteColor.save(BaseNote.COLOR_DEFAULT) and set
colorAdapter.defaultColor = BaseNote.COLOR_DEFAULT then notify via
colorAdapter.notifyDataSetChanged(); if you prefer skipping, simply return
without showing MaterialAlertDialogBuilder when oldColor ==
BaseNote.COLOR_DEFAULT. Ensure you reference BaseNote.COLOR_DEFAULT,
showColorSelectDialog, preferences.defaultNoteColor.save, and
colorAdapter.defaultColor in the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9ca35dba-448d-41ca-ab83-f2de10482293
⛔ Files ignored due to path filters (1)
app/translations.xlsxis excluded by!**/*.xlsx
📒 Files selected for processing (16)
TRANSLATIONS.mdapp/src/main/java/com/philkes/notallyx/data/dao/BaseNoteDao.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/LockedActivity.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/main/MainActivity.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/note/NoteActionHandler.ktapp/src/main/java/com/philkes/notallyx/presentation/view/main/ColorAdapter.ktapp/src/main/java/com/philkes/notallyx/presentation/view/main/ColorVH.ktapp/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.ktapp/src/main/java/com/philkes/notallyx/presentation/viewmodel/NotallyModel.ktapp/src/main/java/com/philkes/notallyx/presentation/viewmodel/preference/NotallyXPreferences.ktapp/src/main/java/com/philkes/notallyx/utils/ColorActivityExtensions.ktapp/src/main/res/drawable/star.xmlapp/src/main/res/drawable/star_rate_filled.xmlapp/src/main/res/layout/dialog_color_picker.xmlapp/src/main/res/layout/recycler_color.xmlapp/src/main/res/values/strings.xml
💤 Files with no reviewable changes (1)
- app/src/main/java/com/philkes/notallyx/data/dao/BaseNoteDao.kt
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/philkes/notallyx/utils/ColorActivityExtensions.kt`:
- Around line 74-83: The code is persisting the sentinel BaseNote.COLOR_DEFAULT
into preferences via preferences.defaultNoteColor.save(oldColor), which stores a
placeholder instead of a concrete swatch and can break consumers that expect an
actual color string; change the branch so it does not save the sentinel value —
either resolve the real swatch before saving (e.g. determine the concrete color
from the adapter/item list and call
preferences.defaultNoteColor.save(resolvedColor) and set
colorAdapter.defaultColor = resolvedColor) or simply skip
preferences.defaultNoteColor.save when oldColor == BaseNote.COLOR_DEFAULT and
only update the UI state (colorAdapter.defaultColor and notifyDataSetChanged).
- Around line 215-259: The neutral button is only updated when
preferences.defaultNoteColor changes and its "revert" path calls
performLongClick() with no long-click listener, making it stale and a no-op
after picker moves; fix by wiring the neutral Button (found via
dialog.getButton(AlertDialog.BUTTON_NEUTRAL)) to be updated whenever
binding.ColorPicker.colorEnvelope changes (reuse the existing
ColorPicker.setColorListener or the observer callback to recompute
isDefaultColor, set text/icon/tooltips and onClick), and implement a real revert
action: store the dialog's initial color (e.g., oldColor/initialColor) and have
the neutral button's onClick restore that color into
binding.ColorPicker.setColor / setPaintColor and update ColorCode and
preferences.defaultNoteColor.save as appropriate so the button stays in sync and
actually reverts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1223abb2-3241-46eb-b1b4-20c4e3b39a44
📒 Files selected for processing (5)
app/src/main/java/com/philkes/notallyx/presentation/activity/main/MainActivity.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/note/NoteActionHandler.ktapp/src/main/java/com/philkes/notallyx/utils/ColorActivityExtensions.ktapp/src/main/res/layout/dialog_color_picker.xmlapp/src/main/res/layout/recycler_color.xml
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/main/java/com/philkes/notallyx/presentation/activity/main/MainActivity.kt
- app/src/main/res/layout/recycler_color.xml
- app/src/main/java/com/philkes/notallyx/presentation/activity/note/NoteActionHandler.kt
Comment on lines
+74
to
+83
| if (oldColor == BaseNote.COLOR_DEFAULT) { | ||
| MaterialAlertDialogBuilder(this@showColorSelectDialog) | ||
| .setTitle(R.string.set_as_default_color) | ||
| .setMessage(R.string.set_as_default_color_message) | ||
| .setPositiveButton(R.string.make_default) { _, _ -> | ||
| preferences.defaultNoteColor.save(oldColor) | ||
| colorAdapter.defaultColor = oldColor | ||
| colorAdapter.notifyDataSetChanged() | ||
| } | ||
| .setNegativeButton(android.R.string.cancel, null) |
Contributor
There was a problem hiding this comment.
Don't persist BaseNote.COLOR_DEFAULT as the preference value.
Line 79 saves the placeholder entry, not a concrete swatch. That can overwrite defaultNoteColor with the sentinel token and break later code paths that expect an actual color string. This branch should resolve the real default color value before saving, or avoid writing the preference here altogether.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/com/philkes/notallyx/utils/ColorActivityExtensions.kt`
around lines 74 - 83, The code is persisting the sentinel BaseNote.COLOR_DEFAULT
into preferences via preferences.defaultNoteColor.save(oldColor), which stores a
placeholder instead of a concrete swatch and can break consumers that expect an
actual color string; change the branch so it does not save the sentinel value —
either resolve the real swatch before saving (e.g. determine the concrete color
from the adapter/item list and call
preferences.defaultNoteColor.save(resolvedColor) and set
colorAdapter.defaultColor = resolvedColor) or simply skip
preferences.defaultNoteColor.save when oldColor == BaseNote.COLOR_DEFAULT and
only update the UI state (colorAdapter.defaultColor and notifyDataSetChanged).
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
app/src/main/java/com/philkes/notallyx/utils/ColorActivityExtensions.kt (2)
75-92:⚠️ Potential issue | 🔴 CriticalPersisting sentinel
COLOR_DEFAULTas the preference value corrupts default color state.When
oldColor == BaseNote.COLOR_DEFAULT(the sentinel tile), this branch can save the sentinel itself intopreferences.defaultNoteColor(line 84). Downstream code expecting an actual hex color string will break.The logic here appears inverted—the
COLOR_DEFAULTtile represents "use the preference," not a color that can become the new default. Consider removing this branch entirely or re-evaluating the UX intent: long-pressing the default placeholder tile shouldn't prompt to "set as default.",
Suggested fix: remove sentinel-saving path
if (oldColor == BaseNote.COLOR_DEFAULT) { - if (oldColor == preferences.defaultNoteColor.value) { - showToast(R.string.default_color_hint) - return - } else { - MaterialAlertDialogBuilder(this@showColorSelectDialog) - .setTitle(R.string.set_as_default_color) - .setMessage(R.string.set_as_default_color_message) - .setPositiveButton(R.string.make_default) { _, _ -> - preferences.defaultNoteColor.save(oldColor) - colorAdapter.defaultColor = oldColor - colorAdapter.notifyItemRangeChanged(1, actualColors.size - 1) - } - .setNegativeButton(android.R.string.cancel, null) - .show() - return - } + // The default placeholder tile cannot be set as default itself + showToast(R.string.default_color_hint) + return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/philkes/notallyx/utils/ColorActivityExtensions.kt` around lines 75 - 92, The branch inside showColorSelectDialog that handles oldColor == BaseNote.COLOR_DEFAULT is saving the sentinel value into preferences (via preferences.defaultNoteColor.save(oldColor)) which corrupts default color state; remove or change this branch so long-pressing the sentinel tile does not prompt to "set as default" or save BaseNote.COLOR_DEFAULT—either delete the entire if (oldColor == BaseNote.COLOR_DEFAULT) block or short-circuit it to just return/ignore, and ensure you do not call preferences.defaultNoteColor.save(...) or update colorAdapter.defaultColor/notifyItemRangeChanged with the sentinel value.
239-265:⚠️ Potential issue | 🟠 MajorNeutral button becomes stale after picker moves;
performLongClick()is a no-op.Two issues remain from prior review:
Stale state: The observer (line 239) only fires when
defaultNoteColorLiveData changes. After the user drags the color picker,currentColorchanges but the button text/icon/click-handler stay frozen until the next preference emission.No-op click: Line 252 calls
performLongClick(), but nosetOnLongClickListeneris registered on this button, so the click does nothing.,
Suggested direction
Extract the button-update logic into a local function and call it both from the observer and from the
ColorEnvelopeListenercallback (lines 221-234):fun updateNeutralButton(defaultColor: ColorString) { dialog.getButton(AlertDialog.BUTTON_NEUTRAL)?.apply { val currentColor = binding.ColorPicker.colorEnvelope.toColorString() val isDefaultColor = currentColor == defaultColor setText(if (isDefaultColor) R.string.text_default else R.string.make_default) // ... icon, tooltip ... setOnClickListener { if (!isDefaultColor) { preferences.defaultNoteColor.save(currentColor) } } // Either implement a real long-click action or remove the branch } }Then invoke
updateNeutralButton(preferences.defaultNoteColor.value)insideColorEnvelopeListenerafter updatingpositiveButton.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/philkes/notallyx/utils/ColorActivityExtensions.kt` around lines 239 - 265, The neutral button UI/code is stale because the Observer on preferences.defaultNoteColor only updates when the LiveData emits and performLongClick() is a no-op; extract the button update logic into a helper like updateNeutralButton(defaultColor: ColorString) that reads binding.ColorPicker.colorEnvelope.toColorString(), sets text/icon/tooltip, and installs a real setOnClickListener that calls preferences.defaultNoteColor.save(currentColor) when not default (and either remove the performLongClick branch or implement setOnLongClickListener for the intended behavior); call updateNeutralButton(preferences.defaultNoteColor.value) from both the Observer (observer: Observer<ColorString>) and from the ColorEnvelopeListener callback (after you update the positive button) so the neutral button reflects picker moves immediately.
🧹 Nitpick comments (1)
app/src/main/java/com/philkes/notallyx/utils/ColorActivityExtensions.kt (1)
271-276: UseColorStringtype consistently across function signatures.
showDeleteColorDialogusesList<String>andStringparameters whileshowColorSelectDialogandshowEditColorDialoguseColorString. SinceColorStringis a typealias forString, this compiles but weakens type semantics and makes the API inconsistent.Suggested alignment
-private fun AppCompatActivity.showDeleteColorDialog( - colors: List<String>, - oldColor: String, - setNavigationbarLight: Boolean?, - callback: (selectedColor: String, oldColor: String?) -> Unit, - deleteCallback: (colorToDelete: String, newColor: String) -> Unit, +private fun AppCompatActivity.showDeleteColorDialog( + colors: List<ColorString>, + oldColor: ColorString, + setNavigationbarLight: Boolean?, + callback: (selectedColor: ColorString, oldColor: ColorString?) -> Unit, + deleteCallback: (colorToDelete: ColorString, newColor: ColorString) -> Unit, ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/philkes/notallyx/utils/ColorActivityExtensions.kt` around lines 271 - 276, The function showDeleteColorDialog should use the ColorString typealias instead of raw String to match other APIs; update its signature to accept colors: List<ColorString>, oldColor: ColorString (nullable if previously nullable), and change the callback and deleteCallback parameter types to use ColorString/ColorString? (i.e., callback: (selectedColor: ColorString, oldColor: ColorString?) -> Unit and deleteCallback: (colorToDelete: ColorString, newColor: ColorString) -> Unit). After changing the signature in showDeleteColorDialog, update any call sites to pass/expect ColorString types accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/src/main/java/com/philkes/notallyx/utils/ColorActivityExtensions.kt`:
- Around line 75-92: The branch inside showColorSelectDialog that handles
oldColor == BaseNote.COLOR_DEFAULT is saving the sentinel value into preferences
(via preferences.defaultNoteColor.save(oldColor)) which corrupts default color
state; remove or change this branch so long-pressing the sentinel tile does not
prompt to "set as default" or save BaseNote.COLOR_DEFAULT—either delete the
entire if (oldColor == BaseNote.COLOR_DEFAULT) block or short-circuit it to just
return/ignore, and ensure you do not call preferences.defaultNoteColor.save(...)
or update colorAdapter.defaultColor/notifyItemRangeChanged with the sentinel
value.
- Around line 239-265: The neutral button UI/code is stale because the Observer
on preferences.defaultNoteColor only updates when the LiveData emits and
performLongClick() is a no-op; extract the button update logic into a helper
like updateNeutralButton(defaultColor: ColorString) that reads
binding.ColorPicker.colorEnvelope.toColorString(), sets text/icon/tooltip, and
installs a real setOnClickListener that calls
preferences.defaultNoteColor.save(currentColor) when not default (and either
remove the performLongClick branch or implement setOnLongClickListener for the
intended behavior); call updateNeutralButton(preferences.defaultNoteColor.value)
from both the Observer (observer: Observer<ColorString>) and from the
ColorEnvelopeListener callback (after you update the positive button) so the
neutral button reflects picker moves immediately.
---
Nitpick comments:
In `@app/src/main/java/com/philkes/notallyx/utils/ColorActivityExtensions.kt`:
- Around line 271-276: The function showDeleteColorDialog should use the
ColorString typealias instead of raw String to match other APIs; update its
signature to accept colors: List<ColorString>, oldColor: ColorString (nullable
if previously nullable), and change the callback and deleteCallback parameter
types to use ColorString/ColorString? (i.e., callback: (selectedColor:
ColorString, oldColor: ColorString?) -> Unit and deleteCallback: (colorToDelete:
ColorString, newColor: ColorString) -> Unit). After changing the signature in
showDeleteColorDialog, update any call sites to pass/expect ColorString types
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9290e8c4-f030-420a-8a9c-d775a856010e
📒 Files selected for processing (3)
app/src/main/java/com/philkes/notallyx/presentation/view/main/ColorVH.ktapp/src/main/java/com/philkes/notallyx/utils/ColorActivityExtensions.ktapp/src/main/res/drawable/dashed_background.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/philkes/notallyx/presentation/view/main/ColorVH.kt
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
app/src/main/java/com/philkes/notallyx/utils/ColorActivityExtensions.kt (2)
75-91:⚠️ Potential issue | 🟠 MajorPersisting
BaseNote.COLOR_DEFAULTsentinel as the preference value is still problematic.This code path saves the literal string
"DEFAULT"intopreferences.defaultNoteColorwhen the user long-presses the DEFAULT swatch. While this may be intentional to represent "no custom default," downstream consumers expecting an actual hex color string (e.g.,#RRGGBB) could break or behave unexpectedly.Consider either:
- Document clearly that
defaultNoteColor.valuecan be the sentinel"DEFAULT"and ensure all consumers handle it, or- Use a separate mechanism (e.g., a nullable preference or a dedicated "reset to default" value) to distinguish "user chose the system default" from "user chose a specific color."
Additionally, the UX flow is confusing: line 76 checks
if (oldColor == preferences.defaultNoteColor.value)which compares two strings that are both"DEFAULT"— this condition will be true when the preference is already set to"DEFAULT", showing a toast. But if the preference holds an actual color like#FF0000, then this saves"DEFAULT"over it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/philkes/notallyx/utils/ColorActivityExtensions.kt` around lines 75 - 91, The code currently persists the sentinel BaseNote.COLOR_DEFAULT into preferences.defaultNoteColor when the user chooses the DEFAULT swatch; instead, change the flow so selecting the DEFAULT clears or sets a dedicated "no custom default" value rather than saving the literal sentinel string. In the showColorSelectDialog branch handling oldColor == BaseNote.COLOR_DEFAULT, keep the toast when preferences.defaultNoteColor.value already represents "no custom default", but on confirm call the preference-clearing API (or save null / a dedicated nullable marker) rather than saving BaseNote.COLOR_DEFAULT, and update colorAdapter.defaultColor and notifyItemRangeChanged as before; also ensure preferences.defaultNoteColor's type/consumers are updated to accept a nullable/explicit-reset state. Reference: BaseNote.COLOR_DEFAULT, preferences.defaultNoteColor, colorAdapter.defaultColor, showColorSelectDialog.
241-267:⚠️ Potential issue | 🟠 MajorNeutral button becomes stale when color picker moves;
performLongClick()is a no-op.Two issues remain from previous review:
Stale button state: The observer at line 241 only fires when
preferences.defaultNoteColorchanges. When the user moves the color picker, the neutral button's text ("Default"vs"Make Default"), icon, and click behavior are not updated. The button shows the state from the last preference change, not the current picker color.No-op click handler: At line 254,
setOnClickListener { performLongClick() }is called, but nosetOnLongClickListeneris attached to this button. This makes the click handler do nothing.To fix, the button update logic should also be called from within the
ColorEnvelopeListener(around line 224-236) whenever the picker color changes. Additionally, implement a real action for theisDefaultColorbranch (e.g., show a confirmation to reset the default, or simply disable/hide the button when the color is already the default).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/philkes/notallyx/utils/ColorActivityExtensions.kt` around lines 241 - 267, The neutral button update logic currently inside the Observer<ColorString> (observer) only runs when preferences.defaultNoteColor changes, so wire the same update routine into the ColorEnvelopeListener used by binding.ColorPicker (the listener around where the color picker changes are handled) so the button text, icon and click handler are refreshed on every picker movement; extract the button-update block into a single helper (e.g., updateNeutralButtonState) that reads binding.ColorPicker.colorEnvelope.toColorString(), compares to preferences.defaultNoteColor, sets text/icon and click behavior, and call that helper from both the Observer and the ColorEnvelopeListener inside showEditColorDialog; also replace the no-op setOnClickListener { performLongClick() } by a real action for the isDefaultColor branch (for example disable or hide the button, or show a confirmation/reset dialog) instead of calling performLongClick().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/res/values/strings.xml`:
- Line 237: The string resource named "make_default" is ambiguous; update its
value from "Make default" to a more explicit label such as "Make default color"
to provide context for menus/dialogs and improve localization clarity; locate
the <string name="make_default"> entry and replace the text content accordingly,
then run a quick UI/l10n check to ensure it fits in the layout and update any
translation files if present.
---
Duplicate comments:
In `@app/src/main/java/com/philkes/notallyx/utils/ColorActivityExtensions.kt`:
- Around line 75-91: The code currently persists the sentinel
BaseNote.COLOR_DEFAULT into preferences.defaultNoteColor when the user chooses
the DEFAULT swatch; instead, change the flow so selecting the DEFAULT clears or
sets a dedicated "no custom default" value rather than saving the literal
sentinel string. In the showColorSelectDialog branch handling oldColor ==
BaseNote.COLOR_DEFAULT, keep the toast when preferences.defaultNoteColor.value
already represents "no custom default", but on confirm call the
preference-clearing API (or save null / a dedicated nullable marker) rather than
saving BaseNote.COLOR_DEFAULT, and update colorAdapter.defaultColor and
notifyItemRangeChanged as before; also ensure preferences.defaultNoteColor's
type/consumers are updated to accept a nullable/explicit-reset state. Reference:
BaseNote.COLOR_DEFAULT, preferences.defaultNoteColor, colorAdapter.defaultColor,
showColorSelectDialog.
- Around line 241-267: The neutral button update logic currently inside the
Observer<ColorString> (observer) only runs when preferences.defaultNoteColor
changes, so wire the same update routine into the ColorEnvelopeListener used by
binding.ColorPicker (the listener around where the color picker changes are
handled) so the button text, icon and click handler are refreshed on every
picker movement; extract the button-update block into a single helper (e.g.,
updateNeutralButtonState) that reads
binding.ColorPicker.colorEnvelope.toColorString(), compares to
preferences.defaultNoteColor, sets text/icon and click behavior, and call that
helper from both the Observer and the ColorEnvelopeListener inside
showEditColorDialog; also replace the no-op setOnClickListener {
performLongClick() } by a real action for the isDefaultColor branch (for example
disable or hide the button, or show a confirmation/reset dialog) instead of
calling performLongClick().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b0772f61-ad3b-4af7-9e03-5f92067e85a1
⛔ Files ignored due to path filters (1)
app/translations.xlsxis excluded by!**/*.xlsx
📒 Files selected for processing (4)
TRANSLATIONS.mdapp/src/main/java/com/philkes/notallyx/utils/ColorActivityExtensions.ktapp/src/main/res/layout/dialog_color.xmlapp/src/main/res/values/strings.xml
💤 Files with no reviewable changes (1)
- app/src/main/res/layout/dialog_color.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- TRANSLATIONS.md
| <string name="list">List</string> | ||
| <string name="list_item_auto_sort">Sort List items</string> | ||
| <string name="locked">Locked</string> | ||
| <string name="make_default">Make default</string> |
Contributor
There was a problem hiding this comment.
Make the action label explicit for context.
Line 237 ("Make default") is a bit ambiguous in isolation and may localize poorly. Prefer "Make default color" so the action remains clear in menus/dialogs without surrounding context.
💬 Suggested text update
- <string name="make_default">Make default</string>
+ <string name="make_default">Make default color</string>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <string name="make_default">Make default</string> | |
| <string name="make_default">Make default color</string> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/res/values/strings.xml` at line 237, The string resource named
"make_default" is ambiguous; update its value from "Make default" to a more
explicit label such as "Make default color" to provide context for menus/dialogs
and improve localization clarity; locate the <string name="make_default"> entry
and replace the text content accordingly, then run a quick UI/l10n check to
ensure it fits in the layout and update any translation files if present.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
app/src/main/java/com/philkes/notallyx/utils/ColorActivityExtensions.kt (2)
223-255:⚠️ Potential issue | 🟠 MajorNeutral default button is stale after picker changes, and default-path click is a no-op.
The button state is only recomputed in the
defaultNoteColorobserver, not whenColorPickerchanges. Also, Line 254 usesperformLongClick()but no long-click listener is attached to this button in this dialog.Proposed fix
+ fun updateDefaultButton(defaultColor: ColorString = preferences.defaultNoteColor.value) { + dialog.getButton(AlertDialog.BUTTON_NEUTRAL)?.apply { + val currentColor = binding.ColorPicker.colorEnvelope.toColorString() + val isDefaultColor = currentColor == defaultColor + setText(if (isDefaultColor) R.string.text_default else R.string.make_default) + TooltipCompat.setTooltipText( + this, + getString( + if (isDefaultColor) R.string.default_color_hint + else R.string.set_as_default_color_message + ) + ) + setOnClickListener { + if (!isDefaultColor) { + preferences.defaultNoteColor.save(currentColor) + } else { + // Real action instead of performLongClick no-op: revert picker + binding.ColorPicker.selectByHsvColor(selectedColor) + } + } + } + } ... ColorPicker.setColorListener( ColorEnvelopeListener { color, _ -> TileView.setPaintColor(color.color) val colorString = color.toColorString() val isSaveEnabled = colorString == oldColor || colorString !in colors positiveButton.isEnabled = isSaveEnabled ColorExistsText.visibility = if (isSaveEnabled) View.GONE else View.VISIBLE + updateDefaultButton() if (!editTextChangedByUser) { ColorCode.setText(color.hexCode.argbToRgbString()) } else editTextChangedByUser = false } ) ... - val observer: Observer<ColorString> = Observer { defaultColor -> - dialog.getButton(AlertDialog.BUTTON_NEUTRAL)?.apply { - ... - } - } + val observer: Observer<ColorString> = Observer { defaultColor -> + updateDefaultButton(defaultColor) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/philkes/notallyx/utils/ColorActivityExtensions.kt` around lines 223 - 255, The neutral "default" button is only updated inside the defaultNoteColor Observer and thus becomes stale when the ColorPicker changes, and its else-branch uses performLongClick() with no long-click handler; fix by moving or duplicating the neutral-button update logic so it's run both in the Observer and inside the ColorPicker.setColorListener callback: obtain dialog.getButton(AlertDialog.BUTTON_NEUTRAL), compute currentColor via binding.ColorPicker.colorEnvelope.toColorString(), set text/tooltip accordingly and install an onClickListener that either saves the new default via preferences.defaultNoteColor.save(newColor) or performs the intended default action; remove or replace the performLongClick() call and, if a long-press is desired, explicitly set a proper setOnLongClickListener on that same button.
75-86:⚠️ Potential issue | 🟠 MajorPersisting
BaseNote.COLOR_DEFAULThere stores a sentinel instead of a real color.In this branch,
oldColoris the placeholder token (BaseNote.COLOR_DEFAULT), sopreferences.defaultNoteColor.save(oldColor)writes a non-concrete value into preferences.Proposed fix
if (oldColor == BaseNote.COLOR_DEFAULT) { - if (oldColor == preferences.defaultNoteColor.value) { + if (oldColor == preferences.defaultNoteColor.value) { showToast(R.string.default_color_hint) return } else { MaterialAlertDialogBuilder(this@showColorSelectDialog) .setTitle(R.string.set_as_default_color) .setMessage(R.string.set_as_default_color_message) .setPositiveButton(R.string.make_default) { _, _ -> - preferences.defaultNoteColor.save(oldColor) - colorAdapter.defaultColor = oldColor - colorAdapter.notifyItemRangeChanged(1, actualColors.size - 1) + // Do not persist BaseNote.COLOR_DEFAULT sentinel. + // Resolve a concrete swatch first (or skip saving here). + val resolvedColor = + currentColor?.takeIf { it != BaseNote.COLOR_DEFAULT && it != BaseNote.COLOR_NEW } + ?: preferences.defaultNoteColor.value + if (resolvedColor != BaseNote.COLOR_DEFAULT) { + preferences.defaultNoteColor.save(resolvedColor) + colorAdapter.defaultColor = resolvedColor + colorAdapter.notifyItemRangeChanged(1, actualColors.size - 1) + } else { + showToast(R.string.default_color_hint) + } } .setNegativeButton(android.R.string.cancel, null) .show() return } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/philkes/notallyx/utils/ColorActivityExtensions.kt` around lines 75 - 86, The code saves the sentinel BaseNote.COLOR_DEFAULT into preferences (preferences.defaultNoteColor.save(oldColor)) which stores a non-concrete value; instead resolve the sentinel to a real color before saving. Modify the branch handling oldColor == BaseNote.COLOR_DEFAULT so that you compute a concreteColor (for example pick the adapter/current default or the first entry in actualColors that is not BaseNote.COLOR_DEFAULT, e.g., concreteColor = colorAdapter.defaultColor or actualColors.first { it != BaseNote.COLOR_DEFAULT }) and call preferences.defaultNoteColor.save(concreteColor), then set colorAdapter.defaultColor = concreteColor and notify the adapter; ensure you do not save BaseNote.COLOR_DEFAULT itself.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/src/main/java/com/philkes/notallyx/utils/ColorActivityExtensions.kt`:
- Around line 223-255: The neutral "default" button is only updated inside the
defaultNoteColor Observer and thus becomes stale when the ColorPicker changes,
and its else-branch uses performLongClick() with no long-click handler; fix by
moving or duplicating the neutral-button update logic so it's run both in the
Observer and inside the ColorPicker.setColorListener callback: obtain
dialog.getButton(AlertDialog.BUTTON_NEUTRAL), compute currentColor via
binding.ColorPicker.colorEnvelope.toColorString(), set text/tooltip accordingly
and install an onClickListener that either saves the new default via
preferences.defaultNoteColor.save(newColor) or performs the intended default
action; remove or replace the performLongClick() call and, if a long-press is
desired, explicitly set a proper setOnLongClickListener on that same button.
- Around line 75-86: The code saves the sentinel BaseNote.COLOR_DEFAULT into
preferences (preferences.defaultNoteColor.save(oldColor)) which stores a
non-concrete value; instead resolve the sentinel to a real color before saving.
Modify the branch handling oldColor == BaseNote.COLOR_DEFAULT so that you
compute a concreteColor (for example pick the adapter/current default or the
first entry in actualColors that is not BaseNote.COLOR_DEFAULT, e.g.,
concreteColor = colorAdapter.defaultColor or actualColors.first { it !=
BaseNote.COLOR_DEFAULT }) and call
preferences.defaultNoteColor.save(concreteColor), then set
colorAdapter.defaultColor = concreteColor and notify the adapter; ensure you do
not save BaseNote.COLOR_DEFAULT itself.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 88561357-517b-47e7-af3f-f428efd0d40e
⛔ Files ignored due to path filters (1)
app/translations.xlsxis excluded by!**/*.xlsx
📒 Files selected for processing (36)
TRANSLATIONS.mdapp/src/main/java/com/philkes/notallyx/data/dao/BaseNoteDao.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/LockedActivity.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/main/MainActivity.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/note/NoteActionHandler.ktapp/src/main/java/com/philkes/notallyx/presentation/view/main/ColorAdapter.ktapp/src/main/java/com/philkes/notallyx/presentation/view/main/ColorVH.ktapp/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.ktapp/src/main/java/com/philkes/notallyx/presentation/viewmodel/NotallyModel.ktapp/src/main/java/com/philkes/notallyx/presentation/viewmodel/preference/NotallyXPreferences.ktapp/src/main/java/com/philkes/notallyx/utils/ColorActivityExtensions.ktapp/src/main/res/drawable/dashed_background.xmlapp/src/main/res/drawable/star.xmlapp/src/main/res/drawable/star_rate_filled.xmlapp/src/main/res/layout/dialog_color.xmlapp/src/main/res/layout/dialog_color_picker.xmlapp/src/main/res/layout/recycler_color.xmlapp/src/main/res/values-cs/strings.xmlapp/src/main/res/values-de/strings.xmlapp/src/main/res/values-es/strings.xmlapp/src/main/res/values-fr/strings.xmlapp/src/main/res/values-it/strings.xmlapp/src/main/res/values-my/strings.xmlapp/src/main/res/values-nb/strings.xmlapp/src/main/res/values-nl/strings.xmlapp/src/main/res/values-nn/strings.xmlapp/src/main/res/values-pl/strings.xmlapp/src/main/res/values-pt-rBR/strings.xmlapp/src/main/res/values-ro/strings.xmlapp/src/main/res/values-ru/strings.xmlapp/src/main/res/values-sl/strings.xmlapp/src/main/res/values-uk/strings.xmlapp/src/main/res/values-vi/strings.xmlapp/src/main/res/values-zh-rCN/strings.xmlapp/src/main/res/values-zh-rTW/strings.xmlapp/src/main/res/values/strings.xml
💤 Files with no reviewable changes (2)
- app/src/main/java/com/philkes/notallyx/data/dao/BaseNoteDao.kt
- app/src/main/res/layout/dialog_color.xml
✅ Files skipped from review due to trivial changes (11)
- app/src/main/res/values-ro/strings.xml
- app/src/main/res/values-nn/strings.xml
- app/src/main/res/values-es/strings.xml
- app/src/main/res/values-nl/strings.xml
- app/src/main/res/values-de/strings.xml
- app/src/main/res/values-cs/strings.xml
- app/src/main/res/values-nb/strings.xml
- app/src/main/res/values-ru/strings.xml
- app/src/main/res/values-zh-rTW/strings.xml
- app/src/main/res/values-zh-rCN/strings.xml
- app/src/main/res/values-vi/strings.xml
🚧 Files skipped from review as they are similar to previous changes (11)
- app/src/main/res/values/strings.xml
- app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt
- app/src/main/res/drawable/dashed_background.xml
- app/src/main/res/layout/dialog_color_picker.xml
- app/src/main/java/com/philkes/notallyx/presentation/view/main/ColorVH.kt
- app/src/main/java/com/philkes/notallyx/presentation/viewmodel/preference/NotallyXPreferences.kt
- app/src/main/java/com/philkes/notallyx/presentation/view/main/ColorAdapter.kt
- app/src/main/java/com/philkes/notallyx/presentation/activity/main/MainActivity.kt
- app/src/main/res/drawable/star.xml
- app/src/main/java/com/philkes/notallyx/presentation/activity/LockedActivity.kt
- app/src/main/java/com/philkes/notallyx/presentation/activity/note/NoteActionHandler.kt
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Closes #630
Summary by CodeRabbit
New Features
Documentation