ADFA-3874 | Fix radio group ID generation and refactor widget rendering#1271
Open
ADFA-3874 | Fix radio group ID generation and refactor widget rendering#1271
Conversation
Refactor AndroidWidget to support nested view groups and add grammars for compound buttons.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 Walkthrough
Risks & Best-practice violations:
WalkthroughThis PR refactors the Android UI rendering pipeline: introduces a stateful AndroidWidget hierarchy and container widgets, centralizes LayoutItem→AndroidWidget conversion in WidgetFactory, introduces Android XML constants, restructures grammar/attribute validators with inheritance, and updates text/dimension cleaning logic. ChangesUnified Widget & Grammar Refactoring
Sequence DiagramsequenceDiagram
participant LR as LayoutRenderer
participant WF as WidgetFactory
participant AW as AndroidWidget
participant XC as XmlContext
LR->>WF: createWidgets(item)
alt SimpleView
WF->>AW: createSimpleWidget(box)
AW->>XC: resolveId()/assembleAttributes()
else HorizontalRow
WF->>WF: createHorizontalRow(boxes)
loop per child
WF->>AW: createSimpleWidget(box)
AW->>AW: assembleAttributes()
end
WF->>AW: HorizontalRowWidget(children)
else RadioGroup/CheckboxGroup
WF->>WF: createRadioGroup/createCheckboxGroup(boxes)
WF->>XC: resolveRadioGroupId()/resolveBaseId()
loop per child
WF->>AW: createSimpleWidget(child)
AW->>AW: apply group attrs / set android:checked
end
WF->>AW: RadioGroupWidget(children, checkedId)
end
WF-->>LR: List<AndroidWidget>
loop render each widget
LR->>AW: render(context, indent)
AW->>AW: assembleAttributes()
AW->>AW: getChildren()
alt has children
AW->>AW: render child.render()
else leaf
AW->>AW: self-close tag
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 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. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/utils/TextCleaner.kt (1)
27-27: ⚡ Quick winReuse
leadingMarkerRegexhere to eliminate the duplicated inline literal.The inline pattern on this line is byte-for-byte identical to
leadingMarkerRegexintroduced in this same PR. Leaving it inlined means a future change toleadingMarkerRegexsilently diverges fromcleanTextPreservingLeadingO.♻️ Proposed fix
- .replace(Regex("^[\\[\\]()●○□☑✓-]+\\s*"), "") + .replace(leadingMarkerRegex, "")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/utils/TextCleaner.kt` at line 27, The inline Regex pattern ".replace(Regex(\"^[\\\\[\\\\]()●○□☑✓-]+\\\\s*\"), \"\")" in cleanTextPreservingLeadingO duplicates the existing leadingMarkerRegex; replace the inline Regex call with a reuse of the leadingMarkerRegex constant (e.g., .replace(leadingMarkerRegex, "")) so future changes to leadingMarkerRegex apply consistently across the function.cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/xml/WidgetFactory.kt (1)
119-125: 💤 Low valueConsider skipping zero-gap margins.
getMarginEndForHorizontalGapalways returns a margin attribute, even when the gap is 0. Emittingandroid:layout_marginEnd="0dp"is harmless but adds noise to the generated XML.Skip zero margins
private fun getMarginEndForHorizontalGap(boxes: List<ScaledBox>, currentIndex: Int): Map<String, String> { if (currentIndex >= boxes.lastIndex) return emptyMap() val currentBox = boxes[currentIndex] val nextBox = boxes[currentIndex + 1] val gap = maxOf(0, nextBox.x - (currentBox.x + currentBox.w)) + if (gap == 0) return emptyMap() return mapOf("android:layout_marginEnd" to "${gap}dp") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/xml/WidgetFactory.kt` around lines 119 - 125, The getMarginEndForHorizontalGap function always emits an android:layout_marginEnd attribute even when gap is 0; change the logic so after computing gap = maxOf(0, nextBox.x - (currentBox.x + currentBox.w)) you return emptyMap() when gap == 0 and only return mapOf("android:layout_marginEnd" to "${gap}dp") when gap > 0, keeping the early bounds check on currentIndex and using the existing variables (boxes, currentIndex, currentBox, nextBox, gap) to locate where to add this conditional.cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/xml/AndroidWidget.kt (1)
11-12: 💤 Low valueMutable state on sealed class instances.
Using
varproperties (idOverride,extraAttrs) makes widgets stateful. This works if widgets are created fresh per render (which appears to be the case inWidgetFactory), but could cause issues if widgets are cached or re-rendered.This is acceptable given the current usage pattern, but worth documenting as an invariant.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/xml/AndroidWidget.kt` around lines 11 - 12, The sealed class AndroidWidget exposes mutable properties idOverride and extraAttrs which makes instances stateful; either make these properties immutable (convert to constructor params or vals) or, if you must keep them mutable, add a clear documentation comment on AndroidWidget explaining the invariant that WidgetFactory must create fresh widget instances for each render and widgets must never be cached/reused; alternatively provide an explicit copy/withIdOverride/withExtraAttrs factory method on AndroidWidget so callers create modified copies instead of mutating idOverride/extraAttrs directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/grammar/AttributeValidator.kt`:
- Around line 39-51: SpDimensionRangeValidator currently only accepts integer
SPs because spRegex is "^(\\d+)sp$" and validate uses toIntOrNull; change the
regex (spRegex) to accept optional decimal parts (e.g.
"^(\\d+(?:\\.\\d+)?)sp$"), parse the captured group with toDoubleOrNull instead
of toIntOrNull, and compare the parsed Double against the minSp/maxSp bounds by
converting them to Double (e.g. value in minSp.toDouble()..maxSp.toDouble());
return trimmed when in range and null otherwise, leaving the class name and
validate method intact.
In
`@cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/xml/WidgetFactory.kt`:
- Around line 127-137: The normalization in resolveRadioGroupId is too broad
(the second when-branch uses startsWith("r") && contains("grou")) and will catch
unrelated IDs; tighten it by requiring the id to start with "rb" (or match a
stricter regex) before mapping to "rb_group" — e.g., replace the condition with
normalizedId.startsWith("rb") && normalizedId.contains("grou") or use a regex
like normalizedId.matches("^rb[_-]?grou.*") so only intended IDs are normalized
to "rb_group" while leaving other values untouched; keep the first branch for
"radio_grou" -> "radio_group" and return via context.resolveId as before.
---
Nitpick comments:
In
`@cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/xml/AndroidWidget.kt`:
- Around line 11-12: The sealed class AndroidWidget exposes mutable properties
idOverride and extraAttrs which makes instances stateful; either make these
properties immutable (convert to constructor params or vals) or, if you must
keep them mutable, add a clear documentation comment on AndroidWidget explaining
the invariant that WidgetFactory must create fresh widget instances for each
render and widgets must never be cached/reused; alternatively provide an
explicit copy/withIdOverride/withExtraAttrs factory method on AndroidWidget so
callers create modified copies instead of mutating idOverride/extraAttrs
directly.
In
`@cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/xml/WidgetFactory.kt`:
- Around line 119-125: The getMarginEndForHorizontalGap function always emits an
android:layout_marginEnd attribute even when gap is 0; change the logic so after
computing gap = maxOf(0, nextBox.x - (currentBox.x + currentBox.w)) you return
emptyMap() when gap == 0 and only return mapOf("android:layout_marginEnd" to
"${gap}dp") when gap > 0, keeping the early bounds check on currentIndex and
using the existing variables (boxes, currentIndex, currentBox, nextBox, gap) to
locate where to add this conditional.
In
`@cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/utils/TextCleaner.kt`:
- Line 27: The inline Regex pattern
".replace(Regex(\"^[\\\\[\\\\]()●○□☑✓-]+\\\\s*\"), \"\")" in
cleanTextPreservingLeadingO duplicates the existing leadingMarkerRegex; replace
the inline Regex call with a reuse of the leadingMarkerRegex constant (e.g.,
.replace(leadingMarkerRegex, "")) so future changes to leadingMarkerRegex apply
consistently across the function.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 47cb9a56-13af-41fd-a864-dfa247fffce3
📒 Files selected for processing (9)
cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/FuzzyAttributeParser.ktcv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/grammar/AttributeValidator.ktcv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/grammar/UiGrammarValidator.ktcv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/grammar/WidgetGrammar.ktcv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/xml/AndroidConstants.ktcv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/xml/AndroidWidget.ktcv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/xml/LayoutRenderer.ktcv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/domain/xml/WidgetFactory.ktcv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/utils/TextCleaner.kt
Daniel-ADFA
approved these changes
May 5, 2026
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
2 participants
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 PR fixes the issue where individual widgets within a radio button group generated incorrect IDs (e.g.,
rb_group_textste_l6sp). To solve this and improve layout rendering, the widget creation logic has been extracted fromLayoutRendererinto a dedicatedWidgetFactory.Additionally, this PR includes:
AndroidWidgetto support child rendering viaAndroidViewGroup, making it easier to render composite layouts likeRadioGroupandLinearLayout(viaHorizontalRowWidget).AndroidConstantsandAndroidWidgetTagsto centralize standard Android XML strings and widget names.WidgetGrammarinto a robust interface hierarchy (LayoutGrammar,TextGrammar,CompoundButtonGrammar) and added grammars forRadioButton,CheckBox,Switch, andRadioGroup.BooleanValidatorandSpDimensionRangeValidatorfor stricter attribute validation.TextCleanerwith targeted regexes to correctly strip leading standalone circles and duplicated marker artifacts, and fixed dimension parsing to accurately extractspvalues.Details
LayoutRendererhas been vastly simplified and now delegates creation toWidgetFactory. The rendering pipeline insideAndroidWidgetwas split intoresolveWidgetId,assembleAttributes, andwriteXmlto safely handle hierarchical attributes and default dimensions. View group attributes (likeandroid:checkedButtonand orientation margins) are now evaluated directly inside the new factory.document_5172673150319593366.mp4
Ticket
ADFA-3874
Observation
The shift to an object-oriented composite pattern for widgets (
AndroidViewGroup) and the introduction ofAndroidConstantssignificantly cleans up the XML generation pipeline. This paves the way for adding more complex nested layouts (like ConstraintLayout or complex cards) in the future without bloating the renderer.