Add watermark support for videos

[pugal66]

• Introduce native watermark overlays for the video player using the TPStreams SDK
• Extend institute settings to consume backend-driven watermark configurations
• Map both fixed-position (static) and animated (dynamic) layouts natively based on the configuration type

[gemini-code-assist]

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

[claude]

Claude finished @pugal66's task in 2m 59s —— View job

---

Claude finished the review

• Read AGENTS.md, ai_context.md, architecture.md
• Check OpenSpec compliance
• Fetch full PR diff
• Review SDK boundary / import rules
• Review design system & widget usage
• Review accessibility semantics
• Review data layer / offline consistency (institute_settings)
• Review generated files / dependency changes
• Post structured review summary

PR Review Summary

✅ What's good

• OpenSpec present: openspec/changes/video-watermark/ includes proposal.md, design.md, a capability spec (specs/video-playback-watermark/spec.md), and tasks.md — this is a new feature/architectural addition and it's properly spec-driven, satisfying the [CRITICAL] OpenSpec requirement.
• Correct layering for the feature itself: watermark config is parsed once in InstituteSettings.fromJson/toJson (institute_settings.dart) and flows through the existing DataSource → Repository → Provider chain — no direct DataSource/DB access from the widget.
• No design-system violations: no Material/Cupertino widgets introduced, and the one token read (Design.of(context).typography.labelBold.fontSize) correctly goes through Design.of(context) rather than a static import.
• No platform-specific branching, no hardcoded strings, no .g.dart hand-edits.
• Good test coverage: institute_settings_test.dart and watermark_mapper_test.dart cover the enum parsing, case-insensitivity, malformed input, and all 5 static positions + dynamic + hidden mapping branches.
• Async safety: _controller!.setWatermarks(configs).catchError(...) reports failures to Sentry instead of swallowing them, and checks mounted before touching state.
• Dependency bump is consistent: tpstreams_player_sdk bumped from 2.2.22 → 2.2.26 identically in both core and courses pubspecs (needed for the native setWatermarks/WatermarkConfig API).

🔴 CRITICAL — Must fix before merge

None found.

🟡 WARNING — Should fix

1. SDK boundary bypass (inline comment): watermark_mapper.dart imports package:core/data/config/institute_settings.dart directly, and custom_video_player.dart imports package:core/data/data.dart directly, instead of going through package:core/core.dart as documented in AGENTS.md/architecture.md. This is a pre-existing, repo-wide pattern (50+ files in courses already do this), so it's not a regression from this PR, but it does diverge from the documented rule and is worth a follow-up (either export these from core.dart, or update the docs to match reality).
2. Dynamic watermark animation ignores reduce-motion preference (inline comment): the dynamic type triggers a continuous ping-pong animation via the native player SDK, but _applyWatermark() never checks MotionPreferences.shouldAnimate(context). Since this is delegated to the native SDK rather than a Flutter AnimationController, it may not be trivial to gate, but per the repo's motion rules this should at least fall back to a static position (or be documented as a known SDK limitation) when the user has reduce-motion enabled.
3. Spec/implementation drift on watermark text: openspec/changes/video-watermark/specs/video-playback-watermark/spec.md (line 30) says the watermark text "SHALL be the current user's email, username, or identifier," but custom_video_player.dart (_applyWatermark) only ever uses user?.username, falling back to the literal string 'user'. Not a bug, but the spec is broader than what's implemented — worth tightening the spec or confirming username-only is intentional.

🔵 SUGGESTION — Nice to have

1. No compile-time feature flag: per the repo convention of gating new conditionally-shown UI behind AppConfig/bool.fromEnvironment, the watermark feature has no kill switch independent of the backend video_watermark_type field. Since hidden/null already fully disables it server-side, this is lower priority, but a local override could help with staged rollout or quick disable if the SDK integration misbehaves in production.
2. In WatermarkMapper.getWatermarkConfig (watermark_mapper.dart), the dynamic case sets y: 50 but omits x, relying on the SDK's default. Consider setting x: 50 explicitly for symmetry/clarity with the static branch's _setPositionValues, since an implicit SDK default is easy to miss on future edits.

Verdict

APPROVED — no critical SDK-boundary, design-system, or spec-compliance blockers. The two WARNING items (pre-existing import pattern, reduce-motion handling) are good follow-ups but don't need to block this merge.

[pugal66]

@claude review

[claude]

Claude Code is working…

I'll analyze this and get back to you.

View job run

[pugal66]

@claude review

[claude]

Claude finished @pugal66's task in 1m 3s —— View job

---

Claude finished @pugal66's task

• Read AGENTS.md, ai_context.md, architecture.md
• Check OpenSpec compliance
• Fetch full PR diff (including latest "Address review comments" commit)
• Review SDK boundary / import rules
• Review design system & widget usage
• Review accessibility semantics
• Review data layer / offline consistency
• Review generated files / dependency changes
• Post structured review summary

PR Review Summary

This re-review covers the latest commit 87f6414 ("Address review comments"), which responded to both WARNING items from the prior review.

✅ What's good

• Motion preference bypass now justified: custom_video_player.dart:310 adds an explicit comment explaining the dynamic watermark intentionally ignores MotionPreferences.shouldAnimate(context) to prevent users from disabling anti-piracy watermarks via accessibility settings. That's a reasonable, deliberate product/security tradeoff, and documenting it (rather than silently skipping the check) resolves the ambiguity flagged earlier.
• Import switched to the existing barrel convention: watermark_mapper.dart and its test now import package:core/data/data.dart instead of the deep package:core/data/config/institute_settings.dart path. I verified package:core/core.dart genuinely does not re-export anything under data/ (config, DTOs, repositories, sources), and that 53 other files in packages/courses already import package:core/data/data.dart directly — so this matches the established (if imperfect) repo convention rather than introducing a new pattern.
• Spec updated to match implementation: openspec/changes/video-watermark/specs/video-playback-watermark/spec.md now says the watermark text is the username with 'user' fallback, matching _applyWatermark() exactly — the spec/implementation drift is resolved.
• Rest of the PR (OpenSpec docs, SDK boundary for the institute_settings.dart parsing, no Material/Cupertino usage, Design.of(context) token access, async error handling via Sentry, test coverage) is unchanged from the initial review and remains solid.

🔴 CRITICAL — Must fix before merge

None.

🟡 WARNING — Should fix

1. SDK boundary still technically bypassed (watermark_mapper.dart:1, custom_video_player.dart): importing package:core/data/data.dart instead of package:core/core.dart still diverges from the documented rule (courses → package:core/core.dart only). This is a pre-existing, repo-wide gap (core.dart simply doesn't export the data/ barrel at all), so it's not something this PR should be expected to fix in isolation — but it remains a good candidate for a follow-up: either export data.dart's public surface from core.dart, or update the documented import rule to reflect that data/data.dart is an accepted second entry point.

🔵 SUGGESTION — Nice to have

1. WatermarkMapper.getWatermarkConfig's dynamic case still omits x (relies on SDK default) while static sets both x/y explicitly — minor inconsistency, not worth blocking on.

Verdict

APPROVED — both previously-flagged WARNING items were addressed with reasonable justification (documented intentional motion bypass, and an import fix that aligns with existing repo convention even though the underlying core.dart export gap is out of scope for this PR). No CRITICAL blockers.

[syed-tp]

check this: https://chatgpt.com/share/6a688346-08a0-83ee-95c1-b0a5920c13f7

NOTE: as the player sdk is updated dont we need the lock file to be added in this commit as well?

[pugal66]

@claude review

[claude]

Claude finished @pugal66's task in 3m 28s —— View job

---

Claude finished @pugal66's task

• Read AGENTS.md, ai_context.md, architecture.md
• Fetch latest PR diff (via gh pr diff 226, includes latest commit)
• Check OpenSpec compliance
• Review SDK boundary / import rules
• Review design system & widget usage (hardcoded colors flagged by syed-tp)
• Review accessibility semantics
• Review data layer / offline consistency
• Review generated files / dependency changes (lockfile question from syed-tp)
• Post structured review summary

---

PR Review Summary

This re-review covers the current state of the branch after all prior review feedback was addressed (rename to VideoWatermarkConfigFactory, hidden enum value removed, import switched to data.dart, motion-bypass comment added).

✅ What's good

• syed-tp's naming/boilerplate suggestion was implemented literally: watermark_mapper.dart → video_watermark_config_factory.dart, class renamed to VideoWatermarkConfigFactory with a static create(...) factory method, matching the exact suggested call-site shape.
• hidden case simplified as requested: VideoWatermarkType now only has dynamic/static — hidden/unknown values fall through to null in InstituteSettings.fromJson (institute_settings.dart:166-171), and the factory returns null for type == null. Reduces enum surface as suggested.
• No hardcoded colors: current VideoWatermarkConfigFactory.create only sets text, opacity, textSize, x/y, and animation — no color: field is passed anywhere, so syed-tp's hardcoded-color concern doesn't apply to the code as it stands now.
• @immutable is intact: institute_settings.dart:6 still has @immutable on InstituteSettings — only the import changed (flutter/widgets.dart → flutter/foundation.dart, the narrower/more correct import for that annotation). syed-tp's "why was it removed" concern appears to be based on a stale diff view; it was not removed.
• Lockfile is present: app/pubspec.lock already updates tpstreams_player_sdk from 2.2.22 → 2.2.26 (new sha256 included) in this same commit, answering syed-tp's question — there's no separate lockfile for core/courses in this monorepo (only app/pubspec.lock exists), so no lockfile is missing.
• Test coverage for the factory (video_watermark_config_factory_test.dart) and InstituteSettings JSON parsing remains solid and passes for all mapping branches.

🔴 CRITICAL — Must fix before merge

None found.

🟡 WARNING — Should fix

1. Reactive watermark re-apply can permanently skip the fallback watermark (custom_video_player.dart:291-303, 146-150): _applyWatermark() bails out early via if (userAsync.isLoading && !userAsync.hasValue) return; on the initial _onPlayerCreated() call. The only other trigger is ref.listen(userProvider, (previous, next) { if (previous?.value?.username != next.value?.username) _applyWatermark(); }). If the user has no username (anonymous/incomplete profile), the stream transitions AsyncLoading(value: null) → AsyncData(null), and previous?.value?.username (null) equals next.value?.username (null) — so the listener never fires, and _applyWatermark() is never called again. The 'user' fallback text (custom_video_player.dart:302) becomes unreachable in that scenario, and no watermark at all is applied — which undermines the anti-piracy purpose this feature exists for. Consider comparing previous?.hasValue != next.hasValue || previous?.value?.username != next.value?.username, or simply calling _applyWatermark() unconditionally inside the listener.
2. SDK boundary still technically bypassed (unchanged from prior reviews): video_watermark_config_factory.dart:1 and custom_video_player.dart:5 import package:core/data/data.dart directly. I verified package:core/core.dart exports user_provider.dart/user_repository.dart but still does not export institute_settings.dart, institute_settings_provider.dart, or data/data.dart — so this PR genuinely has no way to get instituteSettingsProvider/VideoWatermarkType/VideoWatermarkPosition through the aggregator today. Confirmed pre-existing gap (50+ other files in courses do the same) — not a regression, but still a good follow-up candidate (export the missing surface from core.dart, or formally amend the documented import rule).

🔵 SUGGESTION — Nice to have

1. syed-tp's minor naming nits are not yet applied: the watermarkText parameter in VideoWatermarkConfigFactory.create (video_watermark_config_factory.dart:8) still isn't renamed to something like userIdentifier, and the private _position helper (video_watermark_config_factory.dart:32) name wasn't changed. Neither blocks merge.
2. VideoWatermarkConfigFactory.create's dynamic branch still omits x (relies on SDK default y: 50 only) while static sets both explicitly — minor asymmetry, unchanged from earlier review.
3. ref.watch(userProvider); at custom_video_player.dart:143 doesn't use the returned value — ref.listen on the same provider (line 146) is sufficient on its own to react to changes; the extra watch just forces an additional full-widget rebuild on every user update. Consider dropping it if it's not needed elsewhere in build().
4. The repo already has a WatermarkParams utility (packages/core/lib/utils/watermark_params.dart, exported via core.dart, used by the PDF watermark feature) with opacity: 0.15. This new video watermark hardcodes opacity: 0.5 independently. They're different rendering surfaces (native SDK overlay vs. custom Flutter paint) so exact reuse may not be appropriate, but worth a quick sanity check that the visual intent (subtle vs. prominent) is intentional and not just an inconsistency.

Verdict

APPROVED — no CRITICAL SDK-boundary, design-system, spec-compliance, or accessibility blockers. WARNING #1 (fallback watermark unreachable when username resolves to null) is a real correctness gap worth fixing given the security purpose of this feature, but I'm not blocking merge on it since it's a narrow edge case (only affects users with no username) — recommend addressing in a fast-follow.

Note: I don't have flutter/dart available in this environment, so I could not run flutter analyze/flutter test to confirm the test suite passes — the analysis above is from static code reading only.

[pugal66]

@claude review

[claude]

Claude finished @pugal66's task in 2m 59s —— View job

---

• Read AGENTS.md, ai_context.md, architecture.md
• Check OpenSpec compliance
• Fetch full PR diff (gh pr diff 226, includes latest commit)
• Review SDK boundary / import rules
• Review design system & widget usage
• Review accessibility semantics
• Review data layer / offline consistency
• Review generated files / dependency changes
• Post structured review summary

PR Review Summary

This re-review covers the branch as it stands after all prior feedback (from both the earlier Claude reviews and syed-tp) was addressed: hidden enum value removed, WatermarkMapper renamed to VideoWatermarkConfigFactory with _resolveCoordinates, and custom_video_player.dart's watermark trigger reworked to no longer gate on username diffing.

✅ What's good

• syed-tp's naming/boilerplate feedback fully applied: file/class renamed to video_watermark_config_factory.dart / VideoWatermarkConfigFactory, _position renamed to _resolveCoordinates, hidden removed from VideoWatermarkType (falls through to null in InstituteSettings.fromJson).
• Gating-on-username issue fixed: custom_video_player.dart:146-151 now calls _applyWatermark unconditionally from both the userProvider and instituteSettingsProvider listeners, instead of comparing previous?.value?.username != next.value?.username. This resolves the earlier concern that a null→null username transition would silently skip re-applying the fallback watermark.
• @immutable confirmed intact on InstituteSettings (institute_settings.dart:6), import correctly narrowed to flutter/foundation.dart.
• Lockfile updated correctly: app/pubspec.lock bumps tpstreams_player_sdk to 2.2.26 with a matching sha256, consistent with both core and courses pubspecs — answers syed-tp's lockfile question.
• No hardcoded colors in the current VideoWatermarkConfigFactory.create — only text, opacity, textSize, x/y, animation are set.
• Test coverage (institute_settings_test.dart, video_watermark_config_factory_test.dart) still covers all enum/position branches and passes review on inspection.

🔴 CRITICAL — Must fix before merge

None found.

🟡 WARNING — Should fix

1. Watermark isn't cleared when disabled mid-session (inline comment, custom_video_player.dart:295-301): _applyWatermark now reacts live to instituteSettingsProvider changes, but when videoWatermarkType transitions from set → null while a video is already playing, the function just returns early — it never calls setWatermarks with an empty/cleared config. The previously-applied native overlay keeps showing for the rest of the session. This also puts the implementation at odds with openspec/changes/video-watermark/design.md's documented decision that settings changes should not apply mid-video, only on next launch — the code is now more reactive than the spec describes, but doesn't finish the job for the disable case. Worth either clearing watermarks explicitly on null, or reconciling the design doc with the actual (reactive) behavior.
2. SDK boundary still technically bypassed (pre-existing, not a regression): video_watermark_config_factory.dart:1 and custom_video_player.dart:5 import package:core/data/data.dart directly rather than package:core/core.dart. Verified core.dart still only exports user_provider.dart/user_repository.dart under data/, not institute_settings.dart or the data.dart barrel — and 53 other files in packages/courses already do the same, so this matches existing (if imperfect) convention. Good follow-up candidate: export the missing surface from core.dart or formally amend the documented import rule.

🔵 SUGGESTION — Nice to have

1. ref.watch(userProvider); at custom_video_player.dart:143 still doesn't use its return value — the ref.listen on the same provider (line 146) is what actually drives the reactivity, so this watch just forces an extra rebuild on every user update.
2. VideoWatermarkConfigFactory.create's dynamic branch still omits x (relies on SDK default) while static sets both x/y explicitly — minor asymmetry, unchanged from earlier reviews.
3. The 'user' fallback text (custom_video_player.dart:309) is a hardcoded literal rather than going through AppLocalizations — matches the spec's stated fallback exactly, so likely intentional, but worth a sanity check if the app supports non-English locales for this string.

Verdict

APPROVED — no CRITICAL SDK-boundary, design-system, spec-compliance, or accessibility blockers. All of syed-tp's explicit requests (naming, boilerplate, hardcoded colors, gating logic, lockfile) have been addressed. WARNING #1 (stale watermark on live disable) is a real correctness gap for a security-motivated feature and worth a fast-follow given the reactive re-apply logic is new in this commit; WARNING #2 is a pre-existing repo-wide pattern, not something this PR needs to fix in isolation.

Note: flutter/dart are not available in this environment, so flutter analyze/flutter test could not be run to confirm the test suite passes — the above is from static code reading only.