diff --git a/HANDOFF.md b/HANDOFF.md index 81f456b..63b1521 100644 --- a/HANDOFF.md +++ b/HANDOFF.md @@ -1,10 +1,36 @@ # HANDOFF — current state **Branch:** `main` (pre-1.0, no feature branches). **Last pushed:** `7f14ffb`. -**This session's work is COMMITTED LOCALLY, NOT PUSHED** — two commits ahead of `origin/main`. -Push only when the creator says so. +**This session's work is COMMITTED LOCALLY, NOT PUSHED** — **four** commits ahead of `origin/main` +(`fae8ec5`, `5aedca7`, `f5a9d46`, `de81fa3`) plus this unit. Push only when the creator says so. -## Two work units landed this session +## The 2026-09-26 proof-of-concept round (creator reporting) + +Context: **no YouTube private test recordings are being saved**, so the creator is judging +compositing from **local recordings**. They assumed the live path needed a separate "redirect" — +**it does not, and that is verified, not assumed:** one `_framePump` is constructed in the +`MainViewModel` ctor with a single `brandFlash:` callback, and both `Streaming.Operations.cs:68` +(go-live) and `:199` (record) call that same `StartAsync`. Record+simulcast is one ffmpeg with two +outputs. Five items, tracked in `TASKS.md` → "Creator-reported batch". + +### Done this unit: recording save dialog — Cancel now discards + +`RenameRecordingDialog` was always correct (Enter → `DialogResult=true`, Cancel/X/Escape → `false`). +**The caller was not**: `FinalizeRecordingAsync` only overwrote `stem` when the dialog returned true +and then ran `File.Move` **unconditionally**, so a cancelled dialog silently saved the recording +under the default name. Extracted the decision into internal +`MainViewModel.CompleteRecordingSave(startPath, dir, autoStem, chosenStem, creatorSaved)`: + +- `creatorSaved == false` (Cancel/Escape/X) → **delete the temp file**; the videos folder is left + empty. A failed delete returns `DiscardFailed` and the toast names the file + folder. +- `creatorSaved == true` (Save, or Enter on the pre-filled default) → `File.Move` to the final name. + Blank box still means "keep the auto name" — but only on an explicit Save. + +Test: `ytLive.Tests/RecordingSaveDialogTests.cs` (5 facts, real temp files, no WPF needed) — the +headline asserts Cancel leaves the directory **empty**, not merely "the stem is unchanged". +Lesson recorded in `MyMistakes.md` (falsy `ShowDialog()` falling through into a side effect). + +## Two earlier work units (committed) ### 1. Branding flash is now composited into the OUTPUT (TASK 36 shipped) @@ -71,8 +97,19 @@ The entire implementation is inside `#if DEBUG`. Release compiles to `DataRoot = ## Next -1. Push the two commits when the creator asks. -2. Test-console chat UX (queued): don't clear the chat window, 20px right padding on the pull-out +1. **Ungate the brand flash from `IsLive`** (TASK batch #2) — it must run in every scene and appear + in recordings; today it only starts in `UpdateLiveVisuals()`'s live branch, so a recording made + without ever going live carries no credit. +2. **Confine the ticker to the alert box** (#3) — it is still a global 1920×48 top overlay + (`BlitOverlay(…, 0, 0)`); needs a rect + scale/clip decision in both preview and output. +3. **Multi-instance** (#4) — route `FfmpegLocator._toolsDir` through `InstanceProfile`, and confirm + the launch method: `dotnet run` while the first app holds `bin/…/ytLive.exe` fails with MSB3027 + *before any instance starts* (a build-output lock, not a log conflict). `$env:YTLIVE_INSTANCE=2` + + `dotnet run --no-build` in a second shell is the known-good path. +4. **Post-session efficacy report** (#5) — roll up `CurrentHealth` (dropped frames, duration, health + message) when a stream or recording ends. +5. Push the commits when the creator asks. +6. Test-console chat UX (queued): don't clear the chat window, 20px right padding on the pull-out input, Enter inserts a newline. -3. Decide the alert-gating copy question above. -4. Ship-checklist items live in `TASKS.md` → "1.0 gates". +7. Decide the alert-gating copy question above. +8. Ship-checklist items live in `TASKS.md` → "1.0 gates". diff --git a/MyMistakes.md b/MyMistakes.md index de7caf0..393d54c 100644 --- a/MyMistakes.md +++ b/MyMistakes.md @@ -1047,3 +1047,50 @@ Release binary for it — and it was absent from the *Debug* binary too, because at compile time and never reach metadata**. The control (run it against Debug as well) is what exposed that. Grep for a **field/type** (`InstanceVariable`) or use reflection; never grep for an inlined literal, and always run the check against the build you expect it to FAIL in. + +## 2026-09-26 — A falsy `ShowDialog()` fell through into the side effect: "Cancel" that saved + +The recording save prompt let the creator name the file. It always had a **Cancel** button, and +Cancel always *appeared* to work — the dialog closed, the toast was skipped, nothing visibly went +wrong. The footage was saved anyway, under the default name. + +```csharp +// WRONG — and it type-checked, read fine, and looked correct +string stem = autoStem; +var dialog = new RenameRecordingDialog(autoStem); +if (dialog.ShowDialog() == true && !string.IsNullOrEmpty(dialog.ResultFileName)) + stem = dialog.ResultFileName; + +var finalPath = UniquePath(Path.Combine(dir, stem + ".mp4")); +File.Move(startPath, finalPath); // <-- runs even when the creator hit Cancel +``` + +**The trap:** `ShowDialog()` returning `false` was collapsed into the *default* branch instead of +being treated as a distinct outcome. "Not what I typed" and "I don't want this" are different +answers, and the nullable stem made the first one look like a legitimate value of the second. The +default value of the accumulator absorbed the cancel. + +**The rule, and the grep-able form of it:** when a modal's result gates a side effect, every branch +is explicit and the side effect sits **inside** the accepting branch: + +```csharp +if (!dialog.ShowDialog() == true) { DoTheOppositeThing(); return; } // read the FALSE case first +DoTheThing(); // the effect is not a fall-through +``` + +A `DialogResult`/return value that only ever *modifies* a later value (rather than *permitting* the +effect) is the smell. `null` from a dialog is the same trap wearing a nullable. + +**Why it survived so long:** the side effect is in a *different method* from the modal, and the modal +had its own passing test (`RenameRecordingDialogSizingTests`, which only checks layout). The test +suite was green because the bug was never in the tested unit. **A modal's own test proves nothing +about the decision the modal feeds** — cover the commit-or-discard decision itself, against real +files (`ytLive.Tests/RecordingSaveDialogTests.cs` drives the extracted +`MainViewModel.CompleteRecordingSave` and asserts Cancel leaves the directory EMPTY, not just "the +stem is unchanged"). + +**Creator's actual ruling, worth keeping verbatim:** "if I click cancel, that doesn't mean to accept +the default file name and save the file — that's what pressing enter should do. Cancel should cancel +the save option, discarding the recording." A Cancel button on a destructive-and-final dialog means +*discard*, not *accept default*. When a dialog is the last gate before data is committed, ask what +Cancel should destroy — don't infer "keep the default" just because the default already exists. diff --git a/RenameRecordingDialog.xaml.cs b/RenameRecordingDialog.xaml.cs index f0da8e9..803aa03 100644 --- a/RenameRecordingDialog.xaml.cs +++ b/RenameRecordingDialog.xaml.cs @@ -6,12 +6,16 @@ namespace ytLive; /// Modal that lets the creator rename an auto-named recording before it is /// finalized. Returns the chosen file stem (no extension) via , -/// or null on Cancel / keep-auto-name. +/// or null when the creator cleared the box and hit Save (keep the auto name). +/// DialogResult == false means DISCARD — the caller deletes the footage +/// rather than saving it (CREATOR RULING 2026-09-26). Enter on the pre-filled default +/// is a Save. public partial class RenameRecordingDialog : Window { private readonly string _autoStem; - /// Stem (no extension) the creator chose, or null to keep auto-name. + /// Stem (no extension) the creator chose, or null when the box was + /// cleared on an explicit Save. Never set on Cancel. public string? ResultFileName { get; private set; } public RenameRecordingDialog(string autoStem) diff --git a/TASKS.md b/TASKS.md index 2ac5154..e6a0298 100644 --- a/TASKS.md +++ b/TASKS.md @@ -68,6 +68,32 @@ ## Open items (at a glance) +### Creator-reported batch (2026-09-26) — proof-of-concept round, recordings used as the reference + +The creator is **not** getting YouTube private test recordings saved, so local recordings are the +proof of concept for compositing; the live path is assumed to share it. **Verified true, not assumed:** +one `_framePump` is built in the `MainViewModel` ctor with a single `brandFlash:` callback, and BOTH +`Streaming.Operations.cs:68` (go-live) and `:199` (record) call that same `StartAsync` — there is no +second encoder path needing a "redirect". Record+simulcast is one ffmpeg with two outputs. + +1. ✅ **Recording save dialog: Cancel discards** — was silently saving under the default name. Enter + accepts the default; Cancel/Escape/X **deletes** the footage and names the file if the delete fails. + `MainViewModel.CompleteRecordingSave` (internal) + `ytLive.Tests/RecordingSaveDialogTests.cs`. +2. ☐ **Brand flash must run in ALL scenes, not only while live, and must be in recordings.** Currently + started/stopped from `UpdateLiveVisuals()`'s `IsLive` branch, so recording-only (never go-live) + shows no credit at all. Ungate the presenter from `IsLive`; the pump already carries it to both. +3. ☐ **Ticker confined to the alert box.** The announcement strip is a **global 1920×48 top overlay** + (`AlertOverlayLayer` comment + `BlitOverlay(…, 0, 0)`), but the creator wants it over the *Stream + Alerts video* — i.e. inside the `SourceType.AlertBox` rect, in preview AND output. Needs a rect + + scale/clip decision (see `ai.md`). +4. ☐ **Two instances still refuse to run.** `InstanceProfile` isolates layout/auth/log/WebView, but + `FfmpegLocator._toolsDir` is still the hardcoded shared `%APPDATA%\ytLlive\tools` and both + instances can `Directory.CreateDirectory` + `ExtractBinaries` into it. Also the launch method is + unconfirmed: `dotnet run` while the first app holds `bin/…/ytLive.exe` dies with `MSB3027` before + any instance starts — that is a build-output lock, not a log conflict. +5. ☐ **Post-session efficacy report** — on end of stream *or* recording, report what worked and what + failed, including the **dropped frames** the creator saw. `CurrentHealth` already tracks + `DroppedFrames`/`StreamDuration`; `SessionTeardownTests` is the natural home for the roll-up. - **TASK 3** — items 16 (Text source) + 20 (RewardEvent capture → SQLite, Alerts' persistence half) open; **17 (Alerts) is DONE via TASK 43 (2026-09-24) — native six-event alert box** - **TASK 9** — item 5 open (webcam identity key reconciliation) - **TASK 10** — Velopack update URL pending diff --git a/ViewModels/MainViewModel.Recording.cs b/ViewModels/MainViewModel.Recording.cs index 09307d4..e68bd8e 100644 --- a/ViewModels/MainViewModel.Recording.cs +++ b/ViewModels/MainViewModel.Recording.cs @@ -52,18 +52,36 @@ public partial class MainViewModel : ViewModelBase var autoStem = RecordingFile.BuildFinalName(_recordStartTime, _recordLength); var dir = Path.GetDirectoryName(startPath)!; - // Manual rename modal: give the creator a chance to name the recording; - // null = keep the auto name (or the default recorded above). - string stem = autoStem; + // Manual rename modal: give the creator a chance to name the recording. + // CREATOR RULING 2026-09-26: Cancel means DISCARD, not "save under the + // default name". This used to fall through and File.Move anyway, so a + // cancelled dialog silently kept the recording — Cancel was a lie. var dialog = new RenameRecordingDialog(autoStem); - if (dialog.ShowDialog() == true && !string.IsNullOrEmpty(dialog.ResultFileName)) - stem = dialog.ResultFileName; + var creatorSaved = dialog.ShowDialog() == true; - var finalPath = UniquePath(Path.Combine(dir, stem + ".mp4")); - File.Move(startPath, finalPath); - AppLog.Write($"Recording saved: {finalPath}"); - _notifications.Info("Recording saved", - Path.GetFileName(finalPath) + " — you can rename it in your videos folder at any time."); + var result = CompleteRecordingSave( + startPath, dir, autoStem, dialog.ResultFileName, creatorSaved); + switch (result.Outcome) + { + case RecordingSaveOutcome.Saved: + AppLog.Write($"Recording saved: {result.FinalPath}"); + _notifications.Info("Recording saved", + Path.GetFileName(result.FinalPath!) + + " — you can rename it in your videos folder at any time."); + break; + case RecordingSaveOutcome.Discarded: + AppLog.Write($"Recording discarded by creator: {startPath}"); + _notifications.Info("Recording discarded", + "The recording was deleted and nothing was saved."); + break; + case RecordingSaveOutcome.DiscardFailed: + // A "discarded" recording still sitting on disk is worse than no + // report at all — name the file and the folder so it can be found. + AppLog.Write($"Recording discard failed: {startPath}"); + _notifications.Error("Recording discarded?", + $"Could not delete {Path.GetFileName(startPath)} — it is still in {dir}."); + break; + } } catch (Exception ex) { @@ -73,6 +91,48 @@ public partial class MainViewModel : ViewModelBase } } + /// What happened to the footage once the save dialog was answered, plus + /// where it landed (FinalPath is set only for ). + internal readonly record struct RecordingSaveResult(RecordingSaveOutcome Outcome, string? FinalPath); + + /// What happened to the footage once the save dialog was answered. + internal enum RecordingSaveOutcome + { + Saved, + Discarded, + DiscardFailed, + } + + /// The commit-or-discard decision behind the recording save dialog, split out + /// so it can be tested against real files without a frame pump (CREATOR RULING + /// 2026-09-26). is the dialog's DialogResult: + /// false — Cancel, Escape, or the X — DELETES the footage; only an explicit + /// Save (including Enter on the pre-filled default) moves it to its final name. + internal static RecordingSaveResult CompleteRecordingSave( + string startPath, string dir, string autoStem, string? chosenStem, bool creatorSaved) + { + if (!creatorSaved) + { + if (!File.Exists(startPath)) return new RecordingSaveResult(RecordingSaveOutcome.Discarded, null); + try + { + File.Delete(startPath); + return new RecordingSaveResult(RecordingSaveOutcome.Discarded, null); + } + catch (Exception) + { + return new RecordingSaveResult(RecordingSaveOutcome.DiscardFailed, startPath); + } + } + + // A blank name still means "keep the auto name" (the dialog says so), but + // only on an explicit Save. Enter with the pre-filled default lands here too. + var stem = string.IsNullOrWhiteSpace(chosenStem) ? autoStem : chosenStem!; + var finalPath = UniquePath(Path.Combine(dir, stem + ".mp4")); + File.Move(startPath, finalPath); + return new RecordingSaveResult(RecordingSaveOutcome.Saved, finalPath); + } + private static string UniquePath(string path) { if (!File.Exists(path)) return path; diff --git a/ai.md b/ai.md index 8d2535c..4988519 100644 --- a/ai.md +++ b/ai.md @@ -705,6 +705,20 @@ lights when a session is actually running. **rename-on-stop** to `ty-…-.mp4` (`FinalizeRecordingAsync` after the pump stops & the file closes), numeric `-2`/`-3` suffix on collision (`UniquePath`). Length comes from `_liveElapsed`, which the session timer walks while `IsLive || IsRecording`. +- **Save dialog: Cancel DISCARDS (creator ruling 2026-09-26).** The `RenameRecordingDialog` result is a + two-outcome decision, and it lives in the extracted `MainViewModel.CompleteRecordingSave(startPath, dir, + autoStem, chosenStem, creatorSaved)` (internal, so `ytLive.Tests` can drive it against real files without + a frame pump — see `ytLive.Tests/RecordingSaveDialogTests.cs`): + - `creatorSaved == false` (**Cancel / Escape / the X**) → **DELETE the temp file.** Nothing is moved, + nothing is left in the videos folder. A failed delete returns `DiscardFailed` and the toast NAMES the + file + folder, because a "discarded" recording still on disk is worse than no report. + - `creatorSaved == true` (Save, or **Enter on the pre-filled default**) → `File.Move` to the final name. + A blank/whitespace box still means "keep the auto name" — but only on an explicit Save. + - **The bug this fixed:** the caller used to `File.Move` UNCONDITIONALLY and only overwrite `stem` when + the dialog returned true, so `ShowDialog() == false` fell straight through into the save — Cancel + silently kept the recording under the default name. The modal itself was always correct; the + side effect was in the wrong place. **Any modal whose result gates a side effect must have every + branch handled explicitly — a falsy result must never fall through into the effect.** - **Folder:** default `%APPDATA%\ytLlive\recordings\`, user-overridable via `ChooseRecordFolderCommand` (`OpenFolderDialog`), persisted through `LayoutStore.Load/SaveRecordFolder` (`RecordFolder` key). - **Explicit sign-out only:** `StopStream` no longer clears the session/token. Sign out via Logout / diff --git a/ytLive.Tests/RecordingSaveDialogTests.cs b/ytLive.Tests/RecordingSaveDialogTests.cs new file mode 100644 index 0000000..1b93766 --- /dev/null +++ b/ytLive.Tests/RecordingSaveDialogTests.cs @@ -0,0 +1,117 @@ +using System; +using System.IO; +using Xunit; +using ytLive.ViewModels; + +namespace ytLive.Tests; + +/// +/// The post-recording save dialog (CREATOR RULING 2026-09-26). +/// Reported symptom: clicking Cancel in the save prompt still saved the +/// recording under the default name — Cancel behaved like "accept the default". The +/// creator's intent is explicit: Enter accepts the default file name and saves; +/// Cancel cancels the save and discards the footage. +/// The bug was in the caller, not the modal — FinalizeRecordingAsync ran +/// File.Move unconditionally, so ShowDialog() == false fell straight +/// through into the save. These tests drive the extracted commit-or-discard decision +/// against real files in a temp folder, which is what would have caught it. +/// +public sealed class RecordingSaveDialogTests : IDisposable +{ + private readonly string _dir = Path.Combine( + Path.GetTempPath(), "ytLive-recsave-" + Guid.NewGuid().ToString("N")[..8]); + + private const string AutoStem = "ty-20260926-1405-0130"; + private const string StartStem = "ty-20260926-1405-0000"; + private const string Bytes = "fake-mp4-payload"; + + public RecordingSaveDialogTests() => Directory.CreateDirectory(_dir); + + public void Dispose() + { + try { Directory.Delete(_dir, recursive: true); } catch { /* best-effort */ } + } + + /// The reported bug: Cancel must leave NOTHING in the videos folder — not + /// the temp start-name file, and not a renamed copy of it either. + [Fact] + public void Cancel_DiscardsTheFootage_AndLeavesNothingOnDisk() + { + var start = WriteStartFile(); + + var result = CompleteRecordingSave(start, _dir, AutoStem, chosenStem: null, creatorSaved: false); + + Assert.Equal(MainViewModel.RecordingSaveOutcome.Discarded, result.Outcome); + Assert.False(File.Exists(start), "Cancel must delete the temp recording, not move it"); + Assert.Empty(Directory.GetFiles(_dir)); + } + + /// Enter on the pre-filled default is a Save — the file lands under the + /// auto name with its bytes intact (the creator's other half of the ruling). + [Fact] + public void EnterOnTheDefault_KeepsTheAutoName_AndSavesTheFootage() + { + var start = WriteStartFile(); + + var result = CompleteRecordingSave(start, _dir, AutoStem, AutoStem, creatorSaved: true); + + Assert.Equal(MainViewModel.RecordingSaveOutcome.Saved, result.Outcome); + var expected = Path.Combine(_dir, AutoStem + ".mp4"); + Assert.Equal(expected, result.FinalPath); + Assert.False(File.Exists(start), "Save renames the temp file — it must not be left behind"); + Assert.Equal(Bytes, File.ReadAllText(expected)); + Assert.Single(Directory.GetFiles(_dir)); + } + + /// A cleared name box on an explicit Save still means "keep the auto + /// name" (the dialog promises this) — Cancel is the only way to discard. + [Fact] + public void SaveWithABlankName_StillFallsBackToTheAutoName() + { + var start = WriteStartFile(); + + var result = CompleteRecordingSave(start, _dir, AutoStem, " ", creatorSaved: true); + + Assert.Equal(MainViewModel.RecordingSaveOutcome.Saved, result.Outcome); + Assert.True(File.Exists(Path.Combine(_dir, AutoStem + ".mp4"))); + } + + /// An explicit Save with a chosen name honours that name. + [Fact] + public void SaveWithAChosenName_UsesThatName() + { + var start = WriteStartFile(); + + var result = CompleteRecordingSave(start, _dir, AutoStem, "my-stream", creatorSaved: true); + + Assert.Equal("my-stream.mp4", Path.GetFileName(result.FinalPath!)); + } + + /// Never clobber an earlier recording: a second save with the same name + /// gets uniquified rather than overwriting the first. + [Fact] + public void SaveWithACollidingName_UniquifiesInsteadOfOverwriting() + { + var first = WriteStartFile(); + Assert.Equal( + MainViewModel.RecordingSaveOutcome.Saved, + CompleteRecordingSave(first, _dir, AutoStem, AutoStem, true).Outcome); + + var second = WriteStartFile(); + var result = CompleteRecordingSave(second, _dir, AutoStem, AutoStem, true); + + Assert.Equal(AutoStem + "-2.mp4", Path.GetFileName(result.FinalPath!)); + Assert.Equal(2, Directory.GetFiles(_dir).Length); + } + + private static MainViewModel.RecordingSaveResult CompleteRecordingSave( + string startPath, string dir, string autoStem, string? chosenStem, bool creatorSaved) + => MainViewModel.CompleteRecordingSave(startPath, dir, autoStem, chosenStem, creatorSaved); + + private string WriteStartFile() + { + var start = Path.Combine(_dir, StartStem + ".mp4"); + File.WriteAllText(start, Bytes); + return start; + } +}