feat(browser): persist zoom level across restarts#1930
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughChangesThe settings system persists a default zoom factor, the settings dialog edits it using available WebView zoom levels, and WebView applies it during construction, settings updates, and zoom resets. Default zoom configuration
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Up to standards ✅🟢 Issues
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/libs/browser/webview.cpp`:
- Around line 75-77: Update setZoomLevel so it compares level with
settings()->defaultZoomLevel before assigning the setting or starting
m_zoomPersistTimer; only perform both operations when the value actually
changes.
In `@src/libs/browser/webview.h`:
- Around line 48-49: Initialize WebView’s m_zoomLevel to -1 instead of 0 so the
first setZoomLevel call cannot return early and always applies the saved zoom
setting, including level 0; leave m_zoomPersistTimer unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 84440774-4690-438b-a25a-2f5b32984bf6
📒 Files selected for processing (4)
src/libs/browser/webview.cppsrc/libs/browser/webview.hsrc/libs/core/settings.cppsrc/libs/core/settings.h
|
Thanks for the contribution! I think the feature is useful, but constantly saving zoom on every change would likely annoy many users, who only wanted to zoom in once and never needed it to persist. A better approach would be to add a "default zoom level" setting like most browsers already do. |
|
thanks for the feedback, I'll push an update when I implement it. |
c8d1f5d to
20aef60
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/libs/browser/webview.cpp`:
- Around line 36-38: Remove the Settings::updated connection that invokes
applyDefaultZoomLevel() in the webview initialization, so unrelated settings
saves do not reset existing tabs’ custom zoom levels; retain default zoom
application for newly created tabs through the existing creation path.
- Around line 53-59: Update WebView::resetZoom() to call applyDefaultZoomLevel()
instead of directly setting defaultZoomLevel(), ensuring user-initiated resets
honor the configured defaultZoomFactor while preserving the existing reset
behavior.
In `@src/libs/ui/settingsdialog.cpp`:
- Line 197: Update the default zoom initialization around defaultZoomComboBox
and availableZoomLevels to handle findData returning -1 for missing or invalid
settings->defaultZoomFactor. Select the established valid default zoom entry as
a fallback, ensuring the combo box never remains unselected and does not save an
invalid value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 55180653-af2f-4aa3-b789-cc81740e68c2
📒 Files selected for processing (6)
src/libs/browser/webview.cppsrc/libs/browser/webview.hsrc/libs/core/settings.cppsrc/libs/core/settings.hsrc/libs/ui/settingsdialog.cppsrc/libs/ui/settingsdialog.ui
| connect(Core::Application::instance()->settings(), &Core::Settings::updated, this, [this]() { | ||
| applyDefaultZoomLevel(); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Avoid unconditionally resetting per-tab zoom on unrelated settings saves.
When Settings::updated is emitted (which occurs when the user clicks OK or Apply for any setting change, such as editing a proxy or modifying a font), applyDefaultZoomLevel() is invoked indiscriminately for all open tabs. This will immediately overwrite any per-tab custom zoom level the user might be actively using.
Consider removing this connection so the default zoom applies only to newly created tabs, or track whether the tab is currently using a user-customized zoom before applying the reset.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/libs/browser/webview.cpp` around lines 36 - 38, Remove the
Settings::updated connection that invokes applyDefaultZoomLevel() in the webview
initialization, so unrelated settings saves do not reset existing tabs’ custom
zoom levels; retain default zoom application for newly created tabs through the
existing creation path.
The document browser always started at 100%, so a preferred zoom had to be set again after every launch. Add a default zoom level to Settings > Content. The value applies to open views and to newly created ones, and is stored as a zoom percentage so it stays readable in the configuration file and survives changes to WebView::availableZoomLevels(). Reset Zoom now returns to the configured level rather than 100%. m_zoomLevel starts at an invalid index so that a default of 30%, which is index 0, is not skipped by the equality check in setZoomLevel(). Refs zealdocs#1335.
20aef60 to
1184150
Compare
The document browser always starts at the default zoom level, so a preferred zoom has to be set again after every launch.
The current level is now stored in the content settings group and applied when a WebView is created. It is kept as an index into WebView::availableZoomLevels(), with a negative value meaning unset so that Core does not need to know about the zoom level table owned by Browser.
Saving is debounced by one second, since zooming can be triggered repeatedly via the mouse wheel and Settings::save() rewrites the whole configuration file.
Fixes #1335.
Summary by CodeRabbit
New Features
Improvements