Skip to content

Expose monitorItem as open#134

Open
acoseac wants to merge 1 commit intoamosavian:masterfrom
acoseac:expose-monitor-item-public
Open

Expose monitorItem as open#134
acoseac wants to merge 1 commit intoamosavian:masterfrom
acoseac:expose-monitor-item-public

Conversation

@acoseac
Copy link
Copy Markdown

@acoseac acoseac commented Apr 21, 2026

What

Mark both `SMB2Manager.monitorItem` overloads (completion-handler + async) as `open` so they can be called from outside the AMSMB2 module.

Why

Every sibling API on `SMB2Manager` — `contentsOfDirectory`, `attributesOfItem`, `contents(atPath:range:)`, `downloadItem`, `move`, etc. — is declared `open`. The doc comments on `monitorItem` already describe it as part of the public API ("Monitor file/folder for changes…"), the result type `SMB2FileChangeInfo` and the filter type `SMB2FileChangeType` are both `public struct`, and the test coverage treats it alongside the public surface. The non-public visibility appears to be an oversight rather than a deliberate encapsulation choice.

Being unable to call `monitorItem` means client code can't use SMB2 CHANGE_NOTIFY push notifications without forking the package.

Concrete use case

I'm building a music library scanner that currently polls folders for changes. Moving to CHANGE_NOTIFY eliminates redundant `contentsOfDirectory` round-trips on rescans and lets the app react to new albums being dropped on the NAS within a few seconds. The implementation is ready locally — it just can't reach `monitorItem` behind the `internal` wall.

Change

Two three-character edits, both from default visibility to `open`. Matches the project's access-modifier convention for the rest of `SMB2Manager`'s async + completion-handler API. No behaviour change.

Verification

  • `xcodebuild -scheme AMSMB2 -destination 'platform=iOS Simulator,name=iPhone 17 Pro' build` — succeeds (no new warnings, same libsmb2 C warnings as before).

🤖 Generated with Claude Code

Both monitorItem overloads (completion-handler and async) were
declared at default internal visibility, preventing callers outside
the AMSMB2 module from using SMB2 CHANGE_NOTIFY push notifications.
Every sibling API on SMB2Manager — contentsOfDirectory, attributesOfItem,
contents(atPath:range:), downloadItem — is declared open, and the
doc comments on monitorItem already describe it as part of the
public-facing API. This matches the existing pattern and doesn't
change the signature or behaviour.

Motivation: enables client apps to build push-driven library
watchers (e.g. music scanners that react to folder changes without
polling) on top of AMSMB2 instead of having to fork the package
or vendor a patch.
@amosavian
Copy link
Copy Markdown
Owner

amosavian commented Apr 21, 2026

The test is not passing and that's why it is not open. You can find test here, and it is skipped by now. First we must fix that and then we can expose it. My first impression was that the issue is from libsmb2.

@acoseac
Copy link
Copy Markdown
Author

acoseac commented Apr 21, 2026

Proposed fix for the testMonitor failure in #135. Short version: monitorItem opens the directory with O_SYNC, which libsmb2 maps to FILE_NO_INTERMEDIATE_BUFFERING. Per MS-FSCC §2.1.5.1 that option doesn't apply to directories and Windows rejects the CREATE with STATUS_INVALID_PARAMETER before CHANGE_NOTIFY is ever sent. Dropping O_SYNC from the directory branch makes the CREATE succeed.

Note: with that fix applied there's a separate crash on the response path (Context.swift:392 in generic_handler) once the server actually fires a notification. Filed separately as #136 — doesn't block this PR, and testMonitor stays XCTSkipIf-gated until #136 is also resolved.

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