feat: activate edge snapping for auto-zoom viewport#1667
Open
namearth5005 wants to merge 10 commits intoCapSoftware:mainfrom
Open
feat: activate edge snapping for auto-zoom viewport#1667namearth5005 wants to merge 10 commits intoCapSoftware:mainfrom
namearth5005 wants to merge 10 commits intoCapSoftware:mainfrom
Conversation
…nfigurable fields
…placing hardcoded constants
- Remove #[allow(dead_code)] from apply_edge_snap_to_focus - Apply edge snapping per-frame in render pipeline for Auto mode segments - Add edge_snap_enabled config field (default true) - Set edge_snap_ratio to 0.0 on generated segments when disabled - Add Edge Snapping toggle in experimental settings UI
Comment on lines
+176
to
+193
| <SettingSlider | ||
| label="Min Zoom" | ||
| value={settings.autoZoomConfig?.minZoomAmount ?? 1.2} | ||
| onChange={(v) => handleConfigChange("minZoomAmount", v)} | ||
| min={1.0} | ||
| max={3.0} | ||
| step={0.1} | ||
| format={(v) => `${v.toFixed(1)}x`} | ||
| /> | ||
| <SettingSlider | ||
| label="Max Zoom" | ||
| value={settings.autoZoomConfig?.maxZoomAmount ?? 2.5} | ||
| onChange={(v) => handleConfigChange("maxZoomAmount", v)} | ||
| min={1.5} | ||
| max={4.0} | ||
| step={0.1} | ||
| format={(v) => `${v.toFixed(1)}x`} | ||
| /> |
Contributor
There was a problem hiding this comment.
No validation that Max Zoom ≥ Min Zoom
The two sliders are fully independent: Min Zoom goes up to 3.0× and Max Zoom starts at 1.5×, so a user can set Min Zoom = 3.0 and Max Zoom = 1.5.
When min_zoom_amount > max_zoom_amount, compute_zoom_amount produces inverted results:
- A tight cluster of clicks (small
max_dist) returnsmax_zoom_amount(1.5×) — i.e. minimal zoom. - Spread activity (large
max_dist) returnsmin_zoom_amount(3.0×) — i.e. maximum zoom.
The effect is completely backwards from the labelling. Consider clamping or cross-validating on change:
onChange={(v) => {
const clamped = Math.min(v, settings.autoZoomConfig?.maxZoomAmount ?? 4.0);
handleConfigChange("minZoomAmount", clamped);
}}and symmetrically for the Max Zoom slider:
onChange={(v) => {
const clamped = Math.max(v, settings.autoZoomConfig?.minZoomAmount ?? 1.0);
handleConfigChange("maxZoomAmount", clamped);
}}Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/routes/(window-chrome)/settings/experimental.tsx
Line: 176-193
Comment:
**No validation that Max Zoom ≥ Min Zoom**
The two sliders are fully independent: Min Zoom goes up to 3.0× and Max Zoom starts at 1.5×, so a user can set Min Zoom = 3.0 and Max Zoom = 1.5.
When `min_zoom_amount > max_zoom_amount`, `compute_zoom_amount` produces *inverted* results:
- A tight cluster of clicks (small `max_dist`) returns `max_zoom_amount` (1.5×) — i.e. minimal zoom.
- Spread activity (large `max_dist`) returns `min_zoom_amount` (3.0×) — i.e. maximum zoom.
The effect is completely backwards from the labelling. Consider clamping or cross-validating on change:
```tsx
onChange={(v) => {
const clamped = Math.min(v, settings.autoZoomConfig?.maxZoomAmount ?? 4.0);
handleConfigChange("minZoomAmount", clamped);
}}
```
and symmetrically for the Max Zoom slider:
```tsx
onChange={(v) => {
const clamped = Math.max(v, settings.autoZoomConfig?.minZoomAmount ?? 1.0);
handleConfigChange("maxZoomAmount", clamped);
}}
```
How can I resolve this? If you propose a fix, please make it concise.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Activates the existing dead-code
apply_edge_snap_to_focusfunction to prevent the zoomed viewport from panning into awkward corner positions (refs #1646, builds on #1663, #1664, #1665, #1666):apply_edge_snap_to_focus— removed#[allow(dead_code)], wired into per-frame renderingedge_snap_enabledtoggle in settings (default true); when disabled, segments getedge_snap_ratio: 0.0How it works
When the cursor moves near screen edges during a zoomed Auto segment:
Before: Camera tries to center on cursor → viewport extends past screen edge → awkward cut-off view
After:
apply_edge_snap_to_focussnaps the focus to keep the viewport cleanly aligned with the screen edge → professional-looking edge behavior (like Screen Studio)The function uses
edge_snap_ratio(0.25 = 25% of viewport) as a threshold. Within this zone, the focus snaps to show the edge cleanly rather than following the cursor into the corner.Files changed
crates/rendering/src/zoom_focus_interpolation.rs#[allow(dead_code)]crates/rendering/src/lib.rscrates/project/src/configuration.rsedge_snap_enabledtoAutoZoomConfigapps/desktop/src-tauri/src/recording.rsedge_snap_ratioon generated segmentsapps/desktop/.../experimental.tsxTest plan
Greptile Summary
This PR activates the previously dead-code
apply_edge_snap_to_focusfunction and bundles it with a largeAutoZoomConfigrefactor — converting all zoom-generation magic constants into configurable fields (dead-zone, double-click dedup, right-click filtering, spatial-intensity-based zoom) and exposing a subset of them through a new settings panel.Key changes:
apply_edge_snap_to_focusis now called per-frame inProjectUniforms::new, guarded behindZoomMode::Auto— clean and consistent with existing rendering patterns.generate_zoom_segments_from_clicks_implare replaced withAutoZoomConfigfields, and the config is threaded fromGeneralSettingsStorethrough to segment generation.compute_zoom_amountfunction varies zoom level by spatial spread of click positions; however movement-triggered and single-click segments always receivemax_zoom_amount(2.5× default) becausepositions.len() < 2, compared to the previous flatAUTO_ZOOM_AMOUNT = 1.5×. This is an unintentional behavioural regression.GeneralSettingsStore::geterrors are silently swallowed withunwrap_or(None).unwrap_or_default(), which falls back to edge snap enabled regardless of user preference.Confidence Score: 2/5
AutoZoomConfigrefactor that introduces a silent behavioural regression: movement-triggered segments and single-click segments now zoom atmax_zoom_amount(2.5× by default) instead of the previous 1.5×, becausecompute_zoom_amountunconditionally returnsmax_zoom_amountwhenpositions.len() < 2. Additionally, the new UI sliders allow Min Zoom > Max Zoom, inverting the intensity curve logic in Rust. These two issues affect every user who records with cursor movement, making the PR risky to ship as-is.apps/desktop/src-tauri/src/recording.rs(compute_zoom_amount fallback) andapps/desktop/src/routes/(window-chrome)/settings/experimental.tsx(slider cross-validation)Important Files Changed
AutoZoomConfigfields; introducescompute_zoom_amountfor spatial-intensity-based zoom, dead-zone grouping, double-click dedup, and right-click filtering. Critical bug: movement segments (empty positions) and single-click segments always receivemax_zoom_amountinstead of the previousAUTO_ZOOM_AMOUNT = 1.5×, causing a silent behavioural regression.apply_edge_snap_to_focusper-frame for both current and previous frame focus positions; correctly guards behindZoomMode::Autocheck and mirrors the pattern already used bySegmentsCursor. Segment lookup usesframe_time(output time), consistent with existing zoom rendering code.#[allow(dead_code)]to makeapply_edge_snap_to_focuspublic; no logic changes. The snap function correctly handles viewport bounds usingviewport_halfandsnap_thresholdderived from zoom amount andedge_snap_ratio.AutoZoomConfigstruct with camelCase serde, struct-level#[serde(default)]backed by a fullDefaultimpl; field-level#[serde(default = "...")]annotations on the four newer fields are redundant but harmless. Backward-compatible with existing stored recordings via serde defaults.AppHandleintogenerate_zoom_segments_from_clicksto passAutoZoomConfigfromGeneralSettingsStore. Error handling usesunwrap_or(None).unwrap_or_default()which silently swallows settings load errors, defaulting to edge snap enabled.auto_zoom_config: cap_project::AutoZoomConfigfield toGeneralSettingsStorewith#[serde(default)]and properDefaultinitialisation; straightforward, no issues.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Per-frame render call] --> B[zoom_focus_interpolator.interpolate\ncurrent_recording_time] B --> C{Active segment at\nframe_time?} C -- No segment --> E[Use focus as-is] C -- Manual mode --> E C -- Auto mode --> D[apply_edge_snap_to_focus\nfocus + segment.edge_snap_ratio] D --> F[zoom_focus passed to\nInterpolatedZoom] E --> F G[generate_zoom_segments_from_clicks_impl] --> H{config.ignore_right_clicks?} H -- yes --> I[filter cursor_num != 0] H -- no --> J[Sort by time_ms] I --> J J --> K{double_click_threshold_ms > 0?} K -- yes --> L[deduplicate double-clicks] K -- no --> M[Group clicks\ntime + spatial + dead_zone] L --> M M --> N[Build intervals with positions] N --> O[Add movement intervals\npositions = empty] O --> P[Merge overlapping intervals] P --> Q[compute_zoom_amount\npositions] Q --> R{positions.len < 2?} R -- yes\nmovements & single clicks --> S[return max_zoom_amount ⚠️] R -- no --> T[spatial spread → lerp\nmin..max_zoom_amount] S --> U[ZoomSegment\nedge_snap_ratio = 0.25 if enabled] T --> UComments Outside Diff (2)
apps/desktop/src-tauri/src/recording.rs, line 261-281 (link)max_zoom_amountcompute_zoom_amountreturnsconfig.max_zoom_amountwheneverpositions.len() < 2. Movement-triggered intervals are pushed withvec, so they will always hit this early return and zoom atmax_zoom_amount(2.5× by default). The previous behaviour wasAUTO_ZOOM_AMOUNT = 1.5×for all segments.This is a silent behavioural regression: any recording that had movement-triggered zoom segments will now render with a noticeably harder zoom (2.5× vs 1.5×). Single-click segments (one position) are also affected the same way.
Consider falling back to
config.zoom_amount(the general-purpose baseline) instead ofmax_zoom_amountwhen the spatial spread cannot be computed:Prompt To Fix With AI
apps/desktop/src-tauri/src/lib.rs, line 36-38 (link)unwrap_or(None).unwrap_or_default()discards anyErrfromGeneralSettingsStore::get. If the settings file is corrupted or unreadable, the function silently falls back toAutoZoomConfig::default(), which hasedge_snap_enabled: true. A user who has disabled edge snapping would unknowingly have it re-enabled on every editor reload until the settings file recovers.Consider propagating the error (or at least logging it) so the failure is visible:
Prompt To Fix With AI
Prompt To Fix All With AI
Last reviewed commit: "feat(rendering): act..."