diff --git a/docs/src/content/docs/docs/Advanced/CLI.md b/docs/src/content/docs/docs/Advanced/CLI.md index 4f9fa52ca..0b122c1f7 100644 --- a/docs/src/content/docs/docs/Advanced/CLI.md +++ b/docs/src/content/docs/docs/Advanced/CLI.md @@ -320,9 +320,9 @@ obsidian vault=dev quickadd:run choice="Inbox" value-value=" " verify=true That run is working exactly as designed: the capture's payload was empty, so QuickAdd deliberately left `Inbox.md` alone rather than writing a blank line, and -said so in a notice. A Template set to **Do nothing** when the file already exists -reports the same. If you are counting captures, writing an idempotency marker, or -deciding whether to retry, key off `effect`, not `ok`. +said so in a notice. A Template set to **Keep existing file** when the file +already exists reports the same `effect`. If you are counting captures, writing an +idempotency marker, or deciding whether to retry, key off `effect`, not `ok`. `effect` is present on every **success** payload (`ok:true`), and `unknown` is stated rather than omitted - a missing key reads as `false` in both `jq` and JavaScript, which diff --git a/docs/src/content/docs/docs/Choices/TemplateChoice.md b/docs/src/content/docs/docs/Choices/TemplateChoice.md index cf75ace5a..387b06ebe 100644 --- a/docs/src/content/docs/docs/Choices/TemplateChoice.md +++ b/docs/src/content/docs/docs/Choices/TemplateChoice.md @@ -431,6 +431,7 @@ With **Search existing notes before creating** enabled, this setting is called existing file**, **Create another file**, or **Keep existing file**. - **Update action** - shown only when you choose **Update existing file**. - **New file naming** - shown only when you choose **Create another file**. +- **Keep action** - shown only when you choose **Keep existing file**. ### Let QuickAdd ask each time {#ask-every-time} @@ -442,7 +443,7 @@ already exists: - **Overwrite file** - **Increment trailing number** - **Append duplicate suffix** -- **Do nothing** +- **Open existing file** ### Update the existing note {#update-existing-file} @@ -479,8 +480,7 @@ These options keep the existing file untouched and create a new file instead: ### Keep the existing note {#keep-existing-file} -Selecting **Keep existing file** applies the same result as choosing **Do -nothing** from the prompt: +These options leave the existing file unchanged: -- **Do nothing** - leaves the existing file unchanged and opens it - automatically. This does not require the separate **Open** setting. +- **Open existing file** - opens the existing file, even when **Open** is off. +- **Do nothing** - opens the existing file only when **Open** is on. diff --git a/src/engine/TemplateChoiceEngine.collision.test.ts b/src/engine/TemplateChoiceEngine.collision.test.ts index 6e5ffab5c..8d1de6dde 100644 --- a/src/engine/TemplateChoiceEngine.collision.test.ts +++ b/src/engine/TemplateChoiceEngine.collision.test.ts @@ -78,6 +78,11 @@ vi.mock("../formatters/completeFormatter", () => { vi.mock("../utils/templaterIntegration", () => ({ getTemplater: vi.fn(() => ({})), overwriteTemplaterOnce: vi.fn(), + jumpToNextTemplaterCursorIfPossible: vi.fn(async () => true), +})); +vi.mock("../utils/errorUtils", async (importOriginal) => ({ + ...(await importOriginal()), + reportError: vi.fn(), })); vi.mock("../utils/vaultQueries", () => ({ getAllFolderPathsInVault: vi.fn(() => []), @@ -125,6 +130,9 @@ vi.mock("./TemplateInsertEngine", () => { import { TFile, TFolder, type App } from "obsidian"; import GenericSuggester from "../gui/GenericSuggester/genericSuggester"; +import { Notice } from "../../tests/obsidian-stub"; +import { openFile as openFileInWorkspace } from "../utils/fileOpening"; +import { reportError } from "../utils/errorUtils"; import type { IChoiceExecutor } from "../IChoiceExecutor"; import { settingsStore } from "../settingsStore"; import { getPromptModes } from "../template/fileExistsPolicy"; @@ -739,4 +747,28 @@ describe("TemplateChoiceEngine collision behavior", () => { expect(GenericSuggester.Suggest).toHaveBeenCalledTimes(1); expect(engine.choice.fileExistsBehavior).toEqual({ kind: "prompt" }); }); + + it.each([ + { mode: "doNothing", openFile: false, opens: true, notice: [] }, + { mode: "leaveUnchanged", openFile: false, opens: false, notice: ["'Test Template' already exists."] }, + { mode: "leaveUnchanged", openFile: true, opens: true, notice: [] }, + ] as const)("$mode with openFile=$openFile opens the existing note: $opens", async ({ mode, openFile, opens, notice }) => { + const { app, engine } = createEngine(); + const existingFile = createExistingFile("Test Template.md"); + engine.choice.openFile = openFile; + engine.choice.fileExistsBehavior = { kind: "apply", mode }; + (app.vault.adapter.exists as ReturnType).mockResolvedValue(true); + (app.vault.getAbstractFileByPath as ReturnType).mockReturnValue(existingFile); + vi.mocked(openFileInWorkspace).mockClear(); + vi.mocked(reportError).mockClear(); + const noticesBefore = Notice.instances.length; + + await engine.run(); + + expect(vi.mocked(openFileInWorkspace).mock.calls.map(([, file]) => file)).toEqual( + opens ? [existingFile] : [], + ); + expect(Notice.instances.slice(noticesBefore).map(({ message }) => message)).toEqual(notice); + expect(reportError).not.toHaveBeenCalled(); + }); }); diff --git a/src/engine/TemplateChoiceEngine.ts b/src/engine/TemplateChoiceEngine.ts index 6d71aabbb..b1141ded3 100644 --- a/src/engine/TemplateChoiceEngine.ts +++ b/src/engine/TemplateChoiceEngine.ts @@ -155,8 +155,8 @@ export class TemplateChoiceEngine extends TemplateEngine { // What this run did to its target note (#1615). Derived from the file-exists // resolution the engine actually performed rather than from a byte compare, // which is exact for the two answers an automation acts on: "createNew" - // always writes a new note, and "reuseExisting" — the shipped "Do nothing" - // mode — writes nothing at all. Only "modifyExisting" (append/overwrite) is + // always writes a new note, and "reuseExisting" — the Keep existing file + // modes — writes nothing at all. Only "modifyExisting" (append/overwrite) is // inferred: it always writes, so `changed` can in principle over-report a // write whose bytes happened to match, which is the harmless direction. let effect: ChoiceEffect = "created"; @@ -290,14 +290,18 @@ export class TemplateChoiceEngine extends TemplateEngine { this.placeCursor(createdFile, cursorBeforeLink); } } else if ( - createdNew && + (createdNew || effect === "unchanged") && !linkOptions.enabled && !this.choice.copyLinkToClipboard ) { - // The note was created but nothing else surfaces it (not opened, no - // link appended, not copied to clipboard). Confirm the creation so - // the run isn't silent — mirroring Capture's success notice. - new Notice(`Created '${createdFile.basename}'.`); + // Nothing else surfaces the note (not opened, no link appended, not + // copied to clipboard). Say what happened so the run isn't silent - + // mirroring Capture's success notice. + new Notice( + createdNew + ? `Created '${createdFile.basename}'.` + : `'${createdFile.basename}' already exists.`, + ); } } catch (err) { if ( @@ -515,10 +519,9 @@ export class TemplateChoiceEngine extends TemplateEngine { }; } case "reuseExisting": - log.logMessage(`Opening existing file: ${existingFile!.path}`); return { createdFile: existingFile, - shouldAutoOpen: true, + shouldAutoOpen: mode.opensExistingFile, }; } } diff --git a/src/engine/choiceOutcomeRecorder.ts b/src/engine/choiceOutcomeRecorder.ts index 2e7546ec1..f1cb9e129 100644 --- a/src/engine/choiceOutcomeRecorder.ts +++ b/src/engine/choiceOutcomeRecorder.ts @@ -50,7 +50,7 @@ export class ChoiceOutcomeRecorder { * * `effect` is required rather than defaulted: every call site has to state its claim, * so a new one cannot inherit a positive "something landed" by omission. The four - * existing sites split evenly — two of them (Template's "Do nothing" mode and its + * existing sites split evenly — two of them (Template's Keep existing file modes and its * open-an-existing-note discovery path) commit nothing at all. */ success(file: TFile | undefined, effect: ChoiceEffect): void { diff --git a/src/gui/ChoiceBuilder/TemplateChoiceForm.svelte b/src/gui/ChoiceBuilder/TemplateChoiceForm.svelte index 864002f85..8fd52d9b8 100644 --- a/src/gui/ChoiceBuilder/TemplateChoiceForm.svelte +++ b/src/gui/ChoiceBuilder/TemplateChoiceForm.svelte @@ -9,7 +9,6 @@ import type { } from "../../template/fileExistsPolicy"; import { fileExistsBehaviorCategoryOptions, - getBehaviorCategory, getDefaultBehaviorForCategory, getFileExistsMode, getModesForCategory, @@ -178,18 +177,13 @@ commitPending = addPendingFolder; onDestroy(addPendingFolder); // --- File already exists ------------------------------------------------- -const behaviorCategory = $derived(getBehaviorCategory(choice.fileExistsBehavior)); -const showModeRow = $derived( - behaviorCategory !== "prompt" && behaviorCategory !== "keep", -); -const modeOptions = $derived( - getModesForCategory(behaviorCategory === "update" ? "update" : "create"), -); +const modeRowName = { update: "Update action", create: "New file naming", keep: "Keep action" }; const selectedMode = $derived( choice.fileExistsBehavior.kind === "apply" - ? choice.fileExistsBehavior.mode - : modeOptions[0].id, + ? getFileExistsMode(choice.fileExistsBehavior.mode) + : null, ); +const behaviorCategory = $derived(selectedMode?.category ?? "prompt"); function onCategoryChange(value: string) { choice.fileExistsBehavior = getDefaultBehaviorForCategory( @@ -375,15 +369,15 @@ function onModeChange(value: string) { {/snippet} - {#if showModeRow} + {#if selectedMode} {#snippet control()} ({ + value={selectedMode.id} + options={getModesForCategory(selectedMode.category).map((mode) => ({ value: mode.id, label: mode.label, }))} @@ -393,7 +387,7 @@ function onModeChange(value: string) { {/if} - + {#if choice.openFile} {/if} diff --git a/src/gui/ChoiceBuilder/TemplateChoiceForm.test.ts b/src/gui/ChoiceBuilder/TemplateChoiceForm.test.ts index 8b64d117f..0979eb8f4 100644 --- a/src/gui/ChoiceBuilder/TemplateChoiceForm.test.ts +++ b/src/gui/ChoiceBuilder/TemplateChoiceForm.test.ts @@ -244,16 +244,27 @@ describe("TemplateChoiceForm", () => { expect(settingNames(container)).toContain("File opening location"); }); - it("shows the file-exists mode row only for update/create categories", () => { + it("shows the file-exists mode row for every category except Ask every time", () => { const { container, props } = mountForm(); - expect(settingNames(container)).not.toContain("New file naming"); + const modeRows = ["Update action", "New file naming", "Keep action"]; + expect(settingNames(container).filter((name) => modeRows.includes(name))).toEqual([]); props.choice.fileExistsBehavior = { kind: "apply", mode: "increment" }; flushSync(); - const names = settingNames(container); - expect( - names.includes("New file naming") || names.includes("Update action"), - ).toBe(true); + expect(settingNames(container).filter((name) => modeRows.includes(name))).toEqual(["New file naming"]); + }); + + it("keeps a saved Keep existing file choice on Open existing file and offers Do nothing", async () => { + const { container, props } = mountForm(); + props.choice.fileExistsBehavior = { kind: "apply", mode: "doNothing" }; + flushSync(); + const select = settingItem(container, "Keep action").querySelector("select")!; + expect(select.value).toBe("doNothing"); + expect(Array.from(select.options, (option) => option.text)).toEqual(["Open existing file", "Do nothing"]); + + await fireEvent.change(select, { target: { value: "leaveUnchanged" } }); + flushSync(); + expect(props.choice.fileExistsBehavior).toEqual({ kind: "apply", mode: "leaveUnchanged" }); }); it("persists the copy-link-to-clipboard toggle", async () => { diff --git a/src/template/fileExistsPolicy.test.ts b/src/template/fileExistsPolicy.test.ts index a5a5276f4..8b4a59531 100644 --- a/src/template/fileExistsPolicy.test.ts +++ b/src/template/fileExistsPolicy.test.ts @@ -1,7 +1,6 @@ import { describe, expect, it, vi } from "vitest"; import { fileExistsBehaviorCategoryOptions, - getBehaviorCategory, getDefaultBehaviorForCategory, getFileExistsMode, getExistingNoteAction, @@ -47,22 +46,10 @@ describe("fileExistsPolicy registry", () => { ]); expect(getModesForCategory("keep").map((mode) => mode.id)).toEqual([ "doNothing", + "leaveUnchanged", ]); }); - it("derives behavior category from behavior state", () => { - expect(getBehaviorCategory({ kind: "prompt" })).toBe("prompt"); - expect( - getBehaviorCategory({ kind: "apply", mode: "appendBottom" }), - ).toBe("update"); - expect(getBehaviorCategory({ kind: "apply", mode: "increment" })).toBe( - "create", - ); - expect(getBehaviorCategory({ kind: "apply", mode: "doNothing" })).toBe( - "keep", - ); - }); - it("returns category defaults and preserves modes already in-category", () => { expect(getDefaultBehaviorForCategory("prompt")).toEqual({ kind: "prompt" }); expect(getDefaultBehaviorForCategory("update")).toEqual({ diff --git a/src/template/fileExistsPolicy.ts b/src/template/fileExistsPolicy.ts index c4e31400c..5d4ed5592 100644 --- a/src/template/fileExistsPolicy.ts +++ b/src/template/fileExistsPolicy.ts @@ -63,12 +63,24 @@ export const fileExistsModes = [ resolutionKind: "createNew", }, { + // Saved choices store this id from when it was the only keep mode, so it + // keeps its id and its behavior: it opens the note even with Open off. id: "doNothing", category: "keep", + label: "Open existing file", + description: "Leaves the existing file unchanged and opens it, even when Open is off.", + requiresExistingFile: true, + resolutionKind: "reuseExisting", + opensExistingFile: true, + }, + { + id: "leaveUnchanged", + category: "keep", label: "Do nothing", - description: "Leaves the file unchanged and opens the existing file.", + description: "Leaves the existing file unchanged. It opens only when Open is on.", requiresExistingFile: true, resolutionKind: "reuseExisting", + opensExistingFile: false, }, ] as const; @@ -133,6 +145,7 @@ const legacyFileExistsModeMap: Record = { increment: "increment", duplicateSuffix: "duplicateSuffix", doNothing: "doNothing", + leaveUnchanged: "leaveUnchanged", }; const createNewPathResolvers: Record< @@ -163,18 +176,11 @@ function getCategoryForMode( return getFileExistsMode(modeId).category; } +// The prompt's Do nothing item used to open the note, so its slot now reads Open +// existing file with the same behavior. Leaving the note alone from the prompt is +// what Escape already does. export function getPromptModes(): FileExistsModeDefinition[] { - return [...fileExistsModes]; -} - -export function getBehaviorCategory( - behavior: TemplateFileExistsBehavior, -): FileExistsBehaviorCategoryId { - if (behavior.kind === "prompt") { - return "prompt"; - } - - return getCategoryForMode(behavior.mode); + return fileExistsModes.filter((mode) => mode.id !== "leaveUnchanged"); } export function getDefaultBehaviorForCategory( diff --git a/src/types/ChoiceOutcome.ts b/src/types/ChoiceOutcome.ts index 881b176a9..4e43aa743 100644 --- a/src/types/ChoiceOutcome.ts +++ b/src/types/ChoiceOutcome.ts @@ -12,7 +12,7 @@ import type { TFile } from "obsidian"; * automation asks — it asks "did anything land?". Those came apart in two shipped * configurations that need no interactivity at all: a Capture whose formatted payload * is empty deliberately leaves the file untouched (inserting would add a blank line - * or, on `currentLine`, DELETE the selection), and a Template set to "Do nothing" when + * or, on `currentLine`, DELETE the selection), and a Template set to "Keep existing file" when * the file exists is, by name, a no-op. Both reported an unqualified success, so a * caller counting captures or writing an idempotency marker recorded work that never * happened (#1615). diff --git a/tests/e2e/file-exists-behavior.test.ts b/tests/e2e/file-exists-behavior.test.ts index feca3b6ba..a9f18e229 100644 --- a/tests/e2e/file-exists-behavior.test.ts +++ b/tests/e2e/file-exists-behavior.test.ts @@ -37,6 +37,7 @@ const behavior = { appendTop: { kind: "apply", mode: "appendTop" }, overwrite: { kind: "apply", mode: "overwrite" }, doNothing: { kind: "apply", mode: "doNothing" }, + leaveUnchanged: { kind: "apply", mode: "leaveUnchanged" }, prompt: { kind: "prompt" }, } as const; @@ -330,6 +331,52 @@ describe("functional: file collision behaviors", () => { }); }); +describe("with Open off, only Open existing file opens the note (#2244)", () => { + const expected = { + increment: { file: "open-off-increment1.md", effect: "created", opens: false }, + duplicateSuffix: { file: "open-off-duplicateSuffix (1).md", effect: "created", opens: false }, + appendBottom: { file: "open-off-appendBottom.md", effect: "changed", opens: false }, + appendTop: { file: "open-off-appendTop.md", effect: "changed", opens: false }, + overwrite: { file: "open-off-overwrite.md", effect: "changed", opens: false }, + doNothing: { file: "open-off-doNothing.md", effect: "unchanged", opens: true }, + leaveUnchanged: { file: "open-off-leaveUnchanged.md", effect: "unchanged", opens: false }, + } as const satisfies Record, unknown>; + const modes = Object.keys(expected) as Array; + + async function openSandboxNotes(): Promise { + return obsidian.dev.evalJson(`app.workspace.getLeavesOfType("markdown") + .map((leaf) => leaf.view.file?.path) + .filter((path) => path?.startsWith(${JSON.stringify(sandbox.path(""))}))`); + } + + beforeAll(async () => { + await qa.data().patch((data) => { + clearTestChoices(data); + data.choices.push( + ...modes.map((mode) => + templateChoice(`__qa-test-open-off-${mode}`, `${sandbox.root}/open-off-${mode}`, behavior[mode]), + ), + ); + }); + await qa.reload({ waitUntilReady: true }); + }, 15_000); + + beforeEach(async () => { + await obsidian.dev.eval(`app.workspace.getLeavesOfType("markdown") + .filter((leaf) => leaf.view.file?.path.startsWith(${JSON.stringify(sandbox.path(""))})) + .forEach((leaf) => leaf.detach())`); + }); + + it.each(modes)("%s", async (mode) => { + const { file, effect, opens } = expected[mode]; + await seedFile(`open-off-${mode}.md`, "EXISTING"); + const outcome = await obsidian.execJson("quickadd:run", { choice: `__qa-test-open-off-${mode}`, verify: true }); + expect(outcome).toMatchObject({ ok: true, effect, file: sandbox.path(file) }); + await obsidian.sleep(600); + expect(await openSandboxNotes()).toEqual(opens ? [sandbox.path(file)] : []); + }); +}); + // --------------------------------------------------------------------------- // Phase 2: Migration tests // ---------------------------------------------------------------------------