feat(qt): add global zoom shortcuts - #7216
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00bdcf9910
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const int current_scale{GUIUtil::g_font_registry.GetFontScale()}; | ||
| const int new_scale{delta == 0 | ||
| ? GUIUtil::FontRegistry::DEFAULT_FONT_SCALE | ||
| : std::clamp(current_scale + delta, FONT_SCALE_MIN, FONT_SCALE_MAX)}; |
There was a problem hiding this comment.
adjustFontScale() clamps every non-reset zoom change to [-30, 30], but startup accepts -font-scale values up to [-100, 100] (src/qt/bitcoin.cpp), so users who run with a configured scale outside [-30, 30] will see Ctrl++ or Ctrl+- snap the scale directly to 30/-30 instead of adjusting from their current value. This is a regression introduced by the shortcut path and can unexpectedly shrink/enlarge UI text and persist the wrong value.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
should be resolved
| return; | ||
| } | ||
|
|
||
| GUIUtil::g_font_registry.SetFontScale(new_scale); |
There was a problem hiding this comment.
The shortcut handler updates g_font_registry unconditionally, so Ctrl++/Ctrl+-/Ctrl+0 can still change live font scale even when -font-scale is CLI-overridden; this bypasses the existing override behavior where the Appearance slider is disabled (AppearanceWidget::setModel, isOptionOverridden("-font-scale")). In deployments that pin font scale via config/args, the new shortcuts now violate that expectation and can also write conflicting GUI settings.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
should be resolved
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds global font scaling to the Qt GUI: introduces FONT_SCALE_MIN / FONT_SCALE_MAX / FONT_SCALE_SHORTCUT_STEP and three View actions (Zoom In, Zoom Out, Reset) wired to a new private BitcoinGUI::adjustFontScale(int delta) that clamps, applies, refreshes, and persists font scale when allowed. Expands appearance widget slider range. Updates GUIUtil::AddButtonShortcut to accept an explicit Qt::ShortcutContext (defaulting to WindowShortcut) and registers font-size shortcuts in RPCConsole using the new helper to avoid duplicate shortcuts on theme changes. Sequence DiagramsequenceDiagram
actor User
participant BitcoinGUI
participant GUIUtil
participant OptionsModel
User->>BitcoinGUI: Trigger Zoom In / Zoom Out / Reset
BitcoinGUI->>BitcoinGUI: adjustFontScale(delta)\ncompute & clamp new scale
BitcoinGUI->>GUIUtil: set global font scale / refresh fonts
GUIUtil->>GUIUtil: update font registry\napply fonts across UI
alt OptionsModel available
BitcoinGUI->>OptionsModel: save new font scale
OptionsModel-->>BitcoinGUI: acknowledge
end
GUIUtil-->>User: UI updated with new font scale
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
Seems to be working as expected but
|
00bdcf9 to
04cb014
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04cb01468e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/qt/bitcoingui.cpp`:
- Around line 529-531: The zoom-out shortcut was set to "Ctrl+_" but should be
"Ctrl+-"; update the shortcut string passed to m_zoom_out_action->setShortcuts
to QKeySequence(tr("Ctrl+-")) so it matches QKeySequence::ZoomOut, and make the
same change wherever the alternative shortcut string is used (e.g., the rpc
console's QAction/QKeySequence setup) so both m_zoom_out_action and the rpc
console shortcut use "Ctrl+-" instead of "Ctrl+_".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 741c0042-bcc9-4234-a35f-f11a73b96fbb
📥 Commits
Reviewing files that changed from the base of the PR and between 00bdcf991097a8d73320a92bc0607ebe08b1201c and 04cb01468e99d09514a17603c102bb826eb01aab.
📒 Files selected for processing (3)
CLAUDE.mdsrc/qt/bitcoingui.cppsrc/qt/bitcoingui.h
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/qt/bitcoingui.h
036cbdb to
5743625
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5743625069
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| m_zoom_in_action->setShortcuts({QKeySequence(QKeySequence::ZoomIn), QKeySequence(tr("Ctrl+="))}); | ||
| m_zoom_in_action->setStatusTip(tr("Increase the font size")); | ||
|
|
||
| m_zoom_out_action = new QAction(tr("Zoom &Out"), this); | ||
| m_zoom_out_action->setShortcuts({QKeySequence(QKeySequence::ZoomOut), QKeySequence(tr("Ctrl+_"))}); |
There was a problem hiding this comment.
These new window-level shortcuts reuse Ctrl++/Ctrl+= and Ctrl+-/Ctrl+_, which are already bound in RPCConsole::setButtonIcons() for console font resizing. In -disablewallet mode (or no-wallet builds), BitcoinGUI embeds RPCConsole as the central widget, so both shortcut sets live in the same window context and become ambiguous; users can get inconsistent behavior (global UI scale changes instead of, or competing with, console-only font resizing). This regression is introduced by adding the duplicate key sequences here without scoping them away from the console context.
Useful? React with 👍 / 👎.
5743625 to
391ac6d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 391ac6d679
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -1348,15 +1348,15 @@ void RPCConsole::setButtonIcons() | |||
|
|
|||
| GUIUtil::setIcon(ui->fontBiggerButton, "fontbigger", GUIUtil::ThemedColor::BLUE, consoleButtonsSize); | |||
| //: Main shortcut to increase the RPC console font size. | |||
| ui->fontBiggerButton->setShortcut(tr("Ctrl++")); | |||
| GUIUtil::AddButtonShortcut(ui->fontBiggerButton, tr("Ctrl++"), Qt::WidgetWithChildrenShortcut); | |||
There was a problem hiding this comment.
Using Qt::WidgetWithChildrenShortcut on shortcuts parented to ui->fontBiggerButton/ui->fontSmallerButton makes Ctrl++/Ctrl+- active only when those buttons (or their children) have focus. In normal RPC console use, focus is in lineEdit or messagesWidget, so the keyboard zoom bindings no longer fire there and users lose the console font-resize shortcut behavior. This regression was introduced by replacing setShortcut(...) with AddButtonShortcut(..., Qt::WidgetWithChildrenShortcut) in RPCConsole::setButtonIcons().
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/qt/bitcoingui.cpp`:
- Around line 779-784: After updating the font scale
(GUIUtil::g_font_registry.SetFontScale and GUIUtil::updateFonts()) and saving it
to the options model, call the window width recalculation so the tab toolbar
minimum width is recomputed; add a call to updateWidth() (the
BitcoinGUI::updateWidth() method) after the clientModel/optionsModel update to
recompute main-window width and avoid clipped labels.
- Around line 525-535: adjustFontScale currently returns early when
OptionsModel::isOptionOverridden("-font-scale") is true but leaves the UI
actions active; update adjustFontScale (or the same initialization path) to
query OptionsModel::isOptionOverridden("-font-scale") and call
m_zoom_in_action->setEnabled(false), m_zoom_out_action->setEnabled(false), and
m_zoom_reset_action->setEnabled(false) when overridden (and setEnabled(true)
when not) so the zoom QAction items reflect the override state and no longer
advertise shortcuts that perform no-ops.
In `@src/qt/forms/appearancewidget.ui`:
- Around line 185-189: AppearanceWidget::updateFontScale currently passes the
raw slider value to GUIUtil::g_font_registry.SetFontScale, which can produce an
invalid zero font size for extreme values; fix it by clamping the incoming
nScale to the valid range before calling SetFontScale (use the same bounds as
elsewhere, e.g. constexpr FONT_SCALE_MIN = -100 and FONT_SCALE_MAX = 100 and
apply std::clamp), then call GUIUtil::g_font_registry.SetFontScale(nScale) and
GUIUtil::updateFonts().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8de5e6e9-0f72-4a20-964e-9ad88469d59a
📥 Commits
Reviewing files that changed from the base of the PR and between 04cb01468e99d09514a17603c102bb826eb01aab and 5743625069e86aae1667e1c051e31e1f85065b96.
📒 Files selected for processing (4)
CLAUDE.mdsrc/qt/bitcoingui.cppsrc/qt/bitcoingui.hsrc/qt/forms/appearancewidget.ui
🚧 Files skipped from review as they are similar to previous changes (2)
- CLAUDE.md
- src/qt/bitcoingui.h
| <property name="minimum"> | ||
| <number>-30</number> | ||
| <number>-100</number> | ||
| </property> | ||
| <property name="maximum"> | ||
| <number>30</number> | ||
| <number>100</number> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect every place that defines, clamps, persists, or applies font scale.
rg -n -C3 'SetFontScale\(|GetFontScale\(|DEFAULT_FONT_SCALE|OptionID::FontScale|FontScale|fontScaleSlider|FONT_SCALE_MIN|FONT_SCALE_MAX|-30|-100' srcRepository: dashpay/dash
Length of output: 50369
🏁 Script executed:
# Search for minimum font size checks and scale_steps modifications
rg -n 'MIN_FONT_SIZE|min.*font.*size|m_scale_steps|SetFontScale|GetScaledFontSize' src/qt/guiutil_font.h src/qt/guiutil_font.cpp --max-count=50Repository: dashpay/dash
Length of output: 928
🏁 Script executed:
# Check if there's any validation or clamping of font_scale when it's set
rg -n -A 5 'void SetFontScale|SetFontScale\(' src/qt/ --max-count=50Repository: dashpay/dash
Length of output: 2458
🏁 Script executed:
# Check the UI file to see if the slider has any other constraints
rg -n -B 5 -A 10 'fontScaleSlider' src/qt/forms/appearancewidget.ui | head -80Repository: dashpay/dash
Length of output: 650
🏁 Script executed:
# Check if there's any minimum font size validation or constraints
rg -n 'pointSizeF|setPointSize|MIN|MINIMUM|clamp.*size' src/qt/guiutil_font.cpp | head -50Repository: dashpay/dash
Length of output: 628
🏁 Script executed:
# Check git history or find if there was a previous version with -30..30
rg -n '\-30|\-100' src/qt/forms/appearancewidget.ui src/qt/guiutil_font.h src/qt/bitcoingui.cppRepository: dashpay/dash
Length of output: 185
🏁 Script executed:
# Check the DEFAULT_FONT_SCALE and see if there's documentation
rg -n 'DEFAULT_FONT_SCALE' src/qt/ -A 2 -B 2Repository: dashpay/dash
Length of output: 3384
🏁 Script executed:
# Check if there's any clamping in the font rendering or if Qt has built-in protection
rg -n 'setPointSizeF' src/qt/guiutil_font.cpp -B 3 -A 3 | head -60Repository: dashpay/dash
Length of output: 1280
🏁 Script executed:
# Check if there's any minimum font size handling or if Qt's setPointSizeF has built-in protection
rg -n 'pointSize.*0|font.*size.*0|MIN.*POINT|pointSizeF.*<' src/qt/guiutil_font.cppRepository: dashpay/dash
Length of output: 97
🏁 Script executed:
# Check the context around line 582 in guiutil_font.cpp
sed -n '575,590p' src/qt/guiutil_font.cppRepository: dashpay/dash
Length of output: 610
🏁 Script executed:
# Let's find the full context of where GetScaledFontSize is used and verify the calculation
sed -n '620,635p' src/qt/guiutil_font.cppRepository: dashpay/dash
Length of output: 710
🏁 Script executed:
# Verify by calculating what font size would result at the extremes
# Let's check if there are any tests for font scaling
rg -n 'font.*scale|FontScale.*-100|-30' src/test/ --max-count=20Repository: dashpay/dash
Length of output: 764
🏁 Script executed:
# Let's verify the exact calculation by checking if there's a minimum size applied anywhere
rg -n 'std::max\|clamp\|min.*size' src/qt/guiutil_font.h src/qt/guiutil_font.cpp | head -30Repository: dashpay/dash
Length of output: 38
🏁 Script executed:
# Let's verify the complete picture - check all the places where SetFontScale is called to see if there's any clamping
rg -n -B 3 'SetFontScale\(nScale\)|SetFontScale\(new_scale\)' src/qt/Repository: dashpay/dash
Length of output: 805
🏁 Script executed:
# Check if AppearanceWidget's updateFontScale clamps or validates the input
sed -n '239,243p' src/qt/appearancewidget.cppRepository: dashpay/dash
Length of output: 189
AppearanceWidget::updateFontScale() must validate the slider range before passing to SetFontScale().
The expanded slider range of -100..100 creates an unsafe code path. While command-line argument parsing (bitcoin.cpp:750–757) and keyboard shortcut handling (bitcoingui.cpp:773) both validate and clamp the font scale before applying it, AppearanceWidget::updateFontScale() at line 241 passes the slider value directly to SetFontScale(nScale) with no bounds checking.
With m_font_scale = -100 and m_scale_steps = 0.01, the scaling formula produces: GetScaledFontSize(size) = size * (1 + (-100 * 0.01)) = size * 0 = 0, resulting in zero font size.
Add a clamp in AppearanceWidget::updateFontScale() to ensure the value stays within the valid range, matching the validation already present in bitcoingui.cpp:773:
void AppearanceWidget::updateFontScale(int nScale)
{
constexpr int FONT_SCALE_MIN{-100};
constexpr int FONT_SCALE_MAX{100};
nScale = std::clamp(nScale, FONT_SCALE_MIN, FONT_SCALE_MAX);
GUIUtil::g_font_registry.SetFontScale(nScale);
GUIUtil::updateFonts();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/qt/forms/appearancewidget.ui` around lines 185 - 189,
AppearanceWidget::updateFontScale currently passes the raw slider value to
GUIUtil::g_font_registry.SetFontScale, which can produce an invalid zero font
size for extreme values; fix it by clamping the incoming nScale to the valid
range before calling SetFontScale (use the same bounds as elsewhere, e.g.
constexpr FONT_SCALE_MIN = -100 and FONT_SCALE_MAX = 100 and apply std::clamp),
then call GUIUtil::g_font_registry.SetFontScale(nScale) and
GUIUtil::updateFonts().
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/qt/bitcoingui.cpp (1)
765-767:⚠️ Potential issue | 🟡 MinorZoom actions still present as no-ops when
-font-scaleis overridden.At Line 765,
adjustFontScale()exits early, but the menu actions remain enabled and advertised. Users can trigger shortcuts that do nothing.💡 Proposed fix (sync action enabled state with override)
OptionsModel* optionsModel = _clientModel->getOptionsModel(); if (optionsModel) { + const bool font_scale_locked{optionsModel->isOptionOverridden("-font-scale")}; + m_zoom_in_action->setEnabled(!font_scale_locked); + m_zoom_out_action->setEnabled(!font_scale_locked); + m_zoom_reset_action->setEnabled(!font_scale_locked); unitDisplayControl->setOptionsModel(optionsModel); m_mask_values_action->setChecked(optionsModel->getOption(OptionsModel::OptionID::MaskValues).toBool());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/qt/bitcoingui.cpp` around lines 765 - 767, The early return in adjustFontScale() when clientModel->getOptionsModel()->isOptionOverridden("-font-scale") leaves zoom-related QAction objects enabled and responsive but doing nothing; update adjustFontScale() to explicitly set the enabled state (e.g., zoomInAction, zoomOutAction, resetZoomAction) to false when the option is overridden (and true otherwise), and update their tooltips/status hints to reflect that zooming is disabled when overridden so shortcuts/menu items are not advertised as functional; locate and modify adjustFontScale() and the QAction members (zoomInAction, zoomOutAction, resetZoomAction) to implement this toggle.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/qt/rpcconsole.cpp`:
- Around line 1351-1359: The problem is duplicate QShortcut registrations for
ui->fontBiggerButton and ui->fontSmallerButton because
GUIUtil::AddButtonShortcut is called each time setButtonIcons() runs; fix by
registering these shortcuts only once (e.g. move the GUIUtil::AddButtonShortcut
calls out of setButtonIcons() into the dialog/class constructor or gate them
with a boolean like shortcutsInitialized), or check for existing shortcuts on
the button (via button->findChildren<QShortcut*>() or button->shortcuts())
before calling GUIUtil::AddButtonShortcut for ui->fontBiggerButton and
ui->fontSmallerButton so repeated icon/theme reloads do not create duplicate
QShortcut objects.
---
Duplicate comments:
In `@src/qt/bitcoingui.cpp`:
- Around line 765-767: The early return in adjustFontScale() when
clientModel->getOptionsModel()->isOptionOverridden("-font-scale") leaves
zoom-related QAction objects enabled and responsive but doing nothing; update
adjustFontScale() to explicitly set the enabled state (e.g., zoomInAction,
zoomOutAction, resetZoomAction) to false when the option is overridden (and true
otherwise), and update their tooltips/status hints to reflect that zooming is
disabled when overridden so shortcuts/menu items are not advertised as
functional; locate and modify adjustFontScale() and the QAction members
(zoomInAction, zoomOutAction, resetZoomAction) to implement this toggle.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0a0e7d5c-dbf5-436e-a024-370a66f37c6e
📥 Commits
Reviewing files that changed from the base of the PR and between 5743625069e86aae1667e1c051e31e1f85065b96 and 391ac6d679665c38d9a79296c36a7913317830cd.
📒 Files selected for processing (7)
CLAUDE.mdsrc/qt/bitcoingui.cppsrc/qt/bitcoingui.hsrc/qt/forms/appearancewidget.uisrc/qt/guiutil.cppsrc/qt/guiutil.hsrc/qt/rpcconsole.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- CLAUDE.md
- src/qt/forms/appearancewidget.ui
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/qt/bitcoingui.cpp`:
- Around line 525-535: When adjustFontScale() exits early because the
-font-scale option is overridden, disable the zoom actions so they aren't active
no-ops: in setClientModel() after retrieving optionsModel call
optionsModel->isOptionOverridden("-font-scale") (or similar) and use that
boolean to call setEnabled(false) on m_zoom_in_action, m_zoom_out_action and
m_zoom_reset_action when the option is locked; this keeps the View menu and
shortcut hints in sync with the actual behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 69b282b2-962b-44df-acdb-beea25e6e8ec
📥 Commits
Reviewing files that changed from the base of the PR and between 391ac6d679665c38d9a79296c36a7913317830cd and d414f1d007fb774a36127ce4f8cccb189ea8a201.
📒 Files selected for processing (2)
src/qt/bitcoingui.cppsrc/qt/rpcconsole.cpp
| m_zoom_in_action = new QAction(tr("Zoom &In"), this); | ||
| m_zoom_in_action->setShortcuts({QKeySequence(QKeySequence::ZoomIn), QKeySequence(tr("Ctrl+="))}); | ||
| m_zoom_in_action->setStatusTip(tr("Increase the font size")); | ||
|
|
||
| m_zoom_out_action = new QAction(tr("Zoom &Out"), this); | ||
| m_zoom_out_action->setShortcuts({QKeySequence(QKeySequence::ZoomOut), QKeySequence(tr("Ctrl+_"))}); | ||
| m_zoom_out_action->setStatusTip(tr("Decrease the font size")); | ||
|
|
||
| m_zoom_reset_action = new QAction(tr("Reset &Zoom"), this); | ||
| m_zoom_reset_action->setShortcut(QKeySequence(tr("Ctrl+0"))); | ||
| m_zoom_reset_action->setStatusTip(tr("Reset the font size to default")); |
There was a problem hiding this comment.
Zoom actions remain enabled when -font-scale is overridden.
When adjustFontScale() bails out because -font-scale is overridden, these actions still appear enabled in the View menu and their shortcuts remain advertised. This turns the menu items into silent no-ops for users with overridden configs.
Consider disabling the actions when the option is overridden:
💡 Proposed fix (disable actions when overridden)
In setClientModel() after getting the optionsModel, add:
const bool font_scale_locked{optionsModel->isOptionOverridden("-font-scale")};
m_zoom_in_action->setEnabled(!font_scale_locked);
m_zoom_out_action->setEnabled(!font_scale_locked);
m_zoom_reset_action->setEnabled(!font_scale_locked);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/qt/bitcoingui.cpp` around lines 525 - 535, When adjustFontScale() exits
early because the -font-scale option is overridden, disable the zoom actions so
they aren't active no-ops: in setClientModel() after retrieving optionsModel
call optionsModel->isOptionOverridden("-font-scale") (or similar) and use that
boolean to call setEnabled(false) on m_zoom_in_action, m_zoom_out_action and
m_zoom_reset_action when the option is locked; this keeps the View menu and
shortcut hints in sync with the actual behavior.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR widens the font scale slider from -30..30 to -100..100 and adds Ctrl+/Ctrl- global zoom shortcuts. The most impactful verified issue is that the expanded range allows font scale to reach -100, producing zero-sized (invisible) fonts. Most other findings are valid but lower severity; two findings are false positives.
🔴 1 blocking | 🟡 3 suggestion(s) | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/qt/bitcoingui.cpp`:
- [BLOCKING] lines 90-91: FONT_SCALE_MIN of -100 produces zero-sized fonts
The formula in `guiutil_font.h:85` is `size * (1 + (m_font_scale * 0.01))`. At scale -100 this yields `size * 0 = 0`, making all text invisible. The PR widened the UI slider range from the previous -30..30 (confirmed via git diff of `appearancewidget.ui`) to -100..100 to match these new constants. The old minimum of -30 gave `size * 0.7` which was still legible. A reasonable minimum would be around -50 (giving `size * 0.5`, i.e. half-size text).
- [SUGGESTION] lines 765-768: Menu zoom items remain enabled when -font-scale CLI override is active
When `-font-scale` is overridden via CLI, `adjustFontScale()` silently returns at line 767 without any user feedback. The View > Zoom In / Zoom Out / Reset Zoom menu items remain visually enabled and clickable, but do nothing. Users have no way to know why the controls don't work.
- [SUGGESTION] lines 90-91: Font scale range defined in two places — drift risk
The constants `FONT_SCALE_MIN{-100}` and `FONT_SCALE_MAX{100}` are defined in an anonymous namespace in `bitcoingui.cpp:90-91`. The font scale slider in `appearancewidget.ui:185-189` has matching but independently hardcoded minimum/maximum values. If either is changed independently, the slider and keyboard shortcuts would enforce different ranges.
In `src/qt/rpcconsole.cpp`:
- [SUGGESTION] lines 564-570: Overlapping shortcuts between global zoom and RPC console font-size buttons
The new window-level zoom actions use `QKeySequence::ZoomIn` (Ctrl++) and `Ctrl+=` (line 526 of bitcoingui.cpp). The RPC console registers widget-level shortcuts for the same keys (`Ctrl++`, `Ctrl+=`, `Ctrl+-`, `Ctrl+_`) at `WidgetWithChildrenShortcut` context. When the RPC console has focus, Qt may raise an ambiguous-shortcut conflict since both a `WindowShortcut` and `WidgetWithChildrenShortcut` match the same key. The two actions also do different things: one changes the global app font scale, the other changes only the console font size.
| constexpr int FONT_SCALE_MIN{-100}; | ||
| constexpr int FONT_SCALE_MAX{100}; |
There was a problem hiding this comment.
🔴 Blocking: FONT_SCALE_MIN of -100 produces zero-sized fonts
The formula in guiutil_font.h:85 is size * (1 + (m_font_scale * 0.01)). At scale -100 this yields size * 0 = 0, making all text invisible. The PR widened the UI slider range from the previous -30..30 (confirmed via git diff of appearancewidget.ui) to -100..100 to match these new constants. The old minimum of -30 gave size * 0.7 which was still legible. A reasonable minimum would be around -50 (giving size * 0.5, i.e. half-size text).
source: ['claude']
| if (clientModel && clientModel->getOptionsModel() && | ||
| clientModel->getOptionsModel()->isOptionOverridden("-font-scale")) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Menu zoom items remain enabled when -font-scale CLI override is active
When -font-scale is overridden via CLI, adjustFontScale() silently returns at line 767 without any user feedback. The View > Zoom In / Zoom Out / Reset Zoom menu items remain visually enabled and clickable, but do nothing. Users have no way to know why the controls don't work.
source: ['codex', 'claude']
| GUIUtil::AddButtonShortcut(ui->fontBiggerButton, tr("Ctrl++"), Qt::WidgetWithChildrenShortcut); | ||
| //: Secondary shortcut to increase the RPC console font size. | ||
| GUIUtil::AddButtonShortcut(ui->fontBiggerButton, tr("Ctrl+="), Qt::WidgetWithChildrenShortcut); | ||
| //: Main shortcut to decrease the RPC console font size. | ||
| GUIUtil::AddButtonShortcut(ui->fontSmallerButton, tr("Ctrl+-"), Qt::WidgetWithChildrenShortcut); | ||
| //: Secondary shortcut to decrease the RPC console font size. | ||
| GUIUtil::AddButtonShortcut(ui->fontSmallerButton, tr("Ctrl+_"), Qt::WidgetWithChildrenShortcut); |
There was a problem hiding this comment.
🟡 Suggestion: Overlapping shortcuts between global zoom and RPC console font-size buttons
The new window-level zoom actions use QKeySequence::ZoomIn (Ctrl++) and Ctrl+= (line 526 of bitcoingui.cpp). The RPC console registers widget-level shortcuts for the same keys (Ctrl++, Ctrl+=, Ctrl+-, Ctrl+_) at WidgetWithChildrenShortcut context. When the RPC console has focus, Qt may raise an ambiguous-shortcut conflict since both a WindowShortcut and WidgetWithChildrenShortcut match the same key. The two actions also do different things: one changes the global app font scale, the other changes only the console font size.
source: ['claude']
| constexpr int FONT_SCALE_MIN{-100}; | ||
| constexpr int FONT_SCALE_MAX{100}; |
There was a problem hiding this comment.
🟡 Suggestion: Font scale range defined in two places — drift risk
The constants FONT_SCALE_MIN{-100} and FONT_SCALE_MAX{100} are defined in an anonymous namespace in bitcoingui.cpp:90-91. The font scale slider in appearancewidget.ui:185-189 has matching but independently hardcoded minimum/maximum values. If either is changed independently, the slider and keyboard shortcuts would enforce different ranges.
source: ['claude']
| "<b>" + ui->clearButton->shortcut().toString(QKeySequence::NativeText) + "</b>", | ||
| "<b>" + ui->fontBiggerButton->shortcut().toString(QKeySequence::NativeText) + "</b>", | ||
| "<b>" + ui->fontSmallerButton->shortcut().toString(QKeySequence::NativeText) + "</b>", | ||
| "<b>" + QKeySequence(tr("Ctrl++")).toString(QKeySequence::NativeText) + "</b>", | ||
| "<b>" + QKeySequence(tr("Ctrl+-")).toString(QKeySequence::NativeText) + "</b>", |
There was a problem hiding this comment.
💬 Nitpick: Shortcut display text in clear() help message is hardcoded separately
The clear() method at line 1045-1046 displays Ctrl++ and Ctrl+- as help text using hardcoded QKeySequence(tr("Ctrl++")). The actual shortcuts are registered separately at lines 564-570. If either location is changed independently, the displayed shortcuts in the help text will be wrong.
source: ['claude']
|
✅ Final review complete — no blockers (commit 12c0ebd) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76bec14969
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Register console font-size shortcuts once (not in setButtonIcons which | ||
| // re-runs on theme changes and would create duplicate QShortcut objects). | ||
| //: Main shortcut to increase the RPC console font size. | ||
| GUIUtil::AddButtonShortcut(ui->fontBiggerButton, tr("Ctrl++"), Qt::WidgetWithChildrenShortcut); |
There was a problem hiding this comment.
Using Qt::WidgetWithChildrenShortcut here scopes the shortcut to ui->fontBiggerButton (and similarly ui->fontSmallerButton), which means Ctrl++/Ctrl+- only fire when those buttons have focus. In normal RPC console use, focus is in lineEdit or messagesWidget, so the console font-resize shortcuts stop working; in -disablewallet mode this also lets the new global zoom actions take over instead of console-local resizing. Restoring a window-level shortcut context avoids this regression.
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR #7216 adds Ctrl+/- zoom shortcuts with font scale persistence. Verified 2 agent findings: 1 confirmed with reduced severity (shortcut scope matches existing pattern), 1 dropped (factually incorrect claim about startup clamping). Found 1 additional CLI range inconsistency.
Reviewed commit: 76bec149
💬 2 nitpick(s)
2 additional findings
💬 nitpick: Zoom shortcuts use Qt::WindowShortcut scope (consistent with existing pattern)
src/qt/bitcoingui.cpp (line 1)
The zoom QActions at bitcoingui.cpp:525-535 use the default Qt::WindowShortcut context (no setShortcutContext call found anywhere in src/qt/). This means shortcuts won't fire when a separate dialog (e.g., OptionsDialog) has focus on Linux/Windows. However: (1) this is consistent with ALL other shortcuts in BitcoinGUI (e.g., Ctrl+Shift+D, Ctrl+M), (2) on macOS, native menu bar shortcuts work regardless of active window, and (3) 'global' in the commit message likely refers to zoom affecting all fonts application-wide, not the shortcut scope. Downgraded from medium since it matches existing codebase conventions.
💬 nitpick: CLI -font-scale validation still accepts [-100, 100] while shortcut/slider clamp uses [-50, 100]
src/qt/bitcoin.cpp (line 1)
Commit 76bec14 changed FONT_SCALE_MIN to -50 'to prevent zero-sized fonts', but bitcoin.cpp:750 still validates the -font-scale CLI arg against [-100, 100]. At scale -100, GetScaledFontSize() computes size * (1 + (-100 * 0.01)) = 0, producing zero-sized fonts. The slider range was also updated from [-30,30] to [-50,100] in appearancewidget.ui. Pre-existing issue but made more visible by this PR's intent to prevent zero-sized fonts. Consider updating bitcoin.cpp:750 to use -50 as the minimum for consistency.
Add Ctrl++/Ctrl+=, Ctrl+-, and Ctrl+0 shortcuts for zooming in, out, and resetting font scale. Clamp range and slider bounds use [-100, 100] matching the CLI-accepted range. Shortcuts are disabled when -font-scale is CLI-overridden, consistent with the Appearance slider behavior. Scope existing RPCConsole font-size shortcuts to WidgetWithChildrenShortcut to avoid ambiguity with the new window-level zoom actions when the console is embedded as the central widget (e.g. -disablewallet mode).
Move RPC console font-size shortcut registration out of setButtonIcons() into the constructor so theme reloads don't create duplicate QShortcut objects. Also call updateWidth() after adjustFontScale() to recompute toolbar minimum width when the font scale changes.
At scale -100, the formula size*(1+scale*0.01) yields zero, making all text invisible. Change minimum from -100 to -50 (half-size text) in both the constant and the appearance slider.
76bec14 to
bf517df
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf517dfbc0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| GUIUtil::AddButtonShortcut(ui->fontBiggerButton, tr("Ctrl++"), Qt::WidgetWithChildrenShortcut); | ||
| //: Secondary shortcut to increase the RPC console font size. | ||
| GUIUtil::AddButtonShortcut(ui->fontBiggerButton, tr("Ctrl+="), Qt::WidgetWithChildrenShortcut); |
There was a problem hiding this comment.
Binding these QShortcuts with Qt::WidgetWithChildrenShortcut on ui->fontBiggerButton means they are only active while that button (or its children) has focus; in normal RPC console usage focus is on lineEdit/messagesWidget, so Ctrl++ and Ctrl+- no longer resize console fonts from the keyboard. This is a user-visible regression of existing console shortcut behavior and is especially noticeable when typing commands.
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean feature PR that adds global zoom shortcuts to Dash Qt by reusing existing font-scaling infrastructure and fixes a real duplicate-shortcut bug in the RPC console. The one substantive issue is that the CLI -font-scale validation still accepts -100 (producing zero-sized fonts), inconsistent with the PR's stated goal. The rest of the findings are either resolved at HEAD or false positives.
Reviewed commit: bf517df
🟡 2 suggestion(s) | 💬 1 nitpick(s)
2 additional findings
🟡 suggestion: CLI `-font-scale` validation still allows -100, producing zero-sized fonts
src/qt/bitcoin.cpp (lines 750-757)
The commit bf517df explicitly aims to "prevent zero-sized fonts" by clamping the GUI minimum to -50. The slider (appearancewidget.ui) and shortcut path (bitcoingui.cpp:773) both enforce -50..100. However, the CLI validation at bitcoin.cpp:750 still accepts -100..100, and the help text at line 504 still advertises that range.
With GetScaledFontSize(size) = size * (1 + (scale * 0.01)), a scale of -100 yields size * 0.0 = 0, giving zero-point-size fonts. A user with font-scale=-100 in dash.conf or on the command line would get invisible text.
This is pre-existing behavior, so not blocking for this PR, but since the PR explicitly targets this class of bug it would be consistent to update both the CLI bounds and the help text to match the GUI's -50..100 range.
💡 Suggested change
if (gArgs.IsArgSet("-font-scale")) {
const int nScaleMin = -50, nScaleMax = 100;
int nScale = gArgs.GetIntArg("-font-scale", GUIUtil::g_font_registry.GetFontScale());
if (nScale < nScaleMin || nScale > nScaleMax) {
QMessageBox::critical(nullptr, PACKAGE_NAME,
QObject::tr("Error: Specified font-scale invalid. Valid range %1 to %2.").arg(nScaleMin).arg(nScaleMax));
return EXIT_FAILURE;
}
GUIUtil::g_font_registry.SetFontScale(nScale);
}
🟡 suggestion: Help text for `-font-scale` advertises -100..100, should match actual bounds
src/qt/bitcoin.cpp (line 504)
If the CLI bounds are updated to -50..100 (per the previous finding), the help text at line 504 still says .arg(-100).arg(100) and would need to be updated to .arg(-50).arg(100) to stay accurate.
💡 Suggested change
argsman.AddArg("-font-scale", QObject::tr("Set a scale factor which gets applied to the base font size. Possible range %1 (smallest fonts) to %2 (largest fonts). (default: %3)").arg(-50).arg(100).arg(GUIUtil::FontRegistry::DEFAULT_FONT_SCALE).toStdString(), ArgsManager::ALLOW_ANY, OptionsCategory::GUI);
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/qt/bitcoin.cpp`:
- [SUGGESTION] lines 750-757: CLI `-font-scale` validation still allows -100, producing zero-sized fonts
The commit bf517dfbc0 explicitly aims to "prevent zero-sized fonts" by clamping the GUI minimum to -50. The slider (appearancewidget.ui) and shortcut path (bitcoingui.cpp:773) both enforce -50..100. However, the CLI validation at bitcoin.cpp:750 still accepts -100..100, and the help text at line 504 still advertises that range.
With `GetScaledFontSize(size) = size * (1 + (scale * 0.01))`, a scale of -100 yields `size * 0.0 = 0`, giving zero-point-size fonts. A user with `font-scale=-100` in `dash.conf` or on the command line would get invisible text.
This is pre-existing behavior, so not blocking for this PR, but since the PR explicitly targets this class of bug it would be consistent to update both the CLI bounds and the help text to match the GUI's -50..100 range.
- [SUGGESTION] line 504: Help text for `-font-scale` advertises -100..100, should match actual bounds
If the CLI bounds are updated to -50..100 (per the previous finding), the help text at line 504 still says `.arg(-100).arg(100)` and would need to be updated to `.arg(-50).arg(100)` to stay accurate.
| m_zoom_in_action = new QAction(tr("Zoom &In"), this); | ||
| m_zoom_in_action->setShortcuts({QKeySequence(QKeySequence::ZoomIn), QKeySequence(tr("Ctrl+="))}); | ||
| m_zoom_in_action->setStatusTip(tr("Increase the font size")); | ||
|
|
||
| m_zoom_out_action = new QAction(tr("Zoom &Out"), this); | ||
| m_zoom_out_action->setShortcuts({QKeySequence(QKeySequence::ZoomOut), QKeySequence(tr("Ctrl+_"))}); | ||
| m_zoom_out_action->setStatusTip(tr("Decrease the font size")); | ||
|
|
||
| m_zoom_reset_action = new QAction(tr("Reset &Zoom"), this); | ||
| m_zoom_reset_action->setShortcut(QKeySequence(tr("Ctrl+0"))); | ||
| m_zoom_reset_action->setStatusTip(tr("Reset the font size to default")); |
There was a problem hiding this comment.
💬 Nitpick: Zoom actions remain enabled when -font-scale is overridden on the command line
When -font-scale is set via CLI, adjustFontScale() silently returns at line 766-767 without feedback. The zoom menu items and their shortcuts remain visually enabled, creating silent no-ops. The AppearanceWidget already follows the pattern of disabling its font-scale slider when the option is overridden (appearancewidget.cpp:188). Applying the same pattern to the zoom actions in setClientModel() would keep the UI consistent.
source: ['coderabbit']
The GUI slider and keyboard shortcuts already enforce -50..100, but the CLI validation and help text still allowed -100..100. A scale of -100 produces zero-sized fonts (size * 0.0 = 0). Align CLI bounds with GUI for consistency.
017f474 to
12c0ebd
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12c0ebdda4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Register console font-size shortcuts once (not in setButtonIcons which | ||
| // re-runs on theme changes and would create duplicate QShortcut objects). | ||
| //: Main shortcut to increase the RPC console font size. | ||
| GUIUtil::AddButtonShortcut(ui->fontBiggerButton, tr("Ctrl++"), Qt::WidgetWithChildrenShortcut); |
There was a problem hiding this comment.
Binding the RPC console zoom shortcuts with Qt::WidgetWithChildrenShortcut limits activation to fontBiggerButton/fontSmallerButton focus, so Ctrl++ and Ctrl+- no longer work during normal console usage when focus is in lineEdit or messagesWidget. This regresses previous keyboard behavior from setShortcut(...), and in -disablewallet mode it also allows the new global zoom actions to take over instead of console-local font resizing.
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The feature is mostly well integrated: the global zoom actions reuse the existing font-scale plumbing, and the PR already fixes the earlier zero-size-font problem plus the duplicate-shortcut bug in the RPC console. I verified three remaining suggestion-level issues in the current head: the View zoom actions stay enabled even when -font-scale makes them no-ops, shortcut-driven font-scale changes do not propagate back to the Appearance dialog's mapped slider, and the new RPC console shortcuts are attached to the buttons rather than the console window so they do not fire from normal console focus targets.
Reviewed commit: 12c0ebd
🟡 3 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/qt/bitcoingui.cpp`:
- [SUGGESTION] lines 763-768: Zoom actions stay enabled when `-font-scale` disables the feature
`adjustFontScale()` returns immediately when `OptionsModel::isOptionOverridden("-font-scale")` is true, so the new View > Zoom In / Zoom Out / Reset Zoom actions become silent no-ops. The Appearance widget already handles the same override by disabling its slider in `AppearanceWidget::setModel()`, so leaving the menu actions enabled makes the UI inconsistent and advertises shortcuts that cannot work. Disable the three zoom actions when the option is overridden so the menu state matches the actual behavior.
- [SUGGESTION] lines 779-784: Shortcut-driven font-scale changes do not update the Appearance slider
The new shortcut path writes the persisted value with `clientModel->getOptionsModel()->setOption(OptionsModel::FontScale, new_scale)`, but `OptionsModel::setOption()` does not emit `dataChanged`; only `OptionsModel::setData()` does. `AppearanceWidget` binds `ui->fontScaleSlider` through `QDataWidgetMapper`, so if the options dialog is open while the user presses Ctrl++ / Ctrl+- / Ctrl+0, the live font scale changes but the mapped slider can stay stale until the dialog is rebuilt. Updating the model through `setData()` (or emitting `dataChanged` for `FontScale`) would keep the dialog synchronized with the new shortcut path.
In `src/qt/rpcconsole.cpp`:
- [SUGGESTION] lines 564-570: RPC console font shortcuts are scoped to the buttons instead of the console
`GUIUtil::AddButtonShortcut()` creates `QShortcut` objects parented to the passed button (`new QShortcut(shortcut, button)` in `guiutil.cpp`). With `Qt::WidgetWithChildrenShortcut`, a shortcut is only active while that parent widget or one of its children has focus. Because the new console bindings are attached to `ui->fontBiggerButton` / `ui->fontSmallerButton`, Ctrl++ and Ctrl+- will not fire while focus is in the line edit, message view, or the rest of the RPC console — exactly the places users normally are when they want these shortcuts. Parent the shortcuts to the console/dialog instead of the individual buttons so the advertised bindings are active across the RPC console window.
| void BitcoinGUI::adjustFontScale(int delta) | ||
| { | ||
| if (clientModel && clientModel->getOptionsModel() && | ||
| clientModel->getOptionsModel()->isOptionOverridden("-font-scale")) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Zoom actions stay enabled when -font-scale disables the feature
adjustFontScale() returns immediately when OptionsModel::isOptionOverridden("-font-scale") is true, so the new View > Zoom In / Zoom Out / Reset Zoom actions become silent no-ops. The Appearance widget already handles the same override by disabling its slider in AppearanceWidget::setModel(), so leaving the menu actions enabled makes the UI inconsistent and advertises shortcuts that cannot work. Disable the three zoom actions when the option is overridden so the menu state matches the actual behavior.
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/qt/bitcoingui.cpp`:
- [SUGGESTION] lines 763-768: Zoom actions stay enabled when `-font-scale` disables the feature
`adjustFontScale()` returns immediately when `OptionsModel::isOptionOverridden("-font-scale")` is true, so the new View > Zoom In / Zoom Out / Reset Zoom actions become silent no-ops. The Appearance widget already handles the same override by disabling its slider in `AppearanceWidget::setModel()`, so leaving the menu actions enabled makes the UI inconsistent and advertises shortcuts that cannot work. Disable the three zoom actions when the option is overridden so the menu state matches the actual behavior.
| GUIUtil::g_font_registry.SetFontScale(new_scale); | ||
| GUIUtil::updateFonts(); | ||
| updateWidth(); | ||
|
|
||
| if (clientModel && clientModel->getOptionsModel()) { | ||
| clientModel->getOptionsModel()->setOption(OptionsModel::FontScale, new_scale); |
There was a problem hiding this comment.
🟡 Suggestion: Shortcut-driven font-scale changes do not update the Appearance slider
The new shortcut path writes the persisted value with clientModel->getOptionsModel()->setOption(OptionsModel::FontScale, new_scale), but OptionsModel::setOption() does not emit dataChanged; only OptionsModel::setData() does. AppearanceWidget binds ui->fontScaleSlider through QDataWidgetMapper, so if the options dialog is open while the user presses Ctrl++ / Ctrl+- / Ctrl+0, the live font scale changes but the mapped slider can stay stale until the dialog is rebuilt. Updating the model through setData() (or emitting dataChanged for FontScale) would keep the dialog synchronized with the new shortcut path.
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/qt/bitcoingui.cpp`:
- [SUGGESTION] lines 779-784: Shortcut-driven font-scale changes do not update the Appearance slider
The new shortcut path writes the persisted value with `clientModel->getOptionsModel()->setOption(OptionsModel::FontScale, new_scale)`, but `OptionsModel::setOption()` does not emit `dataChanged`; only `OptionsModel::setData()` does. `AppearanceWidget` binds `ui->fontScaleSlider` through `QDataWidgetMapper`, so if the options dialog is open while the user presses Ctrl++ / Ctrl+- / Ctrl+0, the live font scale changes but the mapped slider can stay stale until the dialog is rebuilt. Updating the model through `setData()` (or emitting `dataChanged` for `FontScale`) would keep the dialog synchronized with the new shortcut path.
| GUIUtil::AddButtonShortcut(ui->fontBiggerButton, tr("Ctrl++"), Qt::WidgetWithChildrenShortcut); | ||
| //: Secondary shortcut to increase the RPC console font size. | ||
| GUIUtil::AddButtonShortcut(ui->fontBiggerButton, tr("Ctrl+="), Qt::WidgetWithChildrenShortcut); | ||
| //: Main shortcut to decrease the RPC console font size. | ||
| GUIUtil::AddButtonShortcut(ui->fontSmallerButton, tr("Ctrl+-"), Qt::WidgetWithChildrenShortcut); | ||
| //: Secondary shortcut to decrease the RPC console font size. | ||
| GUIUtil::AddButtonShortcut(ui->fontSmallerButton, tr("Ctrl+_"), Qt::WidgetWithChildrenShortcut); |
There was a problem hiding this comment.
🟡 Suggestion: RPC console font shortcuts are scoped to the buttons instead of the console
GUIUtil::AddButtonShortcut() creates QShortcut objects parented to the passed button (new QShortcut(shortcut, button) in guiutil.cpp). With Qt::WidgetWithChildrenShortcut, a shortcut is only active while that parent widget or one of its children has focus. Because the new console bindings are attached to ui->fontBiggerButton / ui->fontSmallerButton, Ctrl++ and Ctrl+- will not fire while focus is in the line edit, message view, or the rest of the RPC console — exactly the places users normally are when they want these shortcuts. Parent the shortcuts to the console/dialog instead of the individual buttons so the advertised bindings are active across the RPC console window.
source: ['codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/qt/rpcconsole.cpp`:
- [SUGGESTION] lines 564-570: RPC console font shortcuts are scoped to the buttons instead of the console
`GUIUtil::AddButtonShortcut()` creates `QShortcut` objects parented to the passed button (`new QShortcut(shortcut, button)` in `guiutil.cpp`). With `Qt::WidgetWithChildrenShortcut`, a shortcut is only active while that parent widget or one of its children has focus. Because the new console bindings are attached to `ui->fontBiggerButton` / `ui->fontSmallerButton`, Ctrl++ and Ctrl+- will not fire while focus is in the line edit, message view, or the rest of the RPC console — exactly the places users normally are when they want these shortcuts. Parent the shortcuts to the console/dialog instead of the individual buttons so the advertised bindings are active across the RPC console window.
| QMenu* view = appMenuBar->addMenu(tr("&View")); | ||
| view->addAction(m_zoom_in_action); | ||
| view->addAction(m_zoom_out_action); | ||
| view->addAction(m_zoom_reset_action); | ||
|
|
There was a problem hiding this comment.
why not a part of Window? I think it's a better fit. Or at least settings.
There was a problem hiding this comment.
diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp
index 60f7a4a10c..76f4d8b426 100644
--- a/src/qt/bitcoingui.cpp
+++ b/src/qt/bitcoingui.cpp
@@ -697,10 +697,7 @@ void BitcoinGUI::createMenuBar()
}
settings->addAction(optionsAction);
- QMenu* view = appMenuBar->addMenu(tr("&View"));
- view->addAction(m_zoom_in_action);
- view->addAction(m_zoom_out_action);
- view->addAction(m_zoom_reset_action);
+// QMenu* view = appMenuBar->addMenu(tr("&View"));
QMenu* window_menu = appMenuBar->addMenu(tr("&Window"));
@@ -729,6 +726,11 @@ void BitcoinGUI::createMenuBar()
});
#endif
+ window_menu->addSeparator();
+ window_menu->addAction(m_zoom_in_action);
+ window_menu->addAction(m_zoom_out_action);
+ window_menu->addAction(m_zoom_reset_action);
+
if (walletFrame) {
#ifdef Q_OS_MACOS
window_menu->addSeparator();
| //: Main shortcut to increase the RPC console font size. | ||
| ui->fontBiggerButton->setShortcut(tr("Ctrl++")); | ||
| //: Secondary shortcut to increase the RPC console font size. | ||
| GUIUtil::AddButtonShortcut(ui->fontBiggerButton, tr("Ctrl+=")); | ||
|
|
||
| GUIUtil::setIcon(ui->fontSmallerButton, "fontsmaller", GUIUtil::ThemedColor::BLUE, consoleButtonsSize); | ||
| //: Main shortcut to decrease the RPC console font size. | ||
| ui->fontSmallerButton->setShortcut(tr("Ctrl+-")); | ||
| //: Secondary shortcut to decrease the RPC console font size. | ||
| GUIUtil::AddButtonShortcut(ui->fontSmallerButton, tr("Ctrl+_")); |
There was a problem hiding this comment.
shortcut Ctrl++ doesn't work when RPC Console is open (no zoom for main window); but shortcut for font bigger / font smaller is removed by this PR.
Could you revert these changes?
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR cleanly adds zoom shortcuts/View menu by reusing font-scale infrastructure and fixes a real RPCConsole duplicate-QShortcut bug. No blockers found. Main concerns: (1) UX inconsistency where zoom menu/shortcuts remain enabled when -font-scale is overridden while the appearance slider is correctly disabled; (2) users with persisted scales below the new -50 minimum see surprising zoom-out behavior; (3) the CLI minimum tightening is a soft-breaking change. Codex's macOS Ctrl+0 finding was dropped because Qt's QKeySequence portable parsing already maps 'Ctrl' to Cmd on macOS.
Reviewed commit: 12c0ebd
🟡 4 suggestion(s) | 💬 4 nitpick(s)
1 additional finding
🟡 suggestion: Tightening -font-scale CLI minimum from -100 to -50 is a soft-breaking change
src/qt/bitcoin.cpp (lines 749-758)
Users who previously launched with -font-scale=-80 (valid before, allowed by the prior -100..100 bounds) will now hit a fatal startup error (QMessageBox::critical + EXIT_FAILURE). The rationale of preventing zero-sized fonts is sound, but this deserves a release-note callout. Note also the asymmetry: persisted settings are not bounds-checked at load (optionsmodel.cpp:291), so the same -80 value would silently take effect when restored from settings but fatally reject when supplied via CLI. Either also clamp/validate the persisted value or document the asymmetry.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/qt/bitcoingui.cpp`:
- [SUGGESTION] lines 763-768: Zoom menu items and shortcuts remain enabled when -font-scale is overridden
When `-font-scale` is set on the command line, `adjustFontScale` silently returns at line 767, but the View > Zoom In/Out/Reset menu items remain enabled and the Ctrl++/Ctrl+-/Ctrl+0 shortcuts still fire as no-ops. This is inconsistent with `appearancewidget.cpp:188-191`, where the slider is correctly disabled in the same condition. Disable `m_zoom_in_action`/`m_zoom_out_action`/`m_zoom_reset_action` once the OptionsModel is available (e.g. in `setClientModel`) so the UI honestly reflects what is actually controllable.
- [SUGGESTION] lines 770-773: Persisted scales below the new -50 minimum produce inverted zoom-out behavior
`OptionsModel::Init` calls `SetFontScale(persisted)` directly without bounds checking (optionsmodel.cpp:291), and `FontRegistry::SetFontScale` does not clamp (guiutil_font.h:72). Users upgrading from a build that allowed -100..100 may have a persisted scale like -80. After this PR:
- Zoom-out from -80 computes `clamp(-80 + -3, -50, 100) = -50` — the font grows when the user tries to shrink it.
- Zoom-in from -80 jumps 30 units (to -50) instead of the intended 3.
Clamp `current_scale` itself to `[FONT_SCALE_MIN, FONT_SCALE_MAX]` before computing `new_scale`, or clamp the persisted value at load time in `OptionsModel::Init`, so legacy out-of-range values are normalized rather than producing surprising zoom direction.
- [SUGGESTION] lines 543-545: New shortcut/persistence path lacks automated coverage
The new path wires View actions into `adjustFontScale`, which mutates the live font registry, calls `updateFonts`/`updateWidth`, and persists `OptionsModel::FontScale`. There is no Qt unit test or functional coverage in the diff guarding shortcut registration, clamping behavior, or persistence. A small Qt test that drives the actions and checks `g_font_registry.GetFontScale()` and the persisted setting would protect against future regressions.
In `src/qt/bitcoin.cpp`:
- [SUGGESTION] lines 749-758: Tightening -font-scale CLI minimum from -100 to -50 is a soft-breaking change
Users who previously launched with `-font-scale=-80` (valid before, allowed by the prior `-100..100` bounds) will now hit a fatal startup error (`QMessageBox::critical` + `EXIT_FAILURE`). The rationale of preventing zero-sized fonts is sound, but this deserves a release-note callout. Note also the asymmetry: persisted settings are not bounds-checked at load (`optionsmodel.cpp:291`), so the same -80 value would silently take effect when restored from settings but fatally reject when supplied via CLI. Either also clamp/validate the persisted value or document the asymmetry.
| void BitcoinGUI::adjustFontScale(int delta) | ||
| { | ||
| if (clientModel && clientModel->getOptionsModel() && | ||
| clientModel->getOptionsModel()->isOptionOverridden("-font-scale")) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Zoom menu items and shortcuts remain enabled when -font-scale is overridden
When -font-scale is set on the command line, adjustFontScale silently returns at line 767, but the View > Zoom In/Out/Reset menu items remain enabled and the Ctrl++/Ctrl+-/Ctrl+0 shortcuts still fire as no-ops. This is inconsistent with appearancewidget.cpp:188-191, where the slider is correctly disabled in the same condition. Disable m_zoom_in_action/m_zoom_out_action/m_zoom_reset_action once the OptionsModel is available (e.g. in setClientModel) so the UI honestly reflects what is actually controllable.
source: ['claude', 'codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/qt/bitcoingui.cpp`:
- [SUGGESTION] lines 763-768: Zoom menu items and shortcuts remain enabled when -font-scale is overridden
When `-font-scale` is set on the command line, `adjustFontScale` silently returns at line 767, but the View > Zoom In/Out/Reset menu items remain enabled and the Ctrl++/Ctrl+-/Ctrl+0 shortcuts still fire as no-ops. This is inconsistent with `appearancewidget.cpp:188-191`, where the slider is correctly disabled in the same condition. Disable `m_zoom_in_action`/`m_zoom_out_action`/`m_zoom_reset_action` once the OptionsModel is available (e.g. in `setClientModel`) so the UI honestly reflects what is actually controllable.
| const int current_scale{GUIUtil::g_font_registry.GetFontScale()}; | ||
| const int new_scale{delta == 0 | ||
| ? GUIUtil::FontRegistry::DEFAULT_FONT_SCALE | ||
| : std::clamp(current_scale + delta, FONT_SCALE_MIN, FONT_SCALE_MAX)}; |
There was a problem hiding this comment.
🟡 Suggestion: Persisted scales below the new -50 minimum produce inverted zoom-out behavior
OptionsModel::Init calls SetFontScale(persisted) directly without bounds checking (optionsmodel.cpp:291), and FontRegistry::SetFontScale does not clamp (guiutil_font.h:72). Users upgrading from a build that allowed -100..100 may have a persisted scale like -80. After this PR:
- Zoom-out from -80 computes
clamp(-80 + -3, -50, 100) = -50— the font grows when the user tries to shrink it. - Zoom-in from -80 jumps 30 units (to -50) instead of the intended 3.
Clamp current_scale itself to [FONT_SCALE_MIN, FONT_SCALE_MAX] before computing new_scale, or clamp the persisted value at load time in OptionsModel::Init, so legacy out-of-range values are normalized rather than producing surprising zoom direction.
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/qt/bitcoingui.cpp`:
- [SUGGESTION] lines 770-773: Persisted scales below the new -50 minimum produce inverted zoom-out behavior
`OptionsModel::Init` calls `SetFontScale(persisted)` directly without bounds checking (optionsmodel.cpp:291), and `FontRegistry::SetFontScale` does not clamp (guiutil_font.h:72). Users upgrading from a build that allowed -100..100 may have a persisted scale like -80. After this PR:
- Zoom-out from -80 computes `clamp(-80 + -3, -50, 100) = -50` — the font grows when the user tries to shrink it.
- Zoom-in from -80 jumps 30 units (to -50) instead of the intended 3.
Clamp `current_scale` itself to `[FONT_SCALE_MIN, FONT_SCALE_MAX]` before computing `new_scale`, or clamp the persisted value at load time in `OptionsModel::Init`, so legacy out-of-range values are normalized rather than producing surprising zoom direction.
| connect(m_zoom_in_action, &QAction::triggered, this, [this] { adjustFontScale(FONT_SCALE_SHORTCUT_STEP); }); | ||
| connect(m_zoom_out_action, &QAction::triggered, this, [this] { adjustFontScale(-FONT_SCALE_SHORTCUT_STEP); }); | ||
| connect(m_zoom_reset_action, &QAction::triggered, this, [this] { adjustFontScale(0); }); |
There was a problem hiding this comment.
🟡 Suggestion: New shortcut/persistence path lacks automated coverage
The new path wires View actions into adjustFontScale, which mutates the live font registry, calls updateFonts/updateWidth, and persists OptionsModel::FontScale. There is no Qt unit test or functional coverage in the diff guarding shortcut registration, clamping behavior, or persistence. A small Qt test that drives the actions and checks g_font_registry.GetFontScale() and the persisted setting would protect against future regressions.
source: ['codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/qt/bitcoingui.cpp`:
- [SUGGESTION] lines 543-545: New shortcut/persistence path lacks automated coverage
The new path wires View actions into `adjustFontScale`, which mutates the live font registry, calls `updateFonts`/`updateWidth`, and persists `OptionsModel::FontScale`. There is no Qt unit test or functional coverage in the diff guarding shortcut registration, clamping behavior, or persistence. A small Qt test that drives the actions and checks `g_font_registry.GetFontScale()` and the persisted setting would protect against future regressions.
| if (clientModel && clientModel->getOptionsModel()) { | ||
| clientModel->getOptionsModel()->setOption(OptionsModel::FontScale, new_scale); | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: Open Appearance dialog's slider can fall out of sync after a zoom shortcut
OptionsModel::setOption(FontScale, ...) only calls update(value.toInt()) (optionsmodel.cpp:979-983) and does not emit dataChanged. The Appearance widget binds its slider via QDataWidgetMapper (appearancewidget.cpp:165). If the dialog is open while the user uses the zoom shortcut, the slider stays at the old value and clicking Cancel may revert to a stale prevScale. Either emit dataChanged for the FontScale row after the change, or route the shortcut through setData so the model emits properly.
source: ['claude']
| // Register console font-size shortcuts once (not in setButtonIcons which | ||
| // re-runs on theme changes and would create duplicate QShortcut objects). | ||
| //: Main shortcut to increase the RPC console font size. | ||
| GUIUtil::AddButtonShortcut(ui->fontBiggerButton, tr("Ctrl++"), Qt::WidgetWithChildrenShortcut); | ||
| //: Secondary shortcut to increase the RPC console font size. | ||
| GUIUtil::AddButtonShortcut(ui->fontBiggerButton, tr("Ctrl+="), Qt::WidgetWithChildrenShortcut); | ||
| //: Main shortcut to decrease the RPC console font size. | ||
| GUIUtil::AddButtonShortcut(ui->fontSmallerButton, tr("Ctrl+-"), Qt::WidgetWithChildrenShortcut); | ||
| //: Secondary shortcut to decrease the RPC console font size. | ||
| GUIUtil::AddButtonShortcut(ui->fontSmallerButton, tr("Ctrl+_"), Qt::WidgetWithChildrenShortcut); |
There was a problem hiding this comment.
💬 Nitpick: Console font shortcuts no longer set the button's shortcut() property
Previously, ui->fontBiggerButton->setShortcut(...) made the button's own shortcut() property non-empty (driving Qt's standard button highlight on activation). All four shortcuts now go through AddButtonShortcut, which calls animateClick() and leaves shortcut() empty. This explains why the welcome-text branch had to be switched away from reading shortcut(). Confirm there are no remaining call sites that read fontBiggerButton->shortcut() (a grep shows none) and verify Qt::WidgetWithChildrenShortcut is the right context — the prior WindowShortcut default fired regardless of which child within the console had focus.
source: ['claude']
|
|
||
| constexpr int FONT_SCALE_MIN{-50}; | ||
| constexpr int FONT_SCALE_MAX{100}; | ||
| constexpr int FONT_SCALE_SHORTCUT_STEP{3}; |
There was a problem hiding this comment.
💬 Nitpick: Shortcut step (3) differs from slider singleStep (10)
FONT_SCALE_SHORTCUT_STEP is 3 while appearancewidget.ui defines singleStep of 10 over the same scale. With a -50..100 range, 3 is fine-grained but inconsistent with one tick of the slider. Either align them or add a comment documenting why the shortcut intentionally has finer granularity.
source: ['claude']
| m_zoom_in_action->setStatusTip(tr("Increase the font size")); | ||
|
|
||
| m_zoom_out_action = new QAction(tr("Zoom &Out"), this); | ||
| m_zoom_out_action->setShortcuts({QKeySequence(QKeySequence::ZoomOut), QKeySequence(tr("Ctrl+_"))}); |
There was a problem hiding this comment.
💬 Nitpick: Ctrl+_ is an unusual zoom-out fallback and translatable
Ctrl+_ (Ctrl+Shift+-) is non-standard for zoom-out; the symmetry with Ctrl+= is the apparent motivation. Most apps expose only Ctrl+-. Additionally, the tr()-wrapped shortcut strings are exposed to translators who could (accidentally) localize them, breaking the binding for non-English locales. Consider dropping Ctrl+_ or making the strings non-translatable.
source: ['claude']
|
This pull request has conflicts, please rebase. |
Potential PR merge conflictsThis is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order. If these PRs merge firstThis PR will likely need a rebase:
|
4 participants
Issue being fixed or feature implemented
Settings > Options > Appearance.Ctrl++,Ctrl+-, andCtrl+0behavior into the existing infrastructure.What was done?
Screen.Recording.2026-03-12.at.10.41.57.mov
BitcoinGUIactions for zoom in, zoom out, and reset zoom.QKeySequence::ZoomIn/QKeySequence::ZoomOutshortcuts, with explicitCtrl+=,Ctrl+_, andCtrl+0coverage.Viewmenu to expose the zoom actions in the menu bar.adjustFontScale(int delta)helper inBitcoinGUIthat:GUIUtil::g_font_registry-30..30rangeGUIUtil::updateFonts()OptionsModel::FontScaleHow Has This Been Tested?
Ran, works
Breaking Changes
Checklist: