diff --git a/app/capture.js b/app/capture.js index 7ae7224..7d7db89 100644 --- a/app/capture.js +++ b/app/capture.js @@ -55,6 +55,10 @@ const DEFAULT_CLICK_DEBOUNCE_MS = 200; // How long a Linux raw button event waits for its regular twin (the // representation that carries root coordinates) before firing without them. const LINUX_CLICK_TWIN_MS = 25; +// Longest the window stays visible warming up the recorder at recording +// start. A slow capture-stream start (Windows can take several seconds) must +// not keep the window up and recording un-armed indefinitely. +const WARMUP_MAX_MS = 1500; // Idle gap between legacy frame-loop grabs. Must stay well above zero: // grabbing back-to-back starves the main-process event loop, which delays // delivery of click events from the OS watcher by whole seconds. (The @@ -126,6 +130,10 @@ class CaptureService { this.lastAcceptedClickByButton = new Map(); this.streamBackend = null; this.streamBackendStarting = false; + this.captureGen = 0; // bumped on stop to invalidate in-flight backend starts + // True only while a resume is warming up (window still visible, buffer + // not yet primed). Clicks are ignored until it clears — see armRecording. + this.warmingUp = false; } state() { @@ -317,6 +325,7 @@ class CaptureService { if (wasPaused && !this.session.paused) { this.armRecording(); } else if (!wasPaused && this.session.paused) { + this.warmingUp = false; // cancel any in-flight warmup this.stopFrameLoop(); this.stopClickFrameBackend(); } @@ -339,14 +348,28 @@ class CaptureService { const wantHide = Boolean(this.hiddenForSession && win && !win.isDestroyed()); const recorderWanted = this.settings.get('capture.captureOutsideClicks') !== false && this.clickCaptureAvailable(); + // Recording is not "live" until the window is hidden and the buffer is + // primed. While warming up, the window is still visible and over the + // user's work, so clicks in this period are ignored (onOsClick checks + // warmingUp) instead of being skipped erratically or shot post-click — + // the bug that made a restarted recording "stop after one click". + this.warmingUp = Boolean(wantHide || recorderWanted); + const settleMs = Number(this.settings.get('capture.postHideSettleMs')); const run = async () => { - if (!this.session || this.session.paused) return; + if (!this.session || this.session.paused) { this.warmingUp = false; return; } const startedAt = Date.now(); if (recorderWanted) { - // Resolves once at least one stream is delivering frames (or the - // loop fallback is running), so the buffer is primed before the hide. - try { await this.startClickFrameBackend(); } catch { /* falls back internally */ } - if (!this.session || this.session.paused) return; + // Warm the recorder, but never let a slow backend start (it waits up + // to several seconds for the capture stream) keep the window visible + // and recording un-armed. Cap the wait; if it isn't ready by then, + // hide anyway and let the first click or two take the fresh-shot + // fallback while the stream finishes coming up in the background. + const warm = this.startClickFrameBackend().catch(() => {}); + let capTimer = null; + const cap = new Promise((r) => { capTimer = setTimeout(r, WARMUP_MAX_MS); }); + await Promise.race([warm, cap]); + if (capTimer) clearTimeout(capTimer); + if (!this.session || this.session.paused) { this.warmingUp = false; return; } } // Keep the window visible briefly so the user sees the transition even // when warmup was instant; warmup time counts toward this. @@ -354,17 +377,19 @@ class CaptureService { const elapsed = Date.now() - startedAt; if (elapsed < minVisibleMs) { await new Promise((r) => setTimeout(r, minVisibleMs - elapsed)); - if (!this.session || this.session.paused) return; + if (!this.session || this.session.paused) { this.warmingUp = false; return; } } if (wantHide && win && !win.isDestroyed() && win.isVisible()) { win.hide(); // Let a couple of frames of the now-unobscured screen land before // the user's first click, so that frame shows their work, not the // app window that was just dismissed. - await new Promise((r) => setTimeout(r, this.settings.get('capture.postHideSettleMs') || 150)); + await new Promise((r) => setTimeout(r, Number.isFinite(settleMs) ? settleMs : 150)); } + // Window hidden and buffer primed — clicks now count. + this.warmingUp = false; }; - run().catch(() => {}); + run().catch(() => { this.warmingUp = false; }); } finishSession() { @@ -372,6 +397,7 @@ class CaptureService { clearInterval(this.intervalTimer); this.intervalTimer = null; } + this.warmingUp = false; this.stopClickWatcher(); this.stopFrameLoop(); this.stopClickFrameBackend(); @@ -673,6 +699,12 @@ class CaptureService { return; } if (this.streamBackend || this.streamBackendStarting) return; + // Generation token: a stop/finish/pause bumps it. If it changes while + // this async start is in flight (e.g. the user finishes and restarts + // before a slow start resolves), the backend we built belongs to a dead + // session — discard it instead of installing it, and never leave + // streamBackendStarting stuck so the new session can start its own. + const gen = this.captureGen; this.streamBackendStarting = true; try { // eslint-disable-next-line global-require @@ -691,9 +723,10 @@ class CaptureService { sources: sources.map((s) => ({ id: s.id, display_id: s.display_id })), sampleMs: this.settings.get('capture.frameSampleMs') || 100, }); - if (!ok || !this.session || this.session.paused) { + const stale = gen !== this.captureGen; + if (!ok || stale || !this.session || this.session.paused) { backend.stop(); - if (this.session && !this.session.paused) { + if (!stale && this.session && !this.session.paused) { console.error('[stepforge] stream capture backend failed to start — using in-process frame loop'); this.startFrameLoop(); } @@ -703,16 +736,20 @@ class CaptureService { clog('stream capture backend active'); this.notify('capture:state', this.state()); } catch (err) { - if (this.session && !this.session.paused) { + if (gen === this.captureGen && this.session && !this.session.paused) { console.error(`[stepforge] stream capture backend error (${err && err.message}) — using in-process frame loop`); this.startFrameLoop(); } } finally { - this.streamBackendStarting = false; + if (gen === this.captureGen) this.streamBackendStarting = false; } } stopClickFrameBackend() { + // Invalidate any in-flight start (see captureGen above) and free the + // guard so the next session can start a fresh backend immediately. + this.captureGen += 1; + this.streamBackendStarting = false; if (!this.streamBackend) return; const backend = this.streamBackend; this.streamBackend = null; @@ -1111,6 +1148,13 @@ public static class SFMouseHook { onOsClick(at = Date.now(), osPoint = null, button = 'mouse') { if (!this.session || this.session.paused) return; + // Recording isn't live until the window is hidden and the buffer primed + // (see armRecording). Clicks during warmup land on the still-visible app + // window, not the user's work, so ignore them rather than capturing junk. + if (this.warmingUp) { + clog('click@', Number.isFinite(at) ? at : Date.now(), button, 'ignored — still warming up'); + return; + } const clickAt = Number.isFinite(at) ? at : Date.now(); // Leading-edge debounce: ignore a click that lands within the debounce // window of the last accepted click of the same button. This makes fast diff --git a/app/main.js b/app/main.js index e833fd8..14f6544 100644 --- a/app/main.js +++ b/app/main.js @@ -195,11 +195,22 @@ function createWindow() { console.log('CLICK-SELFTEST burst:', burstSteps, 'of', burstCount, burstSteps === burstCount ? 'OK — no clicks dropped on finish' : 'FAIL — clicks lost'); + // Helper: wait until armRecording has finished warming (window + // hidden, buffer primed) so an injected click counts as a real + // recording click rather than being ignored as a warmup click. + const waitArmed = async () => { + for (let i = 0; i < 80 && capture.warmingUp; i++) { + await new Promise((res) => setTimeout(res, 50)); + } + }; + // Third scenario: the real "Start recording" path. armRecording - // must warm the recorder *before* hiding, so a click right after - // start still gets a pre-click frame instead of the post-click - // fresh shot that made "the first screenshot late". (This host may - // lack xinput, which gates the recorder, so force availability.) + // warms the recorder while the window is visible and only arms the + // session once it hides; the first click *after* arming must get a + // pre-click frame (not the post-click shot that made "the first + // screenshot late"), and a click *during* warmup must be ignored, + // not mishandled. (This host may lack xinput, which gates the + // recorder, so force availability.) const armGuide = store.createGuide({ title: 'arm selftest' }); mainWindow.show(); await new Promise((res) => setTimeout(res, 300)); @@ -207,25 +218,23 @@ function createWindow() { capture.stopClickWatcher(); capture.clickCaptureAvailable = () => true; capture.hiddenForSession = true; // window was visible at session start - capture.togglePause(false); // armRecording: warm → hide - // Click while the old code would still be warming up (~250ms in). - await new Promise((res) => setTimeout(res, 250)); + capture.togglePause(false); // armRecording: warm → hide → arm + // A click during warmup must be ignored (window still visible). + await new Promise((res) => setTimeout(res, 200)); + const warmupClicks = store.getGuide(armGuide.guideId).stepsOrder.length; + capture.onOsClick(Date.now(), toPhysical({ x: bounds.x + 100, y: bounds.y + 100 }), 'button-1'); + await waitArmed(); const armPoint = { x: Math.round(bounds.x + bounds.width * 0.4), y: Math.round(bounds.y + bounds.height * 0.4), }; - const armClickAt = Date.now(); - capture.onOsClick(armClickAt, toPhysical(armPoint), 'button-1'); + capture.onOsClick(Date.now(), toPhysical(armPoint), 'button-1'); await capture.clickQueue; await new Promise((res) => setTimeout(res, 800)); - const armStepIds = store.getGuide(armGuide.guideId).stepsOrder; - let armPreClick = false; - if (armStepIds.length) { - // A pre-click frame is the win; the log line shows the margin. - armPreClick = true; - } - console.log('CLICK-SELFTEST arm: first click ->', armStepIds.length, - 'step(s)', armPreClick ? '(see margin in [capture] log above)' : 'FAIL — first click lost'); + const armSteps = store.getGuide(armGuide.guideId).stepsOrder.length; + console.log('CLICK-SELFTEST arm: warmup-click steps', warmupClicks, + '-> after-arm steps', armSteps, + armSteps === 1 ? 'OK — warmup click ignored, first armed click captured' : 'FAIL'); capture.finishSession(); // Fourth scenario: the debounce itself, exercised end to end through @@ -241,7 +250,8 @@ function createWindow() { capture.hiddenForSession = true; capture.togglePause(false); await capture.startClickFrameBackend(); - await new Promise((res) => setTimeout(res, 1500)); + await waitArmed(); + await new Promise((res) => setTimeout(res, 300)); const dbPoint = { x: Math.round(bounds.x + bounds.width * 0.55), y: Math.round(bounds.y + bounds.height * 0.55), diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 29c213b..cece653 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -51,6 +51,16 @@ Keep-a-Changelog conventions; versions follow semver. ### Fixed +- **Restarting a recording no longer drops clicks or "stops after one + click."** While the recorder warmed up at the start of a session the + window was still visible, so clicks over it were skipped and clicks + elsewhere were shot post-click — and on a restart the backend start could + take several seconds, stretching that bad window out. Recording is now + "armed" only once the window is actually hidden and the buffer is primed: + clicks during warmup are cleanly ignored (the window is covering the + user's work anyway), the window hides within a bounded time even if the + backend is slow to start, and a slow start left over from a finished + session can no longer block the next session from starting its own. - **Fast click bursts no longer lose screenshots.** Finishing or pausing a recording used to cancel every screenshot still being encoded, so a quick series of clicks saved only the first two or three. The capture worker now diff --git a/tests/unit/capture.test.js b/tests/unit/capture.test.js index a6371ec..8ec842b 100644 --- a/tests/unit/capture.test.js +++ b/tests/unit/capture.test.js @@ -554,6 +554,150 @@ test('debounced clicks never reach the capture queue (end to end through onOsCli assert.equal(service.session.count, 2); }); +// ---- warmup gating (recording goes live only after the window hides) ----------- + +test('clicks during warmup are ignored, not captured as junk', () => { + const service = makeService(); + service.session = { guideId: 'g-warm', paused: false, count: 0, intervalSec: 0 }; + service.warmingUp = true; + const seen = []; + service.enqueueClickCapture = (clickPos, at) => { seen.push(at); }; + + service.onOsClick(1000, { x: 10, y: 20 }, 'left'); + assert.deepEqual(seen, [], 'a click while warming up must not be captured'); + + // Once armed, clicks are captured again. + service.warmingUp = false; + service.onOsClick(2000, { x: 10, y: 20 }, 'left'); + assert.deepEqual(seen, [2000]); +}); + +test('pausing and finishing clear the warmup flag', () => { + const service = makeService(); + service.session = { guideId: 'g-warm2', paused: false, count: 0, intervalSec: 0 }; + service.warmingUp = true; + service.togglePause(true); + assert.equal(service.warmingUp, false, 'pause cancels an in-flight warmup'); + + service.session = { guideId: 'g-warm3', paused: false, count: 0, intervalSec: 0 }; + service.warmingUp = true; + service.finishSession(); + assert.equal(service.warmingUp, false, 'finish cancels an in-flight warmup'); +}); + +test('armRecording warms while visible, then hides and arms the session', async () => { + // Reproduces the restart path: recording must not be "live" until the + // window is hidden. A click injected during warmup is ignored; a click + // after arming is captured. + const service = makeService(); + const win = { + destroyed: false, visible: true, + isDestroyed() { return this.destroyed; }, + isVisible() { return this.visible; }, + isMinimized() { return false; }, + hide() { this.visible = false; }, + show() { this.visible = true; }, + focus() {}, getTitle() { return 'StepForge'; }, + getBounds() { return { x: 0, y: 0, width: 800, height: 600 }; }, + }; + service.getWindow = () => win; + service.clickCaptureAvailable = () => true; + // Stub the recorder so warmup resolves fast without real Electron. + service.startClickFrameBackend = async () => {}; + service.session = { guideId: 'g-arm', paused: false, count: 0, intervalSec: 0 }; + service.hiddenForSession = true; + const captured = []; + service.enqueueClickCapture = (clickPos, at) => { captured.push(at); }; + + service.armRecording(); + assert.equal(service.warmingUp, true, 'warming up begins immediately'); + service.onOsClick(1, { x: 10, y: 10 }, 'left'); + assert.deepEqual(captured, [], 'a click during warmup is ignored'); + + // Wait out the warmup (min-visible 400ms + settle 150ms here). + for (let i = 0; i < 40 && service.warmingUp; i++) { + await new Promise((r) => setTimeout(r, 50)); + } + assert.equal(service.warmingUp, false, 'warmup clears'); + assert.equal(win.visible, false, 'the window is hidden once armed'); + + service.onOsClick(2, { x: 10, y: 10 }, 'left'); + assert.deepEqual(captured, [2], 'a click after arming is captured'); + service.finishSession(); +}); + +test('a slow recorder start still arms within the warmup cap', async () => { + // If the backend start hangs (Windows can take seconds), the window must + // still hide and recording must still arm — the restart bug was the + // window staying up for many seconds, dropping every click over it. + const service = makeService(); + const win = { + destroyed: false, visible: true, + isDestroyed() { return this.destroyed; }, + isVisible() { return this.visible; }, + isMinimized() { return false; }, + hide() { this.visible = false; }, + show() { this.visible = true; }, + focus() {}, getTitle() { return 'StepForge'; }, + getBounds() { return { x: 0, y: 0, width: 800, height: 600 }; }, + }; + service.getWindow = () => win; + service.clickCaptureAvailable = () => true; + // Backend start that never resolves within the test. + service.startClickFrameBackend = () => new Promise(() => {}); + service.session = { guideId: 'g-slow', paused: false, count: 0, intervalSec: 0 }; + service.hiddenForSession = true; + + service.armRecording(); + // The cap is 1500ms; wait a bit beyond it. + for (let i = 0; i < 60 && service.warmingUp; i++) { + await new Promise((r) => setTimeout(r, 50)); + } + assert.equal(service.warmingUp, false, 'a hung backend start must not keep recording un-armed'); + assert.equal(win.visible, false, 'the window hides even when the recorder is slow to start'); + service.finishSession(); +}); + +test('a slow backend start from a finished session does not install itself or block a restart', async () => { + // Rapid restart: session A starts a backend that resolves slowly; the user + // finishes A and starts B before it resolves. A's late backend must not + // install into B, and the starting-guard must not block B from starting + // its own. + const service = makeService(); + service.session = { guideId: 'A', paused: false, count: 0, intervalSec: 0 }; + + let releaseA; + const aReady = new Promise((r) => { releaseA = r; }); + const built = []; + // Model the generation guard at the seam: a slow start that checks + // captureGen before installing the backend it built. + service.startClickFrameBackend = async function patched() { + const gen = this.captureGen; + this.streamBackendStarting = true; + try { + await aReady; // slow start + const backend = { isActive: () => true, stop: () => built.push('stopped') }; + if (gen !== this.captureGen || !this.session || this.session.paused) { + backend.stop(); + return; + } + this.streamBackend = backend; + built.push('installed'); + } finally { + if (gen === this.captureGen) this.streamBackendStarting = false; + } + }; + + const startA = service.startClickFrameBackend(); + service.finishSession(); // bumps captureGen, A is now stale + releaseA(); + await startA; + + assert.deepEqual(built, ['stopped'], 'the stale backend is stopped, never installed'); + assert.equal(service.streamBackend, null); + assert.equal(service.streamBackendStarting, false, 'the guard is freed for the next session'); +}); + // ---- coordinate conversion ------------------------------------------------------ test('hook coordinates are converted physical → DIP via screenToDipPoint when available', () => {