feat(qt): add global zoom shortcuts#7216
feat(qt): add global zoom shortcuts#7216PastaPastaPasta wants to merge 2 commits intodashpay:developfrom
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)
📝 Coding Plan
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
|
PastaPastaPasta
force-pushed
the
feat/qt-plus
branch
from
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
📒 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
PastaPastaPasta
force-pushed
the
feat/qt-plus
branch
2 times, most recently
from
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 👍 / 👎.
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).
PastaPastaPasta
force-pushed
the
feat/qt-plus
branch
from
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".
src/qt/rpcconsole.cpp
Outdated
| 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
📒 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
| GUIUtil::g_font_registry.SetFontScale(new_scale); | ||
| GUIUtil::updateFonts(); | ||
|
|
||
| if (clientModel && clientModel->getOptionsModel()) { | ||
| clientModel->getOptionsModel()->setOption(OptionsModel::FontScale, new_scale); | ||
| } |
There was a problem hiding this comment.
Recompute the main-window width after applying the new fonts.
The options-dialog path already reaches updateWidth() on appearance changes, but this shortcut path does not. After zooming in here, the tab toolbar can keep the old minimum width and clip labels until the user resizes the window manually.
📏 Suggested fix
GUIUtil::g_font_registry.SetFontScale(new_scale);
GUIUtil::updateFonts();
+ updateWidth();
if (clientModel && clientModel->getOptionsModel()) {
clientModel->getOptionsModel()->setOption(OptionsModel::FontScale, new_scale);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/qt/bitcoingui.cpp` around lines 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.
| <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
📒 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
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.
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
📒 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']
3 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: