Revert "Discontinue support for Material icon pack"#873
Merged
Conversation
This reverts commit 597ba36
WalkthroughThis pull request introduces a Material icon pack feature to the Valkyrie IDEA plugin. The changes add a new ExpandedGroup composable component for UI layout, extend state management across persistent and in-memory settings with a useMaterialPack boolean property, and implement conditional initialization logic that pre-populates icon pack configurations when the Material pack option is selected. A new UseMaterialPack action enables toggling this feature in the new pack creation flow. Additional UI strings are added to support the feature, and the NestedPacks rendering is updated with a flag to conditionally control nested pack addition. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/common/IconPackEditor.kt (1)
79-84:⚠️ Potential issue | 🟡 Minor
allowAddNestedPacknot honored in the empty-packs branch
AddPackButtonis unconditionally rendered whennestedPacks.isEmpty(), bypassing theallowAddNestedPackguard that is correctly applied in theNestedPackscomposable. WhileuseMaterialPack = truepre-populates nested packs (making the empty branch unreachable in practice today), this is a latent inconsistency.🛡️ Proposed fix
if (inputFieldState.nestedPacks.isEmpty()) { Spacer(8.dp) - AddPackButton( - modifier = Modifier.align(Alignment.Start), - onClick = onAddNestedPack, - ) + if (inputFieldState.allowAddNestedPack) { + AddPackButton( + modifier = Modifier.align(Alignment.Start), + onClick = onAddNestedPack, + ) + } } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/common/IconPackEditor.kt` around lines 79 - 84, The empty-packs branch unconditionally renders AddPackButton causing allowAddNestedPack to be bypassed; update the branch that checks inputFieldState.nestedPacks.isEmpty() to only show the AddPackButton (and Spacer) when allowAddNestedPack is true (same guard used by the NestedPacks composable) so the onAddNestedPack action is only available when allowAddNestedPack permits it; locate the conditional around inputFieldState.nestedPacks.isEmpty() and wrap or gate the AddPackButton render with the allowAddNestedPack check.
🧹 Nitpick comments (1)
tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/newpack/ui/viewmodel/NewPackInputHandler.kt (1)
26-34: Consider extracting hardcoded Material pack constants.The package name, icon pack name, and nested pack names are inlined as string literals. A single companion-object or top-level constants block makes future changes (e.g. if the Material icons package ever moves) a one-line edit and eliminates typo risk.
♻️ Suggested refactor
+private const val MATERIAL_ICONS_PACKAGE = "androidx.compose.material.icons" +private const val MATERIAL_ICONS_PACK_NAME = "Icons" +private val MATERIAL_NESTED_PACKS = listOf("Filled", "Outlined", "Rounded", "TwoTone", "Sharp") // inside the Material Pack branch: - iconPackPackage = InputState(text = "androidx.compose.material.icons", enabled = false), - iconPackName = InputState(text = "Icons", enabled = false), + iconPackPackage = InputState(text = MATERIAL_ICONS_PACKAGE, enabled = false), + iconPackName = InputState(text = MATERIAL_ICONS_PACK_NAME, enabled = false), nestedPacks = listOf( - NestedPack(id = "0", inputFieldState = InputState(text = "Filled", enabled = false)), - NestedPack(id = "1", inputFieldState = InputState(text = "Outlined", enabled = false)), - NestedPack(id = "2", inputFieldState = InputState(text = "Rounded", enabled = false)), - NestedPack(id = "3", inputFieldState = InputState(text = "TwoTone", enabled = false)), - NestedPack(id = "4", inputFieldState = InputState(text = "Sharp", enabled = false)), + *MATERIAL_NESTED_PACKS.mapIndexed { i, name -> + NestedPack(id = "$i", inputFieldState = InputState(text = name, enabled = false)) + }.toTypedArray(), ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/newpack/ui/viewmodel/NewPackInputHandler.kt` around lines 26 - 34, Extract the hardcoded strings used to initialize iconPackPackage, iconPackName, and the nested nestedPacks display names into constants (either a companion object inside NewPackInputHandler or a top-level constants block) and replace the literals with those constants; e.g., define constants like MATERIAL_ICON_PACKAGE, MATERIAL_ICON_NAME and a list MATERIAL_NESTED_PACK_NAMES then initialize nestedPacks by mapping MATERIAL_NESTED_PACK_NAMES to NestedPack(id = index.toString(), inputFieldState = InputState(text = name, enabled = false)), keeping existing types InputState and NestedPack unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/jewel/settings/ExpandedGroup.kt`:
- Around line 31-38: ExpandedGroup currently always starts collapsed which hides
the toggle explaining why fields are locked when useMaterialPack is true; add a
new parameter initiallyExpanded: Boolean = false to ExpandedGroup and initialize
its remembered expanded state with that parameter (replace rememberMutableState
{ false } with rememberMutableState { initiallyExpanded }) so callers can
control initial visibility, then update the NewIconPackCreation /
NewPackCreation call site to pass state.useMaterialPack as the initiallyExpanded
argument.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/newpack/ui/viewmodel/NewPackViewModel.kt`:
- Around line 90-103: The current handling in NewPackViewModel for the
UseMaterialPack action unconditionally sets flatPackage to action.value, which
wipes a user-set flatPackage when disabling the material pack; change the logic
inside the UseMaterialPack branch so that when action.value is true you set
flatPackage = true (or keep current behavior), but when action.value is false
you do NOT overwrite flatPackage in inMemorySettings.update (i.e., only update
useMaterialPack = action.value and leave flatPackage unchanged), and mirror that
in the _state.updateState branch for PickedState (update copy(useMaterialPack =
action.value) without changing flatPackage) to preserve the prior user
preference.
In `@tools/idea-plugin/src/main/resources/messages/Valkyrie.properties`:
- Line 53: The property value for key
iconpack.newpack.creation.use.material.pack.description is grammatically
incorrect; update the string to a correct form such as "Will use the existing
pack from the \"androidx.compose.material.icons\" package" or "Uses the existing
pack from the \"androidx.compose.material.icons\" package" by replacing the
current value for iconpack.newpack.creation.use.material.pack.description in
Valkyrie.properties.
---
Outside diff comments:
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/common/IconPackEditor.kt`:
- Around line 79-84: The empty-packs branch unconditionally renders
AddPackButton causing allowAddNestedPack to be bypassed; update the branch that
checks inputFieldState.nestedPacks.isEmpty() to only show the AddPackButton (and
Spacer) when allowAddNestedPack is true (same guard used by the NestedPacks
composable) so the onAddNestedPack action is only available when
allowAddNestedPack permits it; locate the conditional around
inputFieldState.nestedPacks.isEmpty() and wrap or gate the AddPackButton render
with the allowAddNestedPack check.
---
Nitpick comments:
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/newpack/ui/viewmodel/NewPackInputHandler.kt`:
- Around line 26-34: Extract the hardcoded strings used to initialize
iconPackPackage, iconPackName, and the nested nestedPacks display names into
constants (either a companion object inside NewPackInputHandler or a top-level
constants block) and replace the literals with those constants; e.g., define
constants like MATERIAL_ICON_PACKAGE, MATERIAL_ICON_NAME and a list
MATERIAL_NESTED_PACK_NAMES then initialize nestedPacks by mapping
MATERIAL_NESTED_PACK_NAMES to NestedPack(id = index.toString(), inputFieldState
= InputState(text = name, enabled = false)), keeping existing types InputState
and NestedPack unchanged.
.../idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/jewel/settings/ExpandedGroup.kt
Show resolved
Hide resolved
Comment on lines
+90
to
+103
| is UseMaterialPack -> { | ||
| inMemorySettings.update { | ||
| useMaterialPack = action.value | ||
| flatPackage = action.value | ||
| } | ||
|
|
||
| _state.updateState { | ||
| if (this is PickedState) { | ||
| copy(useMaterialPack = action.value) | ||
| } else { | ||
| this | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
flatPackage preference silently overwritten when disabling material pack
When action.value = false, flatPackage is unconditionally reset to false. If the user had previously set flatPackage = true independently of the material pack toggle, that preference is destroyed without recourse.
Consider preserving the pre-existing flatPackage value when disabling material pack, or only setting flatPackage = true on enable and leaving it untouched on disable:
♻️ Proposed fix
is UseMaterialPack -> {
inMemorySettings.update {
useMaterialPack = action.value
- flatPackage = action.value
+ if (action.value) {
+ flatPackage = true
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@tools/idea-plugin/src/main/kotlin/io/github/composegears/valkyrie/ui/screen/mode/iconpack/newpack/ui/viewmodel/NewPackViewModel.kt`
around lines 90 - 103, The current handling in NewPackViewModel for the
UseMaterialPack action unconditionally sets flatPackage to action.value, which
wipes a user-set flatPackage when disabling the material pack; change the logic
inside the UseMaterialPack branch so that when action.value is true you set
flatPackage = true (or keep current behavior), but when action.value is false
you do NOT overwrite flatPackage in inMemorySettings.update (i.e., only update
useMaterialPack = action.value and leave flatPackage unchanged), and mirror that
in the _state.updateState branch for PickedState (update copy(useMaterialPack =
action.value) without changing flatPackage) to preserve the prior user
preference.
| iconpack.newpack.choose.directory.continue=Continue | ||
| iconpack.newpack.creation.additional.options=Additional options | ||
| iconpack.newpack.creation.use.material.pack=Use Material Iconpack | ||
| iconpack.newpack.creation.use.material.pack.description=Will be used existing pack from "androidx.compose.material.icons" package |
There was a problem hiding this comment.
Grammar issue in the description string.
"Will be used existing pack from..." is grammatically incorrect. Consider: "Will use the existing pack from..." or "Uses the existing pack from...".
📝 Suggested fix
-iconpack.newpack.creation.use.material.pack.description=Will be used existing pack from "androidx.compose.material.icons" package
+iconpack.newpack.creation.use.material.pack.description=Will use the existing pack from "androidx.compose.material.icons" package📝 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
| iconpack.newpack.creation.use.material.pack.description=Will be used existing pack from "androidx.compose.material.icons" package | |
| iconpack.newpack.creation.use.material.pack.description=Will use the existing pack from "androidx.compose.material.icons" package |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/idea-plugin/src/main/resources/messages/Valkyrie.properties` at line
53, The property value for key
iconpack.newpack.creation.use.material.pack.description is grammatically
incorrect; update the string to a correct form such as "Will use the existing
pack from the \"androidx.compose.material.icons\" package" or "Uses the existing
pack from the \"androidx.compose.material.icons\" package" by replacing the
current value for iconpack.newpack.creation.use.material.pack.description in
Valkyrie.properties.
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.
This reverts commit 597ba36
📝 Changelog
If this PR introduces user-facing changes, please update the relevant Unreleased section in changelogs: