Make int input validator locale aware#64
Conversation
📝 WalkthroughWalkthroughThis pull request adds locale-aware integer input validation to the IntInputValidator class. The validate() method now recognizes and handles thousands separators based on the current QLocale, strips them before regex validation, and immediately returns Invalid for out-of-range values. The fixup() method reformats input using QLocale::toString to ensure consistent group separator formatting. A new test suite validates both en_US (comma) and ro_RO (dot) locale behaviors with comprehensive test cases covering valid inputs, boundaries, and fixup transformations. 🚥 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 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: 2
🤖 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
`@framework/uicomponents/qml/Muse/UiComponents/tests/intinputvalidator_tests.cpp`:
- Around line 89-106: Add regression test cases in the intinputvalidator_tests
by extending the runInputTests calls to cover: (1) scenarios where the
validator's bottom bound > 0 and the input is "0" or "" to ensure fixup()
produces the bottom value (e.g., input "0" -> "1" when bottom==1) and validate()
returns Intermediate/Acceptable appropriately, and (2) near- and over-int-limit
cases (INT_MAX, INT_MAX+1, INT_MIN, INT_MIN-1) to assert validate() returns
Invalid for overflowed values and fixup() clamps to INT_MAX/INT_MIN where
appropriate; reference the existing test helper runInputTests and the
validator's validate()/fixup() behavior when adding these new entries similar to
the existing cases around the "0", "", and large-number examples.
In
`@framework/uicomponents/qml/Muse/UiComponents/validators/intinputvalidator.cpp`:
- Around line 46-48: In fixup(), remove the special-case that returns early when
val == 0 so zero is not bypassing clamping; change the logic that currently does
"if (!ok || val == 0) { string = "0"; return; }" to only treat parse failures
(e.g., "if (!ok) { ... }") and then run the existing clamp logic on val (using
the existing bottom/top bounds) before setting string so a parsed zero is
clamped into-range instead of returned as "0".
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2fba5062-e933-4eae-9300-1da812d3e1d5
📒 Files selected for processing (3)
framework/uicomponents/qml/Muse/UiComponents/tests/CMakeLists.txtframework/uicomponents/qml/Muse/UiComponents/tests/intinputvalidator_tests.cppframework/uicomponents/qml/Muse/UiComponents/validators/intinputvalidator.cpp
embarc-gabriel
changed the title
embarc-gabriel
force-pushed
the
make-input-validator-local-aware
branch
from
ffda1ae to
64e137c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@framework/uicomponents/qml/Muse/UiComponents/validators/intinputvalidator.cpp`:
- Around line 100-102: The current check in IntInputValidator that returns
Invalid when val > m_top || val < m_bottom is too strict; modify the validate
logic in IntInputValidator (the validate method that uses m_top and m_bottom) so
that you only return Invalid when the numeric input cannot possibly be completed
into [m_bottom, m_top] (e.g., value > m_top, or value < m_bottom AND the current
text cannot be extended/changed to reach m_bottom), otherwise return
Intermediate for in-progress prefixes; specifically, keep returning Invalid for
val > m_top, but for val < m_bottom detect prefix/partial cases (empty string,
just a sign, or a shorter numeric string that could grow by adding digits or a
sign to reach m_bottom) and return Intermediate for those, only returning
Invalid when the text is a complete number that is definitively below m_bottom
and cannot be changed by further typing into the same field to become valid.
Ensure you reference and use m_top and m_bottom and update the validate(...)
branch that currently contains if (val > m_top || val < m_bottom) { return
Invalid; } accordingly.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 96df75d8-71d5-4790-9f62-4e91d14fee90
📒 Files selected for processing (3)
framework/uicomponents/qml/Muse/UiComponents/tests/CMakeLists.txtframework/uicomponents/qml/Muse/UiComponents/tests/intinputvalidator_tests.cppframework/uicomponents/qml/Muse/UiComponents/validators/intinputvalidator.cpp
| if (val > m_top || val < m_bottom) { | ||
| return Invalid; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Usages of IntInputValidator and the bottom/top bounds configured around them
rg -nP -C3 'IntInputValidator' --iglob '*.qml' --iglob '*.cpp' --iglob '*.h'
# QML bottom bindings (likely on the validator instance)
rg -nP -C2 '\bbottom\s*:' --iglob '*.qml'
# C++ setBottom calls with their argument
rg -nP -C1 '\bsetBottom\s*\(' --iglob '*.cpp' --iglob '*.h'Repository: musescore/muse_framework
Length of output: 50380
Fix IntInputValidator below-bottom behavior (don’t blanket-return Invalid).
IntInputValidator is used by IncrementalPropertyControl.qml with bottom: root.minValue, so real consumers can supply m_bottom > 1 (not just the bottom=1 case the unit tests cover). Returning QValidator::Invalid when val < m_bottom rejects valid in-progress prefixes that could still reach the allowed range, which should stay Intermediate instead.
Update the logic around the if (val > m_top || val < m_bottom) { return Invalid; } check to only return Invalid for inputs that can’t be completed into the range (and keep Intermediate when the partial value could still grow into [m_bottom, m_top], matching the behavior expected by QIntValidator/tests for bottom=1).
🤖 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
`@framework/uicomponents/qml/Muse/UiComponents/validators/intinputvalidator.cpp`
around lines 100 - 102, The current check in IntInputValidator that returns
Invalid when val > m_top || val < m_bottom is too strict; modify the validate
logic in IntInputValidator (the validate method that uses m_top and m_bottom) so
that you only return Invalid when the numeric input cannot possibly be completed
into [m_bottom, m_top] (e.g., value > m_top, or value < m_bottom AND the current
text cannot be extended/changed to reach m_bottom), otherwise return
Intermediate for in-progress prefixes; specifically, keep returning Invalid for
val > m_top, but for val < m_bottom detect prefix/partial cases (empty string,
just a sign, or a shorter numeric string that could grow by adding digits or a
sign to reach m_bottom) and return Intermediate for those, only returning
Invalid when the text is a complete number that is definitively below m_bottom
and cannot be changed by further typing into the same field to become valid.
Ensure you reference and use m_top and m_bottom and update the validate(...)
branch that currently contains if (val > m_top || val < m_bottom) { return
Invalid; } accordingly.
64e137c to
b2dbdef
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
framework/uicomponents/qml/Muse/UiComponents/tests/intinputvalidator_tests.cpp (1)
89-117: 🧹 Nitpick | 🔵 Trivial | 🏗️ Heavy liftThe previous review comment requesting additional regression cases is still unaddressed.
The test suite does not verify
fixup()behavior for out-of-range inputs:
Bottom-bound edge case (lines 113–117): When
bottom = 1, inputs"0"and""are markedInvalid, sofixup()is never called (due to the skip at line 69–71). The test should verify thatfixup()clamps these to"1".Integer overflow edge case (lines 104–105):
INT_MAXandINT_MINare tested forInvalidstate, but there's no verification thatfixup()clamps overflowed values to the validator's[bottom, top]range.Consider adding test cases where the expected state is
IntermediateorAcceptablefor boundary inputs, allowingfixup()to be exercised and its clamping behavior verified.🤖 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 `@framework/uicomponents/qml/Muse/UiComponents/tests/intinputvalidator_tests.cpp` around lines 89 - 117, The tests currently never exercise UiIntInputValidator::fixup for out-of-range inputs; update the test cases passed to runInputTests so at least one out-of-range input yields Intermediate (or Acceptable) rather than Invalid, then call fixup() and assert the edited string is clamped to the set range; specifically add cases after m_validator->setBottom(1)/setTop(10) where input "0" and "" are marked Intermediate and expect fixup to produce "1" (use runInputTests or a dedicated call to m_validator->fixup on the QString and check the result), and add cases for INT_MAX/INT_MIN (e.g., "2147483647" and "-2147483648") that produce Intermediate so fixup clamps them into [bottom, top]; reference runInputTests, m_validator->setTop, m_validator->setBottom, and fixup when locating places to add these assertions.
🤖 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.
Duplicate comments:
In
`@framework/uicomponents/qml/Muse/UiComponents/tests/intinputvalidator_tests.cpp`:
- Around line 89-117: The tests currently never exercise
UiIntInputValidator::fixup for out-of-range inputs; update the test cases passed
to runInputTests so at least one out-of-range input yields Intermediate (or
Acceptable) rather than Invalid, then call fixup() and assert the edited string
is clamped to the set range; specifically add cases after
m_validator->setBottom(1)/setTop(10) where input "0" and "" are marked
Intermediate and expect fixup to produce "1" (use runInputTests or a dedicated
call to m_validator->fixup on the QString and check the result), and add cases
for INT_MAX/INT_MIN (e.g., "2147483647" and "-2147483648") that produce
Intermediate so fixup clamps them into [bottom, top]; reference runInputTests,
m_validator->setTop, m_validator->setBottom, and fixup when locating places to
add these assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 36431f2d-ae47-4633-867e-9c398a55fa5d
📒 Files selected for processing (2)
framework/uicomponents/qml/Muse/UiComponents/tests/CMakeLists.txtframework/uicomponents/qml/Muse/UiComponents/tests/intinputvalidator_tests.cpp
2 participants
Resolves: audacity/audacity#9949
Modify input validator to take group separator into account.
Build configuration
audacity: audacity/audacity/master
audacity platforms: linux_x64
musescore: musescore/MuseScore/master
musescore platforms: linux_x64