Skip to content

SnapSentry 0.17.3 - #5165

Open
mario0318 wants to merge 1 commit into
ramensoftware:mainfrom
mario0318:mod-snap-sentry-0.17.3
Open

SnapSentry 0.17.3#5165
mario0318 wants to merge 1 commit into
ramensoftware:mainfrom
mario0318:mod-snap-sentry-0.17.3

Conversation

@mario0318

@mario0318 mario0318 commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Changelog

If this pull request updates an existing mod, describe the changes below:

  • Recognizes .tif, .tiff, and .jfif images in a watched folder, alongside the PNG, JPEG, BMP, GIF, and WebP it already handled.

Mod authorship

If this pull request introduces a new mod, please complete the section below.

This mod was created by:

    • The submitter, without AI assistance
    • The submitter, with AI assistance
    • Claude
    • ChatGPT
    • Gemini
    • Another AI (please specify):
    • Other (please specify):

Please select the options that best apply. Your selection does not affect the acceptance criteria, but it helps reviewers understand the context of the code and provide relevant feedback.

@windhawk-reviewer windhawk-reviewer Bot added the waiting-for-author The author's turn: request an AI review, or respond to one that was posted. label Aug 20, 2026
@windhawk-reviewer

Copy link
Copy Markdown

Thanks for the pull request! This repository uses a two-stage review: an AI review that you run yourself, followed by a human review.

To get started, comment /ai-review. Once you're happy with the result, comment /ready-for-reviewer to hand it over to a human reviewer.

See the pull request review process for the full details.

@mario0318

Copy link
Copy Markdown
Contributor Author

/ai-review

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-ai-review An AI review was requested and is being prepared. and removed waiting-for-author The author's turn: request an AI review, or respond to one that was posted. labels Aug 20, 2026
@windhawk-reviewer

Copy link
Copy Markdown

Submission review

Note: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding.

Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it.

Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them.


No blocking issues — looks good to merge. I diffed the full source against the merged 0.17.2 and the only change below the version line is the extension list in IsSupportedImage, so the review is about whether the three new extensions are safe to let through the existing pipeline. They are: nothing downstream keys off the extension. ClipboardImage goes through CreateDecoderFromFilename with a null vendor GUID, so WIC picks the decoder from the file's content rather than its name, and the in-box JPEG and TIFF decoders cover .jfif and .tif/.tiff; GetFrame(0) is the right choice for a multi-page TIFF; the 30000 x 30000 dimension guard still holds, and stride * height can't overflow a 32-bit size_t under it. RenameByActiveWindow reuses whatever ext it found, and the Enqueue filters (IsSafeChildName, JustCreated, g_preexisting, g_inflight, g_recent) are all name-based and format-agnostic. The lowercase compare is fine too — CharLowerBuffW on a std::wstring's own buffer, and all nine literals are lowercase ASCII. The tool-mod boilerplate is still a verbatim copy of the wiki snippet.

Optional improvements

Minor polish — none of this affects users, so it's your call.

  • .jfif was added but not its siblings. Windows registers .jpe, .jfi and .jif as JPEG extensions alongside .jpg/.jpeg/.jfif, and .dib alongside .bmp. Since the list is a pure name filter (the decoder is chosen by content), adding them costs nothing and makes the JPEG/BMP coverage complete rather than partial:

    return ext == L".png" || ext == L".jpg" || ext == L".jpeg" ||
           ext == L".jpe" || ext == L".jfif" || ext == L".jfi" ||
           ext == L".jif" || ext == L".bmp" || ext == L".dib" ||
           ext == L".gif" || ext == L".webp" || ext == L".tif" ||
           ext == L".tiff";

    At nine (or thirteen) entries the chain is also getting long enough that a static constexpr std::wstring_view kExtensions[] plus std::ranges::find would read better than the || chain, but that's taste.

  • The PR changelog undersells the previous state. "so more saved captures in a watched folder are handled, not just PNG and JPEG" — .bmp, .gif and .webp were already recognized in 0.17.2. Worth correcting so the human reviewer doesn't have to check.

  • Carried over from the 0.17.2 round, still open: the watch loop's error backoff never escalates. openFailures = 0; runs right after every successful CreateFileW, and the inner-error path waits a flat 2 s:

    CloseHandle(dir);
    if (!reopen) {
        // ... Back off like the open-failure path so a persistently
        // failing watch can't peg a core re-opening and re-enumerating in a spin.
        HANDLE idle[] = {g_stopEvent, g_reloadEvent};
        WaitForMultipleObjects(2, idle, FALSE, 2000);
    }

    The comment says "back off like the open-failure path", but the open-failure path is the one with the 2 s -> 30 s escalation; this one is fixed at 2 s. So on a path where the handle opens but ReadDirectoryChangesW never works (network redirectors and some virtual/sync filesystems return ERROR_INVALID_FUNCTION), the thread re-opens the directory and re-enumerates every file in it via SnapshotExistingNames 30 times a minute, forever. Either reuse the escalating expression here, or move openFailures = 0; to the point where a notification is actually received so the same counter covers both failure shapes.

Functionality notes

Non-critical observations and ideas about the feature behavior itself.

  • .tif/.tiff are well motivated; .jfif mostly widens the download hazard. ShareX and Greenshot both offer TIFF as a capture output format, so that half of the change has a clear capture-tool story. .jfif, though, is overwhelmingly produced by Chromium's "Save image as" rather than by any capture tool — so in practice it mainly increases the chance that a browser-saved image landing in the watched folder is treated as a capture and, with deleteFile on, recycled a few seconds later. That is exactly the case the README's Privacy section warns about ("A brand new file saved or downloaded straight into the folder cannot be told apart from a capture"), so it's consistent with the documented behavior — just worth being deliberate about, since it's the one extension here that isn't a capture format.

  • TIFF is the first format in the list that routinely carries very large images, and the copy path peaks at 2x the decoded size. ClipboardImage allocates malloc(pixels) for the top-down buffer and GlobalAlloc(sizeof(BITMAPV5HEADER) + pixels) for the DIB, both live at once. A 10000x8000 scan is ~320 MB each, so ~640 MB peak in the x86 tool process — enough to fail or badly fragment the address space, where a PNG screenshot never got close. The failure is safe (copied == false -> the file is never deleted), just silent apart from a log line. If you want to halve the peak, the second buffer can go away entirely: IWICImagingFactory::CreateBitmapFlipRotator + Initialize(converter, WICBitmapTransformFlipVertical) produces the bottom-up order the DIB wants, so you can CopyPixels straight into v5 + sizeof(BITMAPV5HEADER) and drop topDown and WriteBottomUp.

  • g_preexisting never forgets a name, even after the file is gone. A name captured by SnapshotExistingNames is ignored for the whole watch session (until a settings change or a watch error re-opens the folder). That is the intended safety invariant while the file exists, but it keeps applying after the user deletes it — so a genuinely new capture that reuses the name is silently skipped with no log line, and a capture tool configured to write a fixed name (capture.png) into a folder that already had one at startup never works at all. The watcher already receives the events needed to fix this, since FILE_NOTIFY_CHANGE_FILE_NAME reports removals too:

    if (info->Action == FILE_ACTION_REMOVED ||
        info->Action == FILE_ACTION_RENAMED_OLD_NAME) {
        std::wstring name(info->FileName, info->FileNameLength / sizeof(wchar_t));
        EnterCriticalSection(&g_lock);
        g_preexisting.erase(name);
        LeaveCriticalSection(&g_lock);
    }

    The invariant is preserved — the file really is gone, so a later ADDED for that name is a new file, and JustCreated still has to agree before anything is queued.

  • Two items from the 0.16.0/0.17.2 rounds are unchanged, noting them only so the human reviewer doesn't have to re-derive them from the diff: RenameByActiveWindow still samples GetForegroundWindow() inside ProcessOne (after WaitForStableFile, and behind any earlier screenshot's countdown) rather than at event time in Enqueue, and the clipboard helpers still make a single OpenClipboard attempt with no retry.


Next steps:

  • /ai-review - after pushing fixes, to get a review of the updated code. You can repeat this as many times as you need, but each review is thorough and usually there's no need for more than 2-3 iterations.
  • /ready-for-reviewer - once you're satisfied with the state of the pull request, to hand it over to a human reviewer. If some findings above are left unaddressed, add a short note explaining why.

See the review process for details.

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-author The author's turn: request an AI review, or respond to one that was posted. and removed waiting-for-ai-review An AI review was requested and is being prepared. labels Aug 20, 2026
@mario0318
mario0318 force-pushed the mod-snap-sentry-0.17.3 branch from 881239d to 52fa391 Compare August 20, 2026 05:13
@mario0318

Copy link
Copy Markdown
Contributor Author

/ai-review

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-ai-review An AI review was requested and is being prepared. and removed waiting-for-author The author's turn: request an AI review, or respond to one that was posted. labels Aug 20, 2026
@windhawk-reviewer

Copy link
Copy Markdown

Submission review

Note: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding.

Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it.

Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them.


No blocking issues — looks good to merge. The whole change is the version bump plus three entries in IsSupportedImage, and nothing else in the file moved. The three new extensions work end to end: CreateDecoderFromFilename picks up WIC's built-in TIFF decoder for .tif/.tiff and the JPEG decoder for .jfif, the 32bppBGRA conversion and CF_DIBV5 payload are format-agnostic from there, and the file / path / none modes never look at the format at all. The per-dimension 30000 cap still keeps stride * height inside a 32-bit size_t, so the wider format set doesn't open an overflow. IsSafeChildName, the g_preexisting / g_inflight / g_recent gating and the JustCreated freshness window are all upstream of the extension check, so the new formats inherit the same "never touch a file that was already there" invariant. The settings block and the code are still in sync (all nine keys declared, all nine read, $options only on the two string settings), the tool-mod boilerplate is still a verbatim copy of the wiki snippet, and the changelog matches what the diff does.

Optional improvements

Minor polish — none of this affects users, so it's your call.

  • The README never says which files count, and that list is now nine entries long. With deleteFile on, the extension list is precisely what decides whether a file dropped in the watched folder can be recycled, but the README only says "any supported image" and "every new image that arrives". Since the Privacy section is already asking the user to judge whether their chosen folder is safe, one line naming the formats (.png, .jpg, .jpeg, .jfif, .bmp, .gif, .webp, .tif, .tiff) gives them what they need to make that call, and it's the natural place for this PR's change to show up in the docs.

  • The extension comparison lowercases through the user's locale. CharLowerBuffW maps via the locale, and all three new extensions contain an i; on a Turkish or Azerbaijani locale an uppercase I lowercases to ı, so .TIF becomes .tıf and matches nothing (the existing .GIF has the same shape). The failure direction is safe — the file is ignored, never wrongly deleted — so it's a nit, but an ordinal comparison sidesteps it entirely: either _wcsicmp(ext.c_str(), L".tif") == 0, or lower with towlower the way classic-photo-viewer does (both run in the CRT's default "C" locale, so they stay ASCII-only).

  • openFailures still never escalates on the notification-failure path. Raised in the 0.17.2 round and left as-is, so just a reminder rather than a re-explanation: openFailures = 0; runs after every successful CreateFileW, so when the handle opens but ReadDirectoryChangesW fails (a redirector or sync filesystem returning ERROR_INVALID_FUNCTION, or a folder that points at a file), the inner loop breaks, waits a flat 2 s, and re-opens plus re-enumerates the folder 30 times a minute forever. Not resetting the counter until a notification has actually arrived, and reusing the same 2 s → 30 s expression on the inner-error path, fixes both.

  • A watched folder that can't be opened is completely silent. The CreateFileW failure path just backs off — no Wh_Log, not even with logDetails on — so a typo'd path, a quoted path pasted from Explorer, or an unavailable network location is indistinguishable from "the mod is running and nothing has been captured". The overflow case a few lines down already logs, so this is the one gap; a single line on the first failure (with GetLastError, and the path gated on logDetails like the other messages) makes a misconfigured folder diagnosable.

  • The fallback dialog rewrites its content text on every callback timer tick. TDF_CALLBACK_TIMER fires roughly every 200 ms, and DialogCallback sends TDM_SET_ELEMENT_TEXT each time even though left only changes once a second — four or five redundant repaints per second. Caching the last left in DialogState and only sending the message when it changes is a two-line change.

Functionality notes

Non-critical observations and ideas about the feature behavior itself.

  • .jfif mostly widens exposure rather than adding a capture format. .tif/.tiff map onto real capture-tool output (ShareX and Greenshot both offer TIFF), but .jfif is the extension Chrome and Edge have historically produced for "Save image as" on a JPEG — a browser-download extension, not something a screenshot tool writes. Since the extension list is exactly what gates auto-deletion, adding it mainly increases the surface for the case the README warns about (a folder that also receives saves and downloads: fresh creation time and fresh write time, so JustCreated can't tell it from a capture). Worth confirming it's deliberate; and if the intent was JPEG-alias completeness rather than a specific tool, .jpe and .jif are the same family and aren't in the list.

  • TIFF is the first format here that routinely comes in sizes far past a screenshot. The image mode holds two full 32bpp copies at once — malloc(pixels) plus GlobalAlloc(sizeof(BITMAPV5HEADER) + pixels) — inside a 32-bit process, and the only guard is the per-dimension 30000 cap, so a 30000×30000 scan asks for ~3.4 GB twice and a 5000×7000 600 dpi page still costs ~270 MB. The failure is safe (the copy fails, so nothing is deleted) but silent apart from a log line. A byte budget next to the dimension cap would fail fast with a clearer message:

    ULONGLONG bytes = (ULONGLONG)width * height * 4;
    if (bytes > 512ull * 1024 * 1024) {
        Wh_Log(L"Image too large to copy (%ux%u)", width, height);
        break;
    }
  • If the goal is "whatever WIC can decode", the list is still partial. Windows also decodes .dib, .ico, .jxr/.wdp out of the box, and .heic/.avif when the extension is installed — classic-photo-viewer carries .jfif, .tif, .tiff, .dib, .ico, .wdp and .jxr for the same job. Not that captures typically arrive as those, but it does suggest this list will keep growing one version bump at a time. That mod's approach — a settings-driven list with the built-in set as the default — would let a user with an unusual capture tool add their own extension without a PR, and would put the answer to "which files can this delete?" directly in the settings UI, which also covers the README point above.


Next steps:

  • /ai-review - after pushing fixes, to get a review of the updated code. You can repeat this as many times as you need, but each review is thorough and usually there's no need for more than 2-3 iterations.
  • /ready-for-reviewer - once you're satisfied with the state of the pull request, to hand it over to a human reviewer. If some findings above are left unaddressed, add a short note explaining why.

See the review process for details.

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-author The author's turn: request an AI review, or respond to one that was posted. and removed waiting-for-ai-review An AI review was requested and is being prepared. labels Aug 20, 2026
@mario0318

Copy link
Copy Markdown
Contributor Author

/ai-review

@mario0318
mario0318 force-pushed the mod-snap-sentry-0.17.3 branch from 52fa391 to 96af5f9 Compare August 20, 2026 08:06
@windhawk-reviewer windhawk-reviewer Bot added waiting-for-ai-review An AI review was requested and is being prepared. and removed waiting-for-author The author's turn: request an AI review, or respond to one that was posted. labels Aug 20, 2026
@windhawk-reviewer

Copy link
Copy Markdown

Submission review

Note: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding.

Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it.

Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them.


Small, self-contained change: version bump, one README paragraph, and a rewritten IsSupportedImage. The code change checks out — name.c_str() + dot is the extension tail (find_last_of already guaranteed the dot exists), and the nine _wcsicmp comparisons are equivalent to the old CharLowerBuffW + == for any name a real filesystem can produce. IsSupportedImage is reached only from Enqueue, so the widened list feeds the same IsSafeChildNameJustCreatedg_preexisting/g_inflight/g_recent chain as before, and the README's new list matches the code exactly. The tool-mod boilerplate is still a verbatim copy of the wiki snippet. One thing about what the wider list now lets the mod delete.

Multi-page TIFF: the copy keeps page 1, the delete takes all of them.

ClipboardImage decodes exactly one frame:

if (FAILED(decoder->GetFrame(0, &frame)) || ...

so for a multi-page file the clipboard ends up holding page 1 while DeleteWatched removes the whole document — recoverable from the Recycle Bin by default, permanent with recycle: false. It passes the mod's own invariant ("never delete when a requested copy failed") because copied is true: the copy succeeded, it just wasn't the whole file. Two ways in: auto-delete with deleteFile on in image/none mode, and the popup's Copy + delete, which sets forceImage and deletes regardless of the configured clipboard mode.

This matters now because .tif/.tiff is the format where multi-page is routine rather than exotic — Windows Fax and Scan, most MFP scanner utilities, and "print to TIFF" drivers all emit multi-page TIFFs, and any of them landing in the watched folder within kFreshCaptureWindowMs looks like a capture. (Animated .gif/.webp have the same shape and predate this PR, but a lost animation is a smaller loss than pages 2–N of a scan.)

Cheapest fix is to let the decoder tell you and treat "more than one frame" the way file/path mode is already treated — copy, don't delete:

static bool ClipboardImage(const std::wstring& path, bool* multiFrame) {
    ...
    UINT frames = 0;
    if (FAILED(decoder->GetFrameCount(&frames)) || frames == 0) {
        break;
    }
    *multiFrame = frames > 1;   // Clipboard will hold frame 0 only.
    if (FAILED(decoder->GetFrame(0, &frame)) || ...
    bool multiFrame = false;
    if (forceImage || s.mode == L"image") {
        copied = ClipboardImage(path, &multiFrame);
    }
    ...
    // forceImage short-circuits payloadReferencesFile, so this has to be its own term.
    bool wantsDelete = action != ACTION_COPY_ONLY && !multiFrame &&
                       (forceImage || (s.deleteFile && !payloadReferencesFile));

plus a Wh_Log line so the skipped deletion isn't silent. "Delete now" is an explicit choice with no copy involved, so leave that path alone. If that's more surgery than you want for this release, the alternative is to drop .tif/.tiff and keep .jfif (JFIF is single-frame JPEG, so it has none of this).

Optional improvements

Minor polish — none of this affects users, so it's your call.

  • "this is exactly the set of files that can be removed" is precise about extensions and vague about everything else. Read quickly it says "files with these extensions get removed", when JustCreated and the pre-existing-names snapshot narrow it a lot further. Something like "only files with these extensions are touched, and only ones that look freshly created" keeps the useful warning without overstating it.

  • The extension list only exists in the README. The folder $description is where someone is actually deciding which folder to point at, and it currently says only "other images saved there are deleted too". Half a sentence naming the types there would put the fact at the point of the decision rather than on the details page.

  • "Ordinal" isn't quite the right word for _wcsicmp. It's documented as locale-dependent (LC_CTYPE), not ordinal — the conclusion in the comment still holds, but for the reason the second half of the comment gives: the mod never calls setlocale, so the CRT stays in "C" and the folding is ASCII-only. Either drop the first word, or make it true by construction with CompareStringOrdinal(ext, -1, L".png", -1, TRUE) == CSTR_EQUAL (or _wcsicmp_l against an invariant locale).

  • Keep the list in one place. Nine chained _wcsicmp calls read fine but are easy to update in the code and forget in the README (and vice versa):

    static constexpr const wchar_t* kImageExtensions[] = {
        L".png", L".jpg", L".jpeg", L".jfif", L".bmp",
        L".gif", L".webp", L".tif", L".tiff",
    };
    ...
    const wchar_t* ext = name.c_str() + dot;
    for (const wchar_t* candidate : kImageExtensions) {
        if (_wcsicmp(ext, candidate) == 0) {
            return true;
        }
    }
    return false;

Functionality notes

Non-critical observations about the feature behavior itself.

  • The 2 s stability budget gets tighter with the formats you just added. WaitForStableFile gives up after 40 × 50 ms and then falls through to "proceed if the file still exists". An uncompressed scan-shaped TIFF is tens of MB, and on a slow or sync-backed volume it can still be growing at that point, so the file that gets decoded is whatever was on disk at 2 s. What saves the delete path there is the assumption in the comment above it — a half-written file fails to decode, so copied is false and nothing is deleted. That holds for a truncated TIFF (strip offsets pointing past EOF), so this is an FYI rather than a bug; if you want to close it anyway, returning false when the size never settled is stricter than proceeding, at the cost of skipping the occasional slow-but-complete write.
  • .heic/.avif are the visible omissions in the new list if the intent is "image formats Windows tools produce" — Photos saves HEIF, and phone-synced captures arrive as HEIC. Both only decode through WIC when the corresponding codec package is installed, and a failed decode means a failed copy means no deletion, so adding them fails safe. Not a request, just where the list's edge now sits.
  • Carry-overs from the 0.17.2 round, unchanged in this diff (noted so the human reviewer doesn't re-derive them from the source): the rename samples the foreground window at processing time rather than capture time; the clipboard copy is a single OpenClipboard attempt with no retry; and openFailures = 0 right after a successful CreateFileW means the 2 s → 30 s escalation only ever applies to open failures, so a folder that opens but whose ReadDirectoryChangesW keeps failing re-opens and re-enumerates every 2 s indefinitely.


Next steps:

  • /ai-review - after pushing fixes, to get a review of the updated code. You can repeat this as many times as you need, but each review is thorough and usually there's no need for more than 2-3 iterations.
  • /ready-for-reviewer - once you're satisfied with the state of the pull request, to hand it over to a human reviewer. If some findings above are left unaddressed, add a short note explaining why.

See the review process for details.

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-author The author's turn: request an AI review, or respond to one that was posted. and removed waiting-for-ai-review An AI review was requested and is being prepared. labels Aug 20, 2026
## Changelog

- Recognizes .tif, .tiff, and .jfif images in a watched folder, alongside the PNG, JPEG, BMP, GIF, and WebP it already handled.
@mario0318
mario0318 force-pushed the mod-snap-sentry-0.17.3 branch from 96af5f9 to cc10cee Compare August 20, 2026 09:28
@mario0318

Copy link
Copy Markdown
Contributor Author

/ai-review

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-ai-review An AI review was requested and is being prepared. and removed waiting-for-author The author's turn: request an AI review, or respond to one that was posted. labels Aug 20, 2026
@windhawk-reviewer

Copy link
Copy Markdown

This pull request has already had 3 AI reviews in the last 24 hours, which is the limit, so no review was posted this time.

Comment /ai-review again after 2026-08-21 04:20 UTC (in 19 hours) to get another one.

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-author The author's turn: request an AI review, or respond to one that was posted. and removed waiting-for-ai-review An AI review was requested and is being prepared. labels Aug 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

waiting-for-author The author's turn: request an AI review, or respond to one that was posted.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant