Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ internal fun BrushDesignerTopBar(
)
} else {
TextButton(onClick = onShowSaveDialog) {
Text(stringResource(R.string.brush_designer_save_to_palette))
Text(stringResource(R.string.brush_designer_save))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Consider using the generic R.string.save instead of the module-specific R.string.brush_designer_save to avoid redundancy, as they both resolve to the same value and the generic one is already used elsewhere in this file (e.g., line 418).

Suggested change
Text(stringResource(R.string.brush_designer_save))
Text(stringResource(R.string.save))

}
TextButton(onClick = onClearCanvas) {
Text(stringResource(R.string.clear))
Expand Down Expand Up @@ -146,7 +146,10 @@ private fun BrushLibraryMenu(
R.string.marker to StockBrushes.marker(),
R.string.pressure_pen to StockBrushes.pressurePen(),
R.string.dashed_line to StockBrushes.dashedLine(),
R.string.emoji_highlighter to StockBrushes.emojiHighlighter("emoji-heart", showMiniEmojiTrail = true),
R.string.emoji_highlighter to StockBrushes.emojiHighlighter(
"emoji-heart",
showMiniEmojiTrail = true
),
)
}

Expand All @@ -166,22 +169,23 @@ private fun BrushLibraryMenu(
onClick = {},
enabled = false
)
stockBrushes.filter { it.first != R.string.emoji_highlighter }.forEach { (nameResId, brushFamily) ->
DropdownMenuItem(
text = { Text(stringResource(nameResId)) },
onClick = {
onLoadBrush(brushFamily)
expanded = false
}
)
}

stockBrushes.filter { it.first != R.string.emoji_highlighter }
.forEach { (nameResId, brushFamily) ->
DropdownMenuItem(
text = { Text(stringResource(nameResId)) },
onClick = {
onLoadBrush(brushFamily)
expanded = false
}
)
}

var showEmojiSubMenu by remember { mutableStateOf(false) }
var itemWidth by remember { mutableStateOf(0) }
var itemHeight by remember { mutableStateOf(0) }
val density = LocalDensity.current
Box(modifier = Modifier.onSizeChanged {

Box(modifier = Modifier.onSizeChanged {
itemWidth = it.width
itemHeight = it.height
}) {
Expand All @@ -195,29 +199,46 @@ private fun BrushLibraryMenu(
DropdownMenu(
expanded = showEmojiSubMenu,
onDismissRequest = { showEmojiSubMenu = false },
offset = DpOffset(x = density.run { itemWidth.toDp() }, y = density.run { -itemHeight.toDp() }),
offset = DpOffset(
x = density.run { itemWidth.toDp() },
y = density.run { -itemHeight.toDp() }),
properties = PopupProperties(focusable = true)
) {
DropdownMenuItem(
text = { Text(stringResource(R.string.emoji_heart)) },
onClick = {
onLoadBrush(StockBrushes.emojiHighlighter("emoji-heart", showMiniEmojiTrail = true))
onLoadBrush(
StockBrushes.emojiHighlighter(
"emoji-heart",
showMiniEmojiTrail = true
)
)
showEmojiSubMenu = false
expanded = false
}
)
DropdownMenuItem(
text = { Text(stringResource(R.string.emoji_star)) },
onClick = {
onLoadBrush(StockBrushes.emojiHighlighter("emoji-star", showMiniEmojiTrail = true))
onLoadBrush(
StockBrushes.emojiHighlighter(
"emoji-star",
showMiniEmojiTrail = true
)
)
showEmojiSubMenu = false
expanded = false
}
)
DropdownMenuItem(
text = { Text(stringResource(R.string.emoji_poop)) },
onClick = {
onLoadBrush(StockBrushes.emojiHighlighter("emoji-poop", showMiniEmojiTrail = true))
onLoadBrush(
StockBrushes.emojiHighlighter(
"emoji-poop",
showMiniEmojiTrail = true
)
)
showEmojiSubMenu = false
expanded = false
}
Expand Down Expand Up @@ -260,7 +281,7 @@ private fun PaletteMenu(
var expanded by remember { mutableStateOf(false) }
Box {
TextButton(onClick = { expanded = true }) {
Text(stringResource(R.string.brush_designer_my_palette))
Text(stringResource(R.string.brush_designer_my_brushes))
}
DropdownMenu(expanded = expanded, onDismissRequest = { expanded = false }) {
if (savedBrushes.isEmpty()) {
Expand Down Expand Up @@ -319,7 +340,7 @@ private fun OverflowMenu(
}
DropdownMenu(expanded = expanded, onDismissRequest = { expanded = false }) {
DropdownMenuItem(
text = { Text(stringResource(R.string.brush_designer_save_to_palette)) },
text = { Text(stringResource(R.string.brush_designer_save)) },
onClick = { onShowSaveDialog(); expanded = false }
)
DropdownMenuItem(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ fun SaveToPaletteDialog(
AlertDialog(
modifier = modifier,
onDismissRequest = onDismiss,
title = { Text(stringResource(R.string.bg_save_to_palette)) },
title = { Text(stringResource(R.string.bg_save_to_cahier)) },
text = {
OutlinedTextField(
value = paletteBrushNameInput,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -485,7 +485,7 @@ fun GraphActionMenu(

Box {
TextButton(onClick = { showPaletteMenu = true }) {
Text(stringResource(R.string.bg_my_palette))
Text(stringResource(R.string.bg_my_brushes))
}

PaletteMenu(
Expand All @@ -509,7 +509,7 @@ fun GraphActionMenu(
shape = RoundedCornerShape(16.dp),
modifier = Modifier.height(40.dp),
) {
Text(stringResource(R.string.bg_save_to_palette))
Text(stringResource(R.string.bg_save))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Consider using the generic R.string.save instead of R.string.bg_save to reduce string resource redundancy.

Suggested change
Text(stringResource(R.string.bg_save))
Text(stringResource(R.string.save))

}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -573,7 +573,7 @@ class BrushGraphViewModel @Inject constructor(
} catch (e: Exception) {
postDebug(
DisplayText.Resource(
com.example.cahier.R.string.bg_err_save_palette,
com.example.cahier.R.string.bg_err_save,
listOf(e.message ?: e.javaClass.simpleName)
)
)
Expand Down Expand Up @@ -611,7 +611,7 @@ class BrushGraphViewModel @Inject constructor(
} catch (e: Exception) {
postDebug(
DisplayText.Resource(
com.example.cahier.R.string.bg_err_load_palette,
com.example.cahier.R.string.bg_err_load,
listOf(e.message ?: e.javaClass.simpleName)
)
)
Expand Down
15 changes: 8 additions & 7 deletions app/src/main/res/values/strings.xml
Original file line number Diff line number Diff line change
Expand Up @@ -105,13 +105,13 @@
<string name="brush_designer_controls_placeholder">Editing controls coming in the next update. Use the preview canvas to draw test strokes.</string>
<string name="brush_designer_close">Close</string>
<string name="brush_designer_stock_brushes">Stock Brushes</string>
<string name="brush_designer_my_palette">My Palette</string>
<string name="brush_designer_my_brushes">My Brushes</string>
<string name="brush_designer_no_saved_brushes">No saved brushes yet</string>
<string name="brush_designer_save_to_palette">Save to Cahier Palette</string>
<string name="brush_designer_save">Save</string>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The string resource brush_designer_save is redundant because it has the same value ('Save') as the generic save string defined on line 43. Since R.string.save is already used in other parts of the application (including within this same module's dialogs), it is better to use the existing generic resource to maintain consistency and reduce localization overhead.

<string name="brush_designer_import">Import</string>
<string name="brush_designer_export">Export</string>
<string name="brush_designer_more_options">More options</string>
<string name="brush_designer_save_dialog_title">Save to Palette</string>
<string name="brush_designer_save_dialog_title">Save to Cahier</string>
<string name="brush_designer_save_dialog_info">• This brush will appear in the main Cahier toolbox.\n• Large textures are stored in RAM. Avoid saving many texture-heavy brushes to prevent performance lag or memory issues.</string>
<string name="brush_designer_brush_name">Brush Name</string>
<string name="brush_designer_tab_tip_shape">Tip Shape</string>
Expand Down Expand Up @@ -278,7 +278,8 @@
<!-- Brush Graph -->
<string name="bg_name_texture">Name Texture</string>
<string name="bg_texture_id">Texture ID</string>
<string name="bg_save_to_palette">Save to Palette</string>
<string name="bg_save">Save</string>
<string name="bg_save_to_cahier">Save to Cahier</string>
Comment on lines +281 to +282
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

There is redundancy between the bg_ prefixed strings and the brush_designer_ prefixed strings. Specifically:

  • bg_save (line 281) is identical to brush_designer_save (line 110) and the generic save (line 43).
  • bg_save_to_cahier (line 282) is identical to brush_designer_save_dialog_title (line 114).
  • bg_my_brushes (line 311) is identical to brush_designer_my_brushes (line 108).

While module-specific prefixes can be useful, for strings that are identical and used across multiple developer tools, it is more maintainable to define a single shared resource (e.g., my_brushes or save_to_cahier) in a common section.

<string name="bg_brush_name">Brush Name</string>
<string name="bg_clear_graph">Clear Graph</string>
<string name="bg_clear_graph_confirmation">Are you sure you want to clear the entire brush graph? This action cannot be undone.</string>
Expand Down Expand Up @@ -307,7 +308,7 @@
<string name="bg_behavior">Behavior</string>
<string name="bg_color_function">Color Function</string>
<string name="bg_texture_layer">Texture Layer</string>
<string name="bg_my_palette">My Palette</string>
<string name="bg_my_brushes">My Brushes</string>
<string name="bg_delete_edge">Delete Edge</string>
<string name="bg_delete_edge_confirmation">Are you sure you want to delete this edge?</string>
<string name="bg_add_node_between">Add Node Between</string>
Expand Down Expand Up @@ -467,8 +468,8 @@
<string name="bg_err_reorganization_failed">Reorganization failed</string>
<string name="bg_err_load_brush_failed">Failed to load brush</string>
<string name="bg_err_cannot_delete_family_node">Cannot delete Family node</string>
<string name="bg_err_save_palette">Failed to save brush to palette: %1$s</string>
<string name="bg_err_load_palette">Failed to load brush from palette: %1$s</string>
<string name="bg_err_save">Failed to save brush to Cahier: %1$s</string>
<string name="bg_err_load">Failed to load brush from Cahier: %1$s</string>
<string name="bg_err_behavior_cannot_accept">Behavior node %1$s cannot accept input from %2$s</string>
<string name="bg_err_behavior_cannot_accept_structural">Behavior node %1$s cannot accept input from structural node %2$s</string>
<string name="bg_err_coat_only_accepts_tip">Coat can only accept input from Tip at the tip port</string>
Expand Down
Loading