Skip to content

Document that SlideShowView::addImage is main thread only - #69

Merged
jdolan merged 1 commit into
mainfrom
doc/slideshow-add-image
Oct 17, 2026
Merged

jdolan merged 1 commit into
mainfrom
doc/slideshow-add-image

Conversation

@jdolan

@jdolan jdolan commented Oct 17, 2026

Copy link
Copy Markdown
Owner

The @remarks on SlideShowView::addImage said that it is thread-safe and may be called from a background thread. It is not: it adds to an Array with no lock, and render reads the same Array on the main thread.

This changes only the doc comment. Quetoo's installer and its new MapshotThumbnailView already collect images in a locked queue on the worker and add them on the main thread.

🤖 Generated with Claude Code

The remarks said that addImage may be called from a background thread,
but it adds to an Array without a lock while render reads the same
Array on the main thread.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings October 17, 2026 17:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The documentation-only change accurately describes the existing threading constraint and has no unresolved findings.

No open issues 🎉

What changed in this PR

Corrects the threading contract for SlideShowView::addImage without changing runtime behavior.

Changes:

  • Replaces the thread-safety claim with a main-thread requirement and guidance for workers to use a guarded queue.
File Description
Sources/​ObjectivelyMVC/​SlideShowView.h Corrects the addImage documentation to reflect unsynchronized image access.

Review effort: Balanced

@jdolan
jdolan merged commit 664f3fa into main Oct 17, 2026
4 checks passed
@jdolan
jdolan deleted the doc/slideshow-add-image branch October 17, 2026 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants