Skip to content

fix: omit group separator in QmlDataFormatter to prevent spinbox validator rejection#33120

Open
kaaviyab77 wants to merge 1 commit intomusescore:masterfrom
kaaviyab77:large-percentages-ui
Open

fix: omit group separator in QmlDataFormatter to prevent spinbox validator rejection#33120
kaaviyab77 wants to merge 1 commit intomusescore:masterfrom
kaaviyab77:large-percentages-ui

Conversation

@kaaviyab77
Copy link
Copy Markdown

Resolves: #32960

Percentage spinboxes in the Properties panel (e.g. fermata time stretch, measure width) were snapping to their minimum value when entering numbers ≥ 1000. Fixed by preventing the number formatter from adding thousands separators (e.g. "1,000"), which the input field's validator did not recognize as a valid number.

  • I signed the CLA
  • The title of the PR describes the problem it addresses
  • Each commit's message describes its purpose and effects, and references the issue it resolves
  • If changes are extensive, there is a sequence of easily reviewable commits
  • The code in the PR follows the coding rules
  • There are no unnecessary changes
  • The code compiles and runs on my machine, preferably after each commit individually
  • I created a unit test or vtest to verify the changes I made (if applicable)

Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

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

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 24, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 87f43cba-00f5-45f0-b9b8-7b82b4197aba

📥 Commits

Reviewing files that changed from the base of the PR and between 7e4c067 and 2aab53f.

📒 Files selected for processing (1)
  • src/framework/ui/view/qmldataformatter.cpp

📝 Walkthrough

Walkthrough

The formatReal function in qmldataformatter.cpp was modified to configure its QLocale instance to omit group separators before converting numeric values to fixed-point strings. The existing logic for trimming trailing zeros and decimal separators remains unchanged, but now operates on formatted strings that exclude thousands separators.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: omitting group separators in QmlDataFormatter to fix spinbox validator rejection.
Description check ✅ Passed The description includes the resolved issue (#32960), clear motivation, and all required CLA and checklist items are marked complete.
Linked Issues check ✅ Passed The PR resolves #32960 by removing group separators from formatted numbers so validators accept large numeric inputs for percentage spinboxes.
Out of Scope Changes check ✅ Passed The single-line change to formatReal() is directly scoped to fixing the group separator issue in #32960.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@mathesoncalum mathesoncalum left a comment

Choose a reason for hiding this comment

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

Is there an argument that we should approach this from a different angle by keeping the group separator and making sure that the input valiator can handle it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Percentages displaying inaccurately w/ large numbers

3 participants