-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat: configurable auto-zoom settings with UI sliders #1663
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
5d54edd
ae39052
04a7fed
b893118
c866805
680a8e5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,50 @@ | ||
| import { Slider as KSlider } from "@kobalte/core/slider"; | ||
| import { WebviewWindow } from "@tauri-apps/api/webviewWindow"; | ||
| import { type } from "@tauri-apps/plugin-os"; | ||
| import { createResource, Show } from "solid-js"; | ||
| import { createStore } from "solid-js/store"; | ||
|
|
||
| import { generalSettingsStore } from "~/store"; | ||
| import { commands, type GeneralSettingsStore } from "~/utils/tauri"; | ||
| import { | ||
| type AutoZoomConfig, | ||
| commands, | ||
| type GeneralSettingsStore, | ||
| } from "~/utils/tauri"; | ||
| import { ToggleSettingItem } from "./Setting"; | ||
|
|
||
| function SettingSlider(props: { | ||
| label: string; | ||
| value: number; | ||
| onChange: (v: number) => void; | ||
| min: number; | ||
| max: number; | ||
| step: number; | ||
| format?: (v: number) => string; | ||
| }) { | ||
| return ( | ||
| <div class="space-y-1.5"> | ||
| <div class="flex justify-between items-center text-sm"> | ||
| <span class="text-gray-11">{props.label}</span> | ||
| <span class="text-gray-12 font-medium"> | ||
| {props.format ? props.format(props.value) : props.value} | ||
| </span> | ||
| </div> | ||
| <KSlider | ||
| value={[props.value]} | ||
| onChange={(v) => props.onChange(v[0])} | ||
| minValue={props.min} | ||
| maxValue={props.max} | ||
| step={props.step} | ||
| > | ||
| <KSlider.Track class="h-[0.3rem] cursor-pointer relative bg-gray-4 rounded-full w-full"> | ||
| <KSlider.Fill class="absolute h-full rounded-full bg-blue-9" /> | ||
| <KSlider.Thumb class="block size-4 rounded-full bg-white border-2 border-blue-9 -top-[0.35rem] outline-none" /> | ||
| </KSlider.Track> | ||
| </KSlider> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| export default function ExperimentalSettings() { | ||
| const [store] = createResource(() => generalSettingsStore.get()); | ||
|
|
||
|
|
@@ -27,9 +65,32 @@ function Inner(props: { initialStore: GeneralSettingsStore | null }) { | |
| enableNativeCameraPreview: false, | ||
| autoZoomOnClicks: false, | ||
| custom_cursor_capture2: true, | ||
| autoZoomConfig: { | ||
| zoomAmount: 1.5, | ||
| clickGroupTimeThreshold: 2.5, | ||
| clickGroupSpatialThreshold: 0.15, | ||
| clickPrePadding: 0.4, | ||
| clickPostPadding: 1.8, | ||
| movementPrePadding: 0.3, | ||
| movementPostPadding: 1.5, | ||
| mergeGapThreshold: 0.8, | ||
| minSegmentDuration: 1.0, | ||
| movementEventDistanceThreshold: 0.02, | ||
| movementWindowDistanceThreshold: 0.08, | ||
| }, | ||
| }, | ||
| ); | ||
|
|
||
| const handleConfigChange = <K extends keyof AutoZoomConfig>( | ||
| key: K, | ||
| value: AutoZoomConfig[K], | ||
| ) => { | ||
| setSettings("autoZoomConfig", key, value); | ||
| generalSettingsStore.set({ | ||
| autoZoomConfig: { ...settings.autoZoomConfig, [key]: value }, | ||
| }); | ||
| }; | ||
|
|
||
| const handleChange = async <K extends keyof typeof settings>( | ||
| key: K, | ||
| value: (typeof settings)[K], | ||
|
|
@@ -95,6 +156,46 @@ function Inner(props: { initialStore: GeneralSettingsStore | null }) { | |
| }} | ||
| /> | ||
| </div> | ||
| <Show when={settings.autoZoomOnClicks}> | ||
| <div class="px-3 py-3 space-y-4"> | ||
| <SettingSlider | ||
| label="Zoom Amount" | ||
| value={settings.autoZoomConfig?.zoomAmount ?? 1.5} | ||
| onChange={(v) => handleConfigChange("zoomAmount", v)} | ||
| min={1.0} | ||
| max={4.0} | ||
| step={0.1} | ||
| format={(v) => `${v.toFixed(1)}x`} | ||
| /> | ||
| <SettingSlider | ||
| label="Sensitivity" | ||
| value={ | ||
| settings.autoZoomConfig?.movementWindowDistanceThreshold ?? | ||
| 0.08 | ||
| } | ||
| onChange={(v) => | ||
| handleConfigChange("movementWindowDistanceThreshold", v) | ||
| } | ||
| min={0.02} | ||
| max={0.2} | ||
| step={0.01} | ||
| format={(v) => { | ||
| if (v <= 0.05) return "High"; | ||
| if (v <= 0.12) return "Medium"; | ||
| return "Low"; | ||
| }} | ||
| /> | ||
|
Comment on lines
+170
to
+187
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The "Sensitivity" slider adjusts let significant_movement = distance >= movement_event_distance_threshold // fixed: 0.02
|| window_distance >= movement_window_distance_threshold; // slider-controlledThis means even at "Low" sensitivity ( To make the slider fully effective, both thresholds should scale together. You could either:
Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/desktop/src/routes/(window-chrome)/settings/experimental.tsx
Line: 170-187
Comment:
**"Sensitivity" slider only controls half the detection condition**
The "Sensitivity" slider adjusts `movementWindowDistanceThreshold` (cumulative window distance) but leaves `movementEventDistanceThreshold` (per-frame distance) fixed at `0.02`. In `generate_zoom_segments_from_clicks_impl`, movement detection is an **OR** condition:
```rust
let significant_movement = distance >= movement_event_distance_threshold // fixed: 0.02
|| window_distance >= movement_window_distance_threshold; // slider-controlled
```
This means even at "Low" sensitivity (`movementWindowDistanceThreshold = 0.2`), any single frame where the cursor travels ≥ 2% of normalised screen width still triggers a zoom. In practice the per-frame branch can dominate, making the "Low" end of the slider much weaker than users expect.
To make the slider fully effective, both thresholds should scale together. You could either:
- Expose `movementEventDistanceThreshold` as part of the sensitivity curve, or
- Derive `movementEventDistanceThreshold` proportionally from the `movementWindowDistanceThreshold` value rather than keeping it at its hardcoded default.
How can I resolve this? If you propose a fix, please make it concise. |
||
| <SettingSlider | ||
| label="Smoothing" | ||
| value={settings.autoZoomConfig?.mergeGapThreshold ?? 0.8} | ||
| onChange={(v) => handleConfigChange("mergeGapThreshold", v)} | ||
| min={0.2} | ||
| max={2.0} | ||
| step={0.1} | ||
| format={(v) => `${v.toFixed(1)}s`} | ||
| /> | ||
| </div> | ||
| </Show> | ||
| </div> | ||
| </div> | ||
| </div> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok().flatten()unwrap_or(None)on aResult<Option<T>>is non-idiomatic and silently discards the error. The codebase already uses a cleaner pattern elsewhere (e.g.recording.rs:598uses.ok().flatten()):This makes the intent explicit — convert
ErrtoNone, then fall back to defaults — and is consistent with the rest of the file.Prompt To Fix With AI
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!