-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: improve governance/proposal dialog strings #6885
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- make strings easier to translate - stop translating non-translatable strings - use `%n` where possible
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughThis change updates UI and message strings to improve translation handling and simplifies result message assembly. In the UI .ui file, several static strings and placeholders are annotated with notr="true". In proposal creation, plural-aware formatting is used for ETA minutes and combo item text generation is adjusted. In governance voting, the result summary is refactored to linearly build a message that conditionally includes success counts, failure counts, and error details. No new UI elements, business logic, or public interfaces are introduced. Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/qt/forms/proposalwizard.ui (1)
430-430: Consider whether "TxID:" should be translatable.The label
"TxID:"is marked withnotr="true". While technical terms are often kept in English, some translation systems prefer to localize technical UI labels for consistency with the rest of the interface. This is a design decision.If you decide "TxID:" should be translatable, remove the
notr="true"attribute:-<string notr="true">TxID:</string> +<string>TxID:</string>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/qt/forms/proposalwizard.ui(7 hunks)src/qt/governancelist.cpp(1 hunks)src/qt/proposalwizard.cpp(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/qt/proposalwizard.cppsrc/qt/governancelist.cpp
🔇 Additional comments (7)
src/qt/proposalwizard.cpp (3)
63-63: LGTM! Numbers should not be translated.The change from
tr("%1").arg(i)toQString().setNum(i)correctly removes translation wrapping from numeric values, which should never be translated.
293-294: LGTM! Proper Qt pluralization.The change to
tr("Estimated time remaining: %n minute(s)", "", mins)correctly uses Qt's plural-aware formatting with the%nplaceholder, enabling proper translations for different plural forms across languages.
324-325: LGTM! Correctly separates translatable from non-translatable text.The change separates the translatable success message from the technical ID by concatenating
tr("Your proposal was submitted successfully.")withQString("\nID: %1").arg(govId). This prevents the ID from being included in translation strings where it doesn't belong.src/qt/forms/proposalwizard.ui (3)
113-113: LGTM! Example placeholders should not be translated.Correctly marking placeholder text like
"short-unique-name"and"https://example.com/my-proposal"withnotr="true"as these are examples that should remain consistent across all languages.Also applies to: 139-139
258-258: LGTM! Numbers should not be translated.Correctly marking the numeric value
"0"withnotr="true"as numbers should not be translated.
345-345: LGTM! Placeholder indicators should not be translated.Correctly marking the placeholder indicator
"-"withnotr="true"as these are UI placeholders that should remain consistent across languages.Also applies to: 440-440, 664-664
src/qt/governancelist.cpp (1)
598-609: LGTM! Proper pluralization and clearer message construction.The changes correctly implement:
- Qt's plural-aware formatting with
%nfor both success and failure counts- Linear message assembly using
+=operator- More explicit condition checking (
nSuccessful > 0instead of!message.isEmpty())- Proper separation of translatable text ("Errors:") from non-translatable content (error details)
This enables proper translations for different plural forms across languages.
knst
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM af85608
af85608 fix: improve governance/proposal dialog strings (UdjinM6) Pull request description: ## Issue being fixed or feature implemented - make strings easier to translate - stop translating non-translatable strings - use `%n` where possible ## What was done? ## How Has This Been Tested? ## Breaking Changes n/a ## Checklist: - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ Top commit has no ACKs. Tree-SHA512: bba2247fc64f152656972d7e30c12eea6d892a622751c8e798a0ccb522c297356b1ae3964d9db25aa1c920eb71d862e026b0a490ddce784563aea9caa5445425
6fd7059 chore: mark v23 as release (pasta) ae08f53 docs: integrate 6946 release notes into final (pasta) 74a222d Merge #6946: feat: show seed on wallet creation (pasta) 877343a Merge #6943: fix: don't treat arrays/objects as string literals for composite methods (pasta) 00368fb Merge #6940: fix: reuse best clsig to avoid potential race condition (pasta) 8eceb98 Merge #6938: fix: logic error in `CheckDecryptionKey` (pasta) 3f30664 Merge #6929: fix: repair `makeseeds.py`, `getblockchaininfo[softforks]` help text, drop extra `generate`s from test, resolve macOS GID issue (pasta) 7ba4f1c Merge #6928: docs: update man pages for 23.0 (pasta) a6c1d6a Merge #6920: chore: update minimum chain work, tx stats, checkpoints, seeds and SECURITY.md for v23 (pasta) 84df1f0 Merge #6909: chore: Translations 2025-10 (pasta) a6449b1 Merge #6885: fix: improve governance/proposal dialog strings (pasta) Pull request description: ## Issue being fixed or feature implemented See commits ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 6fd7059 Tree-SHA512: a0d93a61a4f4978fbe120bea832ce683a8ae7c16c892a381d91ddc4b25344c0f3ad3306a1a30a166a7dfa6e38e4532708587cc23cc372126828b8e22d141dc85
fe1cff3 chore: bump release to 23.0.2 (pasta) a8f15c1 Merge #7032: fix: drop gsl usage from RebuildListFromBlock function wrapper (pasta) 736bb26 chore: bump manpages for 23.0.1 (pasta) d5c7d25 chore: bump nMinimumChainWork and defaultAssumeValid (pasta) 4f8aa71 chore: bump version to 23.0.1 (pasta) 0865b7c docs: add release notes for 23.0.1 (pasta) 2048b42 Merge #6986: test: new commandline argument -tinyblk to use blk size just 64kb instead 16Mb (pasta) 1a9b20c Merge #7013: fix: update BuildTestVectors call to adjust batch size based on output flag (pasta) 36e4679 Merge #7009: fix: include QDebug directly (pasta) 69d0c9c Merge #6999: feat: verify and repair evodb diffs automatically at node startup (pasta) ca16437 Merge #6996: perf: reduce cs_main lock scope in evodb verify/repair operations (pasta) 207526e Merge #6977: fix: bls benchmarks crash when ran independently (pasta) 226aaf4 Merge #6969: feat: add evodb verify and repair RPC commands (pasta) 92abe9b Merge #6964: perf: remove duplicated check of same key in the instant send database (pasta) 5a1ec4c Merge #6961: fix: correct BLS scheme setting in `MigrateLegacyDiffs()` when `nVersion` is present (pasta) bf653d3 Merge #6949: depends: Qt 5.15.18 (pasta) faf58cd merge bitcoin#30774: Qt 5.15.16 (Kittywhiskers Van Gogh) 6a995f5 Merge #6944: fix: HD chain encryption check ordering issue (pasta) 6fd7059 chore: mark v23 as release (pasta) ae08f53 docs: integrate 6946 release notes into final (pasta) 74a222d Merge #6946: feat: show seed on wallet creation (pasta) 877343a Merge #6943: fix: don't treat arrays/objects as string literals for composite methods (pasta) 00368fb Merge #6940: fix: reuse best clsig to avoid potential race condition (pasta) 8eceb98 Merge #6938: fix: logic error in `CheckDecryptionKey` (pasta) 3f30664 Merge #6929: fix: repair `makeseeds.py`, `getblockchaininfo[softforks]` help text, drop extra `generate`s from test, resolve macOS GID issue (pasta) 7ba4f1c Merge #6928: docs: update man pages for 23.0 (pasta) a6c1d6a Merge #6920: chore: update minimum chain work, tx stats, checkpoints, seeds and SECURITY.md for v23 (pasta) 84df1f0 Merge #6909: chore: Translations 2025-10 (pasta) a6449b1 Merge #6885: fix: improve governance/proposal dialog strings (pasta) ebf3a64 docs: typos (pasta) 4ad5533 docs: typos (pasta) f407453 doc: Replace Bitcoin Core PR references with Dash Core backport PRs (pasta) 78d0725 docs: add note on proto bump and platformban p2p (pasta) e0519c3 docs: fix whitespace errors (pasta) bc8db22 docs: minor improvements to release notes (pasta) c338511 docs: reorganize rpc updates to organize extended address changes (thephez) 700c46e style: make heading style consistent (thephez) bd636bd docs: add contributors (pasta) 6d29bc3 docs: revert changes deferred to v24 (pasta) 615f5ff docs: make the downgrade warning more confident (pasta) 567771a Apply suggestions from code review (PastaPastaPasta) 2b3211a docs: add link to 22.1.3 release notes (pasta) 548a38a docs: remove individual release notes files (pasta) e770c25 docs: add v23.0.0 release notes and archive v22.1.3 (pasta) Pull request description: ## Issue being fixed or feature implemented Includes changes from 23.0.0 release too because we never merged it back. ## What was done? ## How Has This Been Tested? ## Breaking Changes ## Checklist: - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK 491db4a kwvg: utACK 491db4a Tree-SHA512: 61895cd1f9d01ac7be1d407afe1ddd24b98e8242cb03229ecd586a4d2d1c43dbc62c98da52a8c715b3a5943fb40e99b23251e691f778779af9d6da94392122a3
Issue being fixed or feature implemented
%nwhere possibleWhat was done?
How Has This Been Tested?
Breaking Changes
n/a
Checklist: