-
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
base: develop
Are you sure you want to change the base?
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.cpp
src/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%n
placeholder, 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
%n
for both success and failure counts- Linear message assembly using
+=
operator- More explicit condition checking (
nSuccessful > 0
instead 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.
Issue being fixed or feature implemented
%n
where possibleWhat was done?
How Has This Been Tested?
Breaking Changes
n/a
Checklist: