feat: Implement adjustable region selector handles (Issue #49)#50
feat: Implement adjustable region selector handles (Issue #49)#50rautelaKamal wants to merge 11 commits intoKartikLabhshetwar:mainfrom
Conversation
- Added region-selector to allowed windows in capabilities/default.json - Added core:window:allow-set-resizable permission - Increased event emission delay from 100ms to 800ms for reliable React mount - Made window visible by default for easier development testing - Fixes KartikLabhshetwar#49
|
@rautelaKamal is attempting to deploy a commit to the knox projects Team on Vercel. A member of the Team first needs to authorize it. |
src-tauri/src/commands.rs
Outdated
| let window = tauri::WebviewWindowBuilder::new( | ||
| &app_handle, | ||
| window_label, | ||
| tauri::WebviewUrl::App("/?region-selector=1".into()), |
There was a problem hiding this comment.
WebviewUrl::App("/?region-selector=1"…) looks inconsistent with the other windows (index.html?overlay=1). Using a root path can be flaky once bundled.
| tauri::WebviewUrl::App("/?region-selector=1".into()), | |
| tauri::WebviewUrl::App("index.html?region-selector=1".into()), |
src-tauri/src/commands.rs
Outdated
|
|
||
| // Give the React component time to mount and set up event listeners | ||
| // Increased from 100ms to 800ms to ensure reliable event delivery | ||
| std::thread::sleep(std::time::Duration::from_millis(800)); |
There was a problem hiding this comment.
This runs inside an async command; std::thread::sleep will block the runtime thread. Prefer an async sleep.
| std::thread::sleep(std::time::Duration::from_millis(800)); | |
| tauri::async_runtime::sleep(std::time::Duration::from_millis(800)).await; |
|
|
||
| /// Clean up a temporary file | ||
| #[tauri::command] | ||
| pub async fn cleanup_temp_file(path: String) -> Result<(), String> { |
There was a problem hiding this comment.
cleanup_temp_file currently lets the frontend delete any path on disk. At minimum, consider restricting deletes to the OS temp dir (after canonicalization) to avoid accidentally removing user files.
| pub async fn cleanup_temp_file(path: String) -> Result<(), String> { | |
| pub async fn cleanup_temp_file(path: String) -> Result<(), String> { | |
| let temp_dir = std::env::temp_dir().canonicalize().unwrap_or_else(|_| std::env::temp_dir()); | |
| let canonical = std::path::PathBuf::from(&path) | |
| .canonicalize() | |
| .map_err(|e| format!("Failed to resolve temp file path: {}", e))?; | |
| if !canonical.starts_with(&temp_dir) { | |
| return Err("Refusing to delete non-temp file".to_string()); | |
| } | |
| std::fs::remove_file(&canonical).map_err(|e| format!("Failed to remove temp file: {}", e)) | |
| } |
src-tauri/src/lib.rs
Outdated
| .resizable(true) | ||
| .decorations(true) | ||
| .visible(false) // Start hidden | ||
| .visible(true) // Show on startup for development |
There was a problem hiding this comment.
This flips the main window to visible unconditionally. If this is meant to be dev-only, it’s safer to gate it so release builds keep the previous “start hidden” behavior.
| .visible(true) // Show on startup for development | |
| .visible(cfg!(debug_assertions)) // Show on startup for development |
| // Clicking on a handle - start dragging it | ||
| dragHandleRef.current = handle; | ||
| isSelectingRef.current = true; | ||
| } else if (hasSelectionRef.current) { |
There was a problem hiding this comment.
This branch says “clicking outside selection”, but it triggers for any click that isn’t on a handle (including inside the selection). That makes it easy to accidentally lose the adjusted region.
One option is to only reset when the click is actually outside the current bounds.
| y: Math.round(region.y), | ||
| width: Math.round(region.width), | ||
| height: Math.round(region.height), | ||
| saveDir: "", // Will use temp dir or get from settings |
There was a problem hiding this comment.
saveDir: "" ends up writing the cropped region into the process CWD (PathBuf::from("")). Passing the same temp dir you used for the monitor screenshots (or including saveDir in the region-selector-show payload) would be safer.
Also: on cancel you only delete screenshotPath even though monitorShots includes multiple temp files; consider cleaning those up too.
src/App.tsx
Outdated
| errStr.includes("__TAURI_INTERNALS__") || errStr.includes("undefined"); | ||
|
|
||
| // If not an initialization error, fail immediately | ||
| if (!isTauriInitError && i === 0) { |
There was a problem hiding this comment.
Minor: invokeWithRetries retries even for non-init errors after the first attempt, and errStr.includes("undefined") is pretty broad. Might be less surprising to bail immediately on any non-init error.
| if (!isTauriInitError && i === 0) { | |
| if (!isTauriInitError) { | |
| console.error(`Command ${command} failed:`, err); | |
| return null; | |
| } |
| ctx.clearRect(0, 0, bounds.width, bounds.height); | ||
|
|
||
| if (isSelectingRef.current || needsUpdateRef.current) { | ||
| if (isSelectingRef.current || hasSelectionRef.current || needsUpdateRef.current) { |
There was a problem hiding this comment.
Now that hasSelectionRef.current keeps this branch true even when the selection is static, the canvas will re-draw every RAF indefinitely. Might be worth short-circuiting when !needsUpdateRef.current && !isSelectingRef.current (and setting needsUpdateRef.current = true once on mount / when entering adjustment mode) so it only re-renders on interaction.
| const monitorShot = screenshotData.monitorShots.find(s => s.path === screenshotData.screenshotPath) || screenshotData.monitorShots[0]; | ||
| // We'll pass the directory of the monitor shot. | ||
| // However, the backend's capture_region expects a directory to save TO. | ||
| // If we pass empty string, it saves to CWD which is bad. | ||
| // We can construct the path using a known safe directory if possible, but we don't have direct fs access here easily. | ||
| // Best effort: extract directory from the path. | ||
| // Since we can't use node's path module, we do string manipulation. | ||
| // Assume standard path separators. | ||
| let saveDir = ""; | ||
| const lastSepIndex = Math.max(monitorShot.path.lastIndexOf("/"), monitorShot.path.lastIndexOf("\\")); | ||
| if (lastSepIndex !== -1) { | ||
| saveDir = monitorShot.path.substring(0, lastSepIndex); | ||
| } | ||
|
|
||
| // Call backend to crop the screenshot | ||
| const croppedPath = await invoke<string>("capture_region", { | ||
| screenshotPath: screenshotData.screenshotPath, | ||
| x: Math.round(region.x), | ||
| y: Math.round(region.y), | ||
| width: Math.round(region.width), | ||
| height: Math.round(region.height), | ||
| saveDir: saveDir, | ||
| }); |
There was a problem hiding this comment.
It looks like the selection coordinates are in the global monitor coordinate space, but capture_region crops relative to the specific screenshot you pass. As written this always crops from screenshotData.screenshotPath, so selecting on a secondary monitor (or negative coords) will crop the wrong pixels.
| const monitorShot = screenshotData.monitorShots.find(s => s.path === screenshotData.screenshotPath) || screenshotData.monitorShots[0]; | |
| // We'll pass the directory of the monitor shot. | |
| // However, the backend's capture_region expects a directory to save TO. | |
| // If we pass empty string, it saves to CWD which is bad. | |
| // We can construct the path using a known safe directory if possible, but we don't have direct fs access here easily. | |
| // Best effort: extract directory from the path. | |
| // Since we can't use node's path module, we do string manipulation. | |
| // Assume standard path separators. | |
| let saveDir = ""; | |
| const lastSepIndex = Math.max(monitorShot.path.lastIndexOf("/"), monitorShot.path.lastIndexOf("\\")); | |
| if (lastSepIndex !== -1) { | |
| saveDir = monitorShot.path.substring(0, lastSepIndex); | |
| } | |
| // Call backend to crop the screenshot | |
| const croppedPath = await invoke<string>("capture_region", { | |
| screenshotPath: screenshotData.screenshotPath, | |
| x: Math.round(region.x), | |
| y: Math.round(region.y), | |
| width: Math.round(region.width), | |
| height: Math.round(region.height), | |
| saveDir: saveDir, | |
| }); | |
| const centerX = region.x + region.width / 2; | |
| const centerY = region.y + region.height / 2; | |
| const targetShot = | |
| screenshotData.monitorShots.find((s) => { | |
| return ( | |
| centerX >= s.x && | |
| centerX <= s.x + s.width && | |
| centerY >= s.y && | |
| centerY <= s.y + s.height | |
| ); | |
| }) ?? screenshotData.monitorShots[0]; | |
| let saveDir = ""; | |
| const lastSepIndex = Math.max( | |
| targetShot.path.lastIndexOf("/"), | |
| targetShot.path.lastIndexOf("\\") | |
| ); | |
| if (lastSepIndex !== -1) { | |
| saveDir = targetShot.path.substring(0, lastSepIndex); | |
| } | |
| const scale = targetShot.scale_factor; | |
| const localX = Math.max(0, region.x - targetShot.x); | |
| const localY = Math.max(0, region.y - targetShot.y); | |
| const localWidth = Math.max(0, Math.min(region.width, targetShot.width - localX)); | |
| const localHeight = Math.max(0, Math.min(region.height, targetShot.height - localY)); | |
| // Call backend to crop the screenshot | |
| const croppedPath = await invoke<string>("capture_region", { | |
| screenshotPath: targetShot.path, | |
| x: Math.round(localX * scale), | |
| y: Math.round(localY * scale), | |
| width: Math.round(localWidth * scale), | |
| height: Math.round(localHeight * scale), | |
| saveDir, | |
| }); |
| saveDir: saveDir, | ||
| }); | ||
|
|
||
| // Emit event back to main window with the cropped image path |
There was a problem hiding this comment.
Small cleanup: on success we keep the full monitor screenshots around. Might be worth deleting them after capture_region succeeds (mirrors the cancel path).
| // Emit event back to main window with the cropped image path | |
| // Clean up temp monitor screenshots now that we have the crop | |
| await Promise.all( | |
| screenshotData.monitorShots.map((shot) => | |
| invoke("cleanup_temp_file", { path: shot.path }).catch((e) => | |
| console.error("Failed to cleanup file:", shot.path, e) | |
| ) | |
| ) | |
| ); | |
| // Emit event back to main window with the cropped image path |
| }) ?? screenshotData.monitorShots[0]; | ||
|
|
||
| // Calculate local coordinates relative to that monitor | ||
| const localX = Math.max(0, region.x - targetShot.x); |
There was a problem hiding this comment.
If the selection is partially outside the target monitor (or the handles allow dragging beyond bounds), targetShot.width - localX can go negative and localX/localY can exceed the screenshot dimensions. Clamping here avoids sending negative/oversized crop params to the backend.
| const localX = Math.max(0, region.x - targetShot.x); | |
| const unclampedX = region.x - targetShot.x; | |
| const unclampedY = region.y - targetShot.y; | |
| const localX = Math.min(Math.max(0, unclampedX), targetShot.width); | |
| const localY = Math.min(Math.max(0, unclampedY), targetShot.height); | |
| const localWidth = Math.max(0, Math.min(region.width, targetShot.width - localX)); | |
| const localHeight = Math.max(0, Math.min(region.height, targetShot.height - localY)); | |
| const scale = targetShot.scale_factor; |
| const centerY = region.y + region.height / 2; | ||
|
|
||
| // Find which monitor the selection center is on | ||
| const targetShot = |
There was a problem hiding this comment.
Small defensive guard: if monitorShots is ever empty (backend returns []), the ?? monitorShots[0] fallback will be undefined and crash on targetShot.x.
| const targetShot = | |
| if (screenshotData.monitorShots.length === 0) { | |
| throw new Error("No monitor screenshots available"); | |
| } | |
| // Find which monitor the selection center is on |
| if (width > 10 && height > 10) { | ||
| // Valid selection - enter adjustment mode | ||
| hasSelectionRef.current = true; | ||
| setInstructionText("Drag handles to adjust · ENTER to confirm · ESC to cancel"); | ||
| needsUpdateRef.current = true; |
There was a problem hiding this comment.
Handle-resize logic assumes startRef/currentRef map to specific corners, but after the initial drag they can be inverted (e.g. drag bottom-right -> top-left). Normalizing once when entering adjustment mode keeps corner/edge handles consistent.
| if (width > 10 && height > 10) { | |
| // Valid selection - enter adjustment mode | |
| hasSelectionRef.current = true; | |
| setInstructionText("Drag handles to adjust · ENTER to confirm · ESC to cancel"); | |
| needsUpdateRef.current = true; | |
| if (width > 10 && height > 10) { | |
| // Normalize refs so start is always top-left and current is bottom-right. | |
| startRef.current = { x, y }; | |
| currentRef.current = { x: x + width, y: y + height }; | |
| // Valid selection - enter adjustment mode | |
| hasSelectionRef.current = true; | |
| setInstructionText("Drag handles to adjust · ENTER to confirm · ESC to cancel"); | |
| needsUpdateRef.current = true; |
| const setupListener = async () => { | ||
| const { listen } = await import("@tauri-apps/api/event"); | ||
| unlisten = await listen<{ path: string }>("capture-complete", async (event) => { | ||
| const screenshotPath = event.payload.path; | ||
| setIsCapturing(false); | ||
|
|
||
| try { | ||
| // Handle the captured region - set it as temp screenshot and open editor | ||
| setTempScreenshotPath(screenshotPath); | ||
| setMode("editing"); | ||
| await invoke("play_screenshot_sound"); | ||
| } catch (err) { | ||
| console.error("Failed to process region capture:", err); | ||
| setError( | ||
| `Failed to process capture: ${err instanceof Error ? err.message : String(err)}` | ||
| ); | ||
| } | ||
| }); | ||
| }; |
There was a problem hiding this comment.
setupListener() is async and its rejection is currently unhandled. Wrapping in a try/catch avoids silent failures if the dynamic import or listen() throws.
| const setupListener = async () => { | |
| const { listen } = await import("@tauri-apps/api/event"); | |
| unlisten = await listen<{ path: string }>("capture-complete", async (event) => { | |
| const screenshotPath = event.payload.path; | |
| setIsCapturing(false); | |
| try { | |
| // Handle the captured region - set it as temp screenshot and open editor | |
| setTempScreenshotPath(screenshotPath); | |
| setMode("editing"); | |
| await invoke("play_screenshot_sound"); | |
| } catch (err) { | |
| console.error("Failed to process region capture:", err); | |
| setError( | |
| `Failed to process capture: ${err instanceof Error ? err.message : String(err)}` | |
| ); | |
| } | |
| }); | |
| }; | |
| const setupListener = async () => { | |
| try { | |
| const { listen } = await import("@tauri-apps/api/event"); | |
| unlisten = await listen<{ path: string }>("capture-complete", async (event) => { | |
| const screenshotPath = event.payload.path; | |
| setIsCapturing(false); | |
| try { | |
| // Handle the captured region - set it as temp screenshot and open editor | |
| setTempScreenshotPath(screenshotPath); | |
| setMode("editing"); | |
| await invoke("play_screenshot_sound"); | |
| } catch (err) { | |
| console.error("Failed to process region capture:", err); | |
| setError( | |
| `Failed to process capture: ${err instanceof Error ? err.message : String(err)}` | |
| ); | |
| } | |
| }); | |
| } catch (err) { | |
| console.error("Failed to set up capture-complete listener:", err); | |
| } | |
| }; |
| if (captureMode === "region") { | ||
| console.log("[App.tsx] Region capture triggered, tempDir:", currentTempDir); | ||
| setIsCapturing(true); | ||
| try { | ||
| console.log("[App.tsx] Calling open_region_selector..."); | ||
| await invoke("open_region_selector", { | ||
| saveDir: currentTempDir, | ||
| }); | ||
| console.log("[App.tsx] open_region_selector succeeded"); | ||
| // Don't proceed - the region selector window will handle completion | ||
| // and emit a "capture-complete" event when done | ||
| return; | ||
| } catch (err) { | ||
| console.error("[App.tsx] Region selector error:", err); | ||
| console.error("Region selector failed:", err); | ||
| setError( | ||
| `Failed to open region selector: ${err instanceof Error ? err.message : String(err)}` | ||
| ); | ||
| setIsCapturing(false); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Nit: setIsCapturing(true) is already called before this branch, and the debug logs / duplicate console.error add noise in normal usage.
| if (captureMode === "region") { | |
| console.log("[App.tsx] Region capture triggered, tempDir:", currentTempDir); | |
| setIsCapturing(true); | |
| try { | |
| console.log("[App.tsx] Calling open_region_selector..."); | |
| await invoke("open_region_selector", { | |
| saveDir: currentTempDir, | |
| }); | |
| console.log("[App.tsx] open_region_selector succeeded"); | |
| // Don't proceed - the region selector window will handle completion | |
| // and emit a "capture-complete" event when done | |
| return; | |
| } catch (err) { | |
| console.error("[App.tsx] Region selector error:", err); | |
| console.error("Region selector failed:", err); | |
| setError( | |
| `Failed to open region selector: ${err instanceof Error ? err.message : String(err)}` | |
| ); | |
| setIsCapturing(false); | |
| return; | |
| } | |
| } | |
| if (captureMode === "region") { | |
| try { | |
| await invoke("open_region_selector", { | |
| saveDir: currentTempDir, | |
| }); | |
| // Don't proceed - the region selector window will handle completion | |
| // and emit a "capture-complete" event when done | |
| return; | |
| } catch (err) { | |
| console.error("Failed to open region selector:", err); | |
| setError( | |
| `Failed to open region selector: ${err instanceof Error ? err.message : String(err)}` | |
| ); | |
| setIsCapturing(false); | |
| return; | |
| } | |
| } |
| const [isReady, setIsReady] = useState(false); | ||
|
|
||
| useEffect(() => { | ||
| console.log("[RegionSelectorWindow] Component mounted, configuring window..."); |
There was a problem hiding this comment.
There’s a lot of verbose console.log in this window (including the checkmark). Might be worth gating these behind import.meta.env.DEV (or removing) so release builds don’t spam the console.
src/App.tsx
Outdated
| if (!mounted) { u8(); return; } else { unlisten8 = u8; } | ||
| }; | ||
|
|
||
| setupListeners(); |
There was a problem hiding this comment.
setupListeners is async and currently invoked without handling rejections, so a failed listen() can turn into an unhandled promise rejection.
| setupListeners(); | |
| void setupListeners().catch((err) => console.error("Failed to set up tray listeners:", err)); |
src/components/RegionSelector.tsx
Outdated
| // Check if inside selection | ||
| const { x, y, width, height } = getSelectionBounds(); | ||
| if (e.clientX >= x && e.clientX <= x + width && e.clientY >= y && e.clientY <= y + height) { | ||
| canvas.style.cursor = 'move'; |
There was a problem hiding this comment.
This sets the cursor to move when hovering inside the selection, but dragging inside the selection currently does nothing (handleMouseDown returns early). Might be less confusing to keep a neutral cursor until move-drag is implemented.
| canvas.style.cursor = 'move'; | |
| canvas.style.cursor = 'default'; |
|
|
||
| act(() => { | ||
| editorActions.setPaddingTransient(75); | ||
| editorActions.setAllPaddingTransient(75); |
There was a problem hiding this comment.
Minor naming: this block now uses setAllPaddingTransient, so it might be worth renaming the describe("setPaddingTransient"...) heading to match the behavior (makes failures easier to interpret).
|
|
||
| const state = useEditorStore.getState(); | ||
| expect(state.settings.padding).toBe(150); | ||
| expect(state.settings.paddingTop).toBe(150); |
There was a problem hiding this comment.
Since the API is setAllPadding, it might be good for this test to assert the other sides too (helps catch a regression where only paddingTop changes).
| expect(state.settings.paddingTop).toBe(150); | |
| expect(state.settings.paddingTop).toBe(150); | |
| expect(state.settings.paddingRight).toBe(150); | |
| expect(state.settings.paddingBottom).toBe(150); | |
| expect(state.settings.paddingLeft).toBe(150); |
| "@vitejs/plugin-react": "^4.7.0", | ||
| "@vitest/coverage-v8": "^4.0.17", | ||
| "autoprefixer": "^10.4.23", | ||
| "happy-dom": "^20.6.1", |
There was a problem hiding this comment.
happy-dom@20.x requires Node >= 20 (per its engines). Might be worth double-checking CI/dev tooling is pinned to Node 20+ so installs/tests don’t start failing unexpectedly.
…ts, and specify node version
src/App.tsx
Outdated
| }; | ||
|
|
||
| setupListeners(); | ||
| setupListeners().catch((err) => |
There was a problem hiding this comment.
If you have no-floating-promises enabled, it may still complain about this call even with .catch. Prefixing with void makes the intent explicit.
| setupListeners().catch((err) => | |
| void setupListeners().catch((err) => | |
| console.error("Failed to set up tray listeners:", err) | |
| ); |
package.json
Outdated
| }, | ||
| "packageManager": "pnpm@10.28.0+sha512.05df71d1421f21399e053fde567cea34d446fa02c76571441bfc1c7956e98e363088982d940465fd34480d4d90a0668bc12362f8aa88000a64e83d0b0e47be48" | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Minor nit: package.json is missing a trailing newline (shows up as \\ No newline at end of file in the diff). Adding one avoids noisy diffs.
| } | |
| } |
This PR implements the adjustable handles feature requested in #49.
Features Added:
Interactive resize handles (4 corners + 4 edges)
"Adjustment mode" - selection doesn't capture immediately
Keyboard support: Enter to confirm, Escape to cancel
Double-click inside selection to confirm
Visual feedback and dynamic instructions
Bug Fixes Included:
Added missing region-selector window permissions
Fixed race condition where React component didn't mount before receiving data (increased delay)
Added allow-set-resizable permission
Screenshots: