Skip to content

feat: Expose subtitle state change event - #177

Merged
Balamurugan-100 merged 3 commits into
mainfrom
feat-subtitle-changed-event
Sep 10, 2026
Merged

feat: Expose subtitle state change event#177
Balamurugan-100 merged 3 commits into
mainfrom
feat-subtitle-changed-event

Conversation

@Balamurugan-100

Copy link
Copy Markdown
Member
  • Hosts could not observe when subtitles were enabled, disabled, or switched to another language, making it difficult to persist the user's subtitle preference.
  • Subtitle state changes were handled internally without notifying the host app.
  • Expose subtitle state changes to host apps, report only real changes, and keep existing integrations backward compatible.

* Hosts could not observe when subtitles were enabled, disabled, or switched to another language, making it difficult to persist the user's subtitle preference.
* Subtitle state changes were handled internally without notifying the host app.
* Expose subtitle state changes to host apps, report only real changes, and keep existing integrations backward compatible.
@claude

claude Bot commented Sep 10, 2026

Copy link
Copy Markdown

Claude finished @Balamurugan-100's task in 2m 13s —— View job


Claude finished reviewing this PR

Todo list

  • Gather context (diff, related files)
  • Understand the request (code review)
  • Review code quality, resource lifecycle, public API/backward compatibility, and security
  • Post review feedback

Summary

This PR adds a subtitleStateChanged delegate callback (UIKit) and an onSubtitleStateChanged closure (SwiftUI) so hosts can observe subtitle enable/disable/language-switch events. The public API additions are backward-compatible (default protocol extension for the delegate method, optional closure for SwiftUI), which is good. However, the two implementations diverge in an important way, and there's one path that isn't covered at all.

Findings

1. UIKit: auto-selected subtitles never fire the delegate (architectural gap)

In Source/TPStreamPlayerViewController.swift:94-100, the delegate is only invoked from the onSubtitleTrackSelected closure, which is wired to PlayerControlsUIView's manual subtitle-menu selection (Source/Views/UIKit/PlayerControlsUIView.swift:199-200).

But activeSubtitleTrack is also mutated directly in two other places that bypass this closure entirely:

  • setupPlayerStatusObserver (TPStreamPlayerViewController.swift:207-211) when autoSelectFirstSubtitle picks the first track on player ready.
  • Any host app that sets playerViewController.activeSubtitleTrack = track directly, since it's a public var.

Neither path calls delegate?.subtitleStateChanged(...). Notably, the bundled StoryboardExample/PlayerViewController.swift enables .autoSelectFirstSubtitle(true), so in the example app itself, the very case the PR is meant to solve (persisting subtitle state) would silently never fire for the auto-selected default track — only for subsequent manual changes.

Compare this with the SwiftUI implementation (TPStreamPlayerView.swift:105-108), where .onChange(of: activeSubtitleTrack) observes the @State var directly, so it does catch the auto-select case. This makes the two platform APIs behave inconsistently for the same configuration.

A more robust fix would move the notification into activeSubtitleTrack's didSet (TPStreamPlayerViewController.swift:35-43), comparing against oldValue, so every mutation path (manual selection, auto-select, and direct host assignment) is covered uniformly — mirroring what the closure currently does ad hoc. That would also let the onSubtitleTrackSelected closure body revert to its original simple assignment.

Fix this →

2. SwiftUI: unclear intent behind the hasAppeared guard

TPStreamPlayerView.swift:105-109:

.onChange(of: activeSubtitleTrack) { track in
    guard hasAppeared || track != nil else { return }
    onSubtitleStateChanged?(track != nil, track?.language)
}
.onAppear { hasAppeared = true }

activeSubtitleTrack starts as nil and SwiftUI's onChange never fires for the initial value — only for actual transitions. So the only way this onChange runs before hasAppeared is true would require onAppear (a sibling modifier on the same view) to fire after a state mutation triggered by onChange(of: viewModel.player.initializationStatus), which isn't a guaranteed/documented ordering. As written, the !hasAppeared && track == nil branch this guards against appears unreachable in the normal flow, making the extra state variable and guard confusing without an explanatory comment. If there's a specific race this is meant to prevent (e.g., a fast synchronous "ready" status before the first render commits), please add a comment describing it — otherwise consider simplifying by removing hasAppeared entirely.

3. Minor: SwiftUI API shape

public var onSubtitleStateChanged: ((Bool, String?) -> Void)? (TPStreamPlayerView.swift:17) is a plain mutable stored property on the View struct. It works (callers can set it before the view is used), but the more idiomatic/discoverable pattern for this SDK's SwiftUI surface would be a chainable modifier, e.g. .onSubtitleStateChanged { enabled, language in ... }, consistent with how the rest of SwiftUI's own onChange/onReceive APIs read. Not blocking, just a consistency suggestion.

Other checks

  • No force-unwraps introduced, [weak self] already used correctly in the touched closure.
  • No new KVO/time-observer lifecycle changes.
  • No access tokens/DRM material touched.
  • Backward compatibility: preserved via default protocol extension method and optional closure.
    · Branch: feat-subtitle-changed-event

@Balamurugan-100

Copy link
Copy Markdown
Member Author

@claude review and provide a verdict

@claude

claude Bot commented Sep 10, 2026

Copy link
Copy Markdown

Claude finished @Balamurugan-100's task in 1m 15s —— View job


Review verdict: ✅ Approve

Todo list

  • Gather context (diff, related files)
  • Understand the request
  • Review code quality, resource lifecycle, public API/backward compatibility, security
  • Post review feedback with verdict

Summary

This PR adds a subtitleStateChanged delegate callback (UIKit) and an onSubtitleStateChanged closure (SwiftUI) so hosts can observe subtitle enable/disable/language-switch events. I re-reviewed against the latest commit (620c5d7 fix: AI comments), which addresses both issues raised in the earlier automated review on this PR.

What changed since the last review

  1. UIKit auto-select gap — fixed. The notification now lives in activeSubtitleTrack's didSet (Source/TPStreamPlayerViewController.swift:35-45), guarded by guard activeSubtitleTrack != oldValue else { return }. This covers every mutation path uniformly: manual selection via PlayerControlsUIView, autoSelectFirstSubtitle in setupPlayerStatusObserver, and direct assignment by host apps. The onSubtitleTrackSelected closure (line 96-98) is back to a simple self?.activeSubtitleTrack = track, so there's no duplicate notification path anymore. SubtitleTrack already conforms to Equatable (Source/Network/Models/SubtitleTrack.swift:3), so the oldValue comparison is valid.
  2. SwiftUI hasAppeared guard — removed. Source/TPStreamPlayerView.swift:104-106 now just calls onSubtitleStateChanged?(track != nil, track?.language) inside .onChange(of: activeSubtitleTrack), relying on SwiftUI's built-in dedup (an onChange doesn't fire for the initial value, only on real transitions), which is correct and simpler than the previous state-flag workaround.

Both platforms now behave consistently for the autoSelectFirstSubtitle case that motivated this PR.

Remaining (non-blocking) observation

  • SwiftUI API shape (Source/TPStreamPlayerView.swift:17): onSubtitleStateChanged is a plain mutable stored property on the View struct. It works, but a chainable modifier (.onSubtitleStateChanged { enabled, language in ... }) would be more idiomatic and consistent with SwiftUI's own onChange/onReceive style. Purely a style suggestion, not a correctness issue — fine to leave as-is or address in a follow-up.

Other checks

  • Backward compatibility preserved: new delegate method has a default empty implementation via protocol extension (Source/TPStreamPlayerViewController.swift:361-364); new SwiftUI closure is optional.
  • No force-unwraps introduced; [weak self] used correctly in the touched closure.
  • No new KVO/time-observer lifecycle changes, no access tokens/DRM material touched.
  • CHANGELOG.md is updated separately at release/version-bump time in this repo's workflow, so no entry is expected in this PR.

Verdict: Approve. Both previously identified issues are resolved and the implementation is now consistent across UIKit and SwiftUI.

@Balamurugan-100
Balamurugan-100 merged commit 6b77603 into main Sep 10, 2026
2 of 3 checks passed
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