diff --git a/app/capture.js b/app/capture.js index 9ef6467..99c286e 100644 --- a/app/capture.js +++ b/app/capture.js @@ -23,8 +23,6 @@ const { encodePng } = require('../core/png'); */ const CLICK_DEBOUNCE_MS = 700; -const CLICK_CAPTURE_CACHE_MS = 75; -const CLICK_CAPTURE_CACHE_MAX_AGE_MS = 400; const CLICK_CAPTURE_HIDE_DELAY_MS = 25; function hasBinary(name) { @@ -45,9 +43,6 @@ class CaptureService { this.session = null; // { guideId, paused, count, intervalSec } this.intervalTimer = null; this.clickWatcher = null; - this.captureCacheTimer = null; - this.captureCache = null; - this.captureCacheRunning = false; this.lastClickCapture = 0; this.shooting = false; } @@ -189,27 +184,15 @@ class CaptureService { const wasPaused = this.session.paused; this.session.paused = typeof force === 'boolean' ? force : !this.session.paused; // Starting/resuming tucks the window away again for clean shots (after - // a brief delay so the user sees it happen) and arms the click-capture - // cache so the very next click is captured instantly. + // a brief delay so the user sees it happen). if (wasPaused && !this.session.paused) { const win = this.getWindow(); - const arm = () => { + const tuck = () => { if (!this.session || this.session.paused) return; if (this.hiddenForSession && win && !win.isDestroyed() && win.isVisible()) win.hide(); - if (this.settings.get('capture.captureOutsideClicks') !== false && this.clickCaptureAvailable()) { - this.startClickCaptureCache(); - } }; - if (this.hiddenForSession && win && !win.isDestroyed()) setTimeout(arm, 400); - else arm(); - } else if (!wasPaused && this.session.paused) { - // Pausing stops the background cache refresh loop, but it does so by - // returning early (see startClickCaptureCache's refresh) without - // resetting captureCacheRunning or clearing captureCache. Without this - // call, resuming would find captureCacheRunning still true, so - // startClickCaptureCache() would no-op and every click afterwards - // would reuse the stale pre-pause frame instead of a fresh one. - this.stopClickCaptureCache(); + if (this.hiddenForSession && win && !win.isDestroyed()) setTimeout(tuck, 400); + else tuck(); } if (this.rebuildTrayMenu) this.rebuildTrayMenu(); this.notify('capture:state', this.state()); @@ -221,7 +204,6 @@ class CaptureService { this.intervalTimer = null; } this.stopClickWatcher(); - this.stopClickCaptureCache(); this.destroySessionTray(); this.session = null; if (this.hiddenForSession) { @@ -258,23 +240,18 @@ class CaptureService { try { const mode = this.settings.get('capture.mode') || 'fullscreen'; const grabMode = mode === 'region' ? 'fullscreen' : mode; - // The background refresh loop (startClickCaptureCache) keeps this - // updated every ~75ms; if it's gone stale (refresh errors silently and - // stops updating, or the cache was never refreshed after a resume), - // fall back to a fresh shot rather than reusing an old background. - const cacheFresh = this.captureCache && this.captureCache.mode === grabMode - && Date.now() - this.captureCache.capturedAt <= CLICK_CAPTURE_CACHE_MAX_AGE_MS; - const cached = trigger === 'click' && cacheFresh ? this.captureCache : null; - const finalResult = cached - ? this.storeFrameAsStep(this.session.guideId, grabMode, cached, clickPos) - : await this.shoot({ - guideId: this.session.guideId, - mode: grabMode, - delayMs: 0, - hideWindowDelayMs: trigger === 'click' ? CLICK_CAPTURE_HIDE_DELAY_MS : null, - refocus: false, // don't steal focus from the app the user is documenting - clickPos, - }); + // Always a fresh shot, started right now. Click captures pass the + // cursor position grabbed at the instant of the click (onOsClick), so + // the marker always lands on the screen the user actually clicked — + // never on a pre-click frame from some moment earlier. + const finalResult = await this.shoot({ + guideId: this.session.guideId, + mode: grabMode, + delayMs: 0, + hideWindowDelayMs: trigger === 'click' ? CLICK_CAPTURE_HIDE_DELAY_MS : null, + refocus: false, // don't steal focus from the app the user is documenting + clickPos, + }); if (finalResult.ok) { this.session.count += 1; this.notify('capture:added', { guideId: this.session.guideId, step: finalResult.step, trigger }); @@ -293,40 +270,6 @@ class CaptureService { // ---- click-triggered capture -------------------------------------------- - startClickCaptureCache() { - if (this.captureCacheRunning) return; - this.captureCacheRunning = true; - const refresh = async () => { - if (!this.session || this.session.paused || !this.captureCacheRunning) return; - try { - if (!this.shooting) { - const mode = this.settings.get('capture.mode') || 'fullscreen'; - const grabMode = mode === 'region' ? 'fullscreen' : mode; - const frame = await this.captureCurrentFrame(grabMode); - if (this.captureCacheRunning && this.session && !this.session.paused) { - this.captureCache = frame; - } - } - } catch { - // Cache misses are fine; click capture falls back to a fresh shot. - } finally { - if (this.session && !this.session.paused && this.captureCacheRunning) { - this.captureCacheTimer = setTimeout(refresh, CLICK_CAPTURE_CACHE_MS); - } - } - }; - this.captureCacheTimer = setTimeout(refresh, 0); - } - - stopClickCaptureCache() { - if (this.captureCacheTimer) { - clearTimeout(this.captureCacheTimer); - this.captureCacheTimer = null; - } - this.captureCacheRunning = false; - this.captureCache = null; - } - startClickWatcher() { this.stopClickWatcher(); try { @@ -383,7 +326,7 @@ while ($true) { this.lastClickCapture = now; // Grab the cursor position synchronously, right when the click is // detected, so the marker lands exactly where the user clicked even if - // the cached frame is a beat stale or a fresh shot takes a moment. + // the shot itself takes a moment to grab. const clickPos = screen.getCursorScreenPoint(); this.sessionCapture('click', clickPos).catch(() => {}); } diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 01a0073..30c8954 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -92,12 +92,13 @@ Initial release. literal text "undefined" by an old bug); a corrupted file is now treated as empty instead of crashing the dialog, and is overwritten with valid JSON the next time settings are saved. -- Click captures no longer reuse the same stale background screenshot - for every step (only the click marker moved). Pausing now fully resets - the click-capture cache so resuming starts a fresh background refresh - loop, and a cached frame older than 400ms (e.g. if the background - refresh silently stops working) is now discarded in favor of a fresh - screenshot. +- Click captures now always take a brand-new screenshot at the instant + of the click, with the click marker at the click-time cursor position. + Previously they reused a pre-click frame from a background cache that + could be stale by the time of the click — so the background image and + the click marker didn't always line up, and after pause/resume the + same frozen frame could be reused for every step with only the marker + moving. The pre-click frame cache has been removed entirely. ### Added (initial feature set) diff --git a/tests/unit/capture.test.js b/tests/unit/capture.test.js index 6c475e5..5974a83 100644 --- a/tests/unit/capture.test.js +++ b/tests/unit/capture.test.js @@ -50,127 +50,41 @@ test('click-triggered session capture uses the low-latency hide pause', async () }); }); -test('click-triggered session capture prefers the cached frame when ready', async () => { +test('every click capture takes a fresh shot — pre-grabbed frames are never reused', async () => { const service = makeService(); - service.settings.get = (key) => { - if (key === 'capture.mode') return 'fullscreen'; - if (key === 'capture.delayMs') return 0; - if (key === 'capture.clickMarker') return true; - if (key === 'capture.clickMarkerColor') return '#E5484D'; - if (key === 'editor.focusedViewDefaultForNewSteps') return false; - return null; - }; service.session = { guideId: 'guide-2', paused: false, count: 0, intervalSec: 0 }; - service.captureCache = { - mode: 'fullscreen', - png: Buffer.from('cached-png'), - size: { width: 120, height: 80 }, - display: { bounds: { x: 10, y: 20, width: 120, height: 80 } }, - cursor: { x: 70, y: 40 }, - capturedAt: Date.now(), + + let shots = 0; + service.captureCurrentFrame = async (mode) => { + shots += 1; + return { + mode, + png: Buffer.from(`frame-${shots}`), + size: { width: 100, height: 100 }, + display: { bounds: { x: 0, y: 0, width: 100, height: 100 } }, + cursor: { x: 50, y: 50 }, + capturedAt: Date.now(), + }; }; - let shootCalled = false; - service.shoot = async () => { - shootCalled = true; - throw new Error('fresh shot should not run when cache is ready'); - }; - - const added = []; - service.store.addStep = (guideId, fields, png, size) => { - added.push({ guideId, fields, png, size }); - return { stepId: 'step-2', ...fields }; - }; - service.notify = (channel, payload) => { - added.push({ channel, payload }); - }; - - const result = await service.sessionCapture('click'); - - assert.equal(result.ok, true); - assert.equal(shootCalled, false); - assert.equal(service.session.count, 1); - assert.equal(added[0].guideId, 'guide-2'); - assert.deepEqual(added[0].png, Buffer.from('cached-png')); - assert.deepEqual(added[0].size, { width: 120, height: 80 }); - assert.equal(added[0].fields.annotations.length, 1); - assert.equal(added[0].fields.annotations[0].type, 'oval'); -}); - -test('click-triggered capture marks the click-time cursor position, not the cached frame\'s (possibly stale) cursor', async () => { - const service = makeService(); - service.settings.get = (key) => { - if (key === 'capture.mode') return 'fullscreen'; - if (key === 'capture.delayMs') return 0; - if (key === 'capture.clickMarker') return true; - if (key === 'capture.clickMarkerColor') return '#E5484D'; - if (key === 'editor.focusedViewDefaultForNewSteps') return false; - return null; - }; - service.session = { guideId: 'guide-3', paused: false, count: 0, intervalSec: 0 }; - service.captureCache = { - mode: 'fullscreen', - png: Buffer.from('cached-png'), - size: { width: 120, height: 80 }, - display: { bounds: { x: 0, y: 0, width: 120, height: 80 } }, - // Stale cursor position from the cache-refresh loop, well outside the - // display — if this were used for the marker, no annotation would be - // placed at all. - cursor: { x: 9999, y: 9999 }, - capturedAt: Date.now(), - }; - - service.shoot = async () => { - throw new Error('fresh shot should not run when cache is ready'); - }; - - let added = null; - service.store.addStep = (guideId, fields, png, size) => { - added = { guideId, fields, png, size }; - return { stepId: 'step-3', ...fields }; + const captured = []; + service.store.addStep = (guideId, fields, png) => { + captured.push(png.toString()); + return { stepId: `step-${captured.length}` }; }; service.notify = () => {}; - // The user clicked dead center of the display. - const result = await service.sessionCapture('click', { x: 60, y: 40 }); + await service.sessionCapture('click', { x: 10, y: 10 }); + await service.sessionCapture('click', { x: 20, y: 20 }); + service.togglePause(true); + service.togglePause(false); + await service.sessionCapture('click', { x: 30, y: 30 }); - assert.equal(result.ok, true); - assert.equal(added.fields.annotations.length, 1); - const marker = added.fields.annotations[0]; - assert.equal(marker.type, 'oval'); - const d = 0.035; - assert.ok(Math.abs(marker.x - (0.5 - d / 2)) < 1e-9); - assert.ok(Math.abs(marker.y - (0.5 - (d * 120 / 80) / 2)) < 1e-9); + assert.deepEqual(captured, ['frame-1', 'frame-2', 'frame-3'], + 'each click must capture a brand-new frame, including after pause/resume'); }); -test('click-triggered session capture falls back to a fresh shot when the cached frame is stale', async () => { - const service = makeService(); - service.session = { guideId: 'guide-stale', paused: false, count: 0, intervalSec: 0 }; - // A frame that's well past the cache's max age — e.g. the background - // refresh loop died (errored silently, or never restarted after a - // pause/resume) and left a frozen, increasingly-stale frame behind. - service.captureCache = { - mode: 'fullscreen', - png: Buffer.from('stale-png'), - size: { width: 120, height: 80 }, - display: { bounds: { x: 0, y: 0, width: 120, height: 80 } }, - cursor: { x: 60, y: 40 }, - capturedAt: Date.now() - 10_000, - }; - - let shootCalled = false; - service.shoot = async () => { - shootCalled = true; - return { ok: true, step: { stepId: 'fresh-step' } }; - }; - - const result = await service.sessionCapture('click', { x: 1, y: 1 }); - - assert.equal(result.ok, true); - assert.equal(shootCalled, true, 'a stale cached frame must not be reused'); -}); - -test('live-shot click capture also marks the click-time cursor position', async () => { +test('click capture marks the click-time cursor position', async () => { const service = makeService(); service.settings.get = (key) => { if (key === 'capture.mode') return 'fullscreen'; @@ -206,7 +120,7 @@ test('live-shot click capture also marks the click-time cursor position', async assert.equal(added.fields.annotations[0].type, 'oval'); }); -test('a new session starts paused and does not hide the window or arm the click cache until "Start recording" is pressed', async () => { +test('a new session starts paused and does not hide the window until "Start recording" is pressed', async () => { const service = makeService(); const win = { destroyed: false, visible: true, minimized: false, hidden: 0, shown: 0, @@ -222,8 +136,6 @@ test('a new session starts paused and does not hide the window or arm the click }; service.getWindow = () => win; service.clickCaptureAvailable = () => true; - let cacheStarted = 0; - service.startClickCaptureCache = () => { cacheStarted += 1; }; try { service.startSession('guide-5'); @@ -231,7 +143,6 @@ test('a new session starts paused and does not hide the window or arm the click assert.equal(service.session.paused, true, 'sessions start paused'); assert.equal(service.state().paused, true); assert.equal(win.hidden, 0, 'window must stay visible until recording starts'); - assert.equal(cacheStarted, 0, 'click-capture cache must not start before recording starts'); // User clicks "Start recording" (the resume action). service.togglePause(false); @@ -240,7 +151,6 @@ test('a new session starts paused and does not hide the window or arm the click await new Promise((r) => setTimeout(r, 450)); assert.equal(win.hidden, 1, 'window hides once recording actually starts'); - assert.equal(cacheStarted, 1, 'click-capture cache is armed once recording starts'); } finally { service.finishSession(); }