-
Notifications
You must be signed in to change notification settings - Fork 120
Backports/1.4/hide secondary radcam stream #3747
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Backports/1.4/hide secondary radcam stream #3747
Conversation
…y stream from radcam
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds conditional visibility logic in VideoManager to hide the RadCam secondary stream unless it has an active stream or the user is in Pirate Mode, mirroring the existing behavior for Fake source devices. Flow diagram for VideoManager device visibility logicflowchart TD
A[Start device visibility check] --> B{Is device name Fake_source}
B -->|Yes| C{Has_active_stream OR is_pirate_mode}
C -->|True| Z1[Show device]
C -->|False| Z2[Hide device]
B -->|No| D{Is device name UnderwaterCam_IPCamera_UnderwaterCam}
D -->|Yes| E{Does device_source_end_with_stream_1}
E -->|Yes| F{Has_active_stream OR is_pirate_mode}
F -->|True| Z1
F -->|False| Z2
E -->|No| Z1
D -->|No| Z1
Z1 --> G[End visibility check]
Z2 --> G
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've found 1 issue, and left some high level feedback:
- The RadCam condition hardcodes both
device.nameand the/stream_1suffix; consider moving these identifiers into a configuration or constants module so they can be maintained without changing component logic. - The visibility logic for Fake source and RadCam secondary streams is now duplicated; you could extract a shared helper (e.g.
shouldShowDevice(device, settings)) to centralize thehas_active_stream || is_pirate_moderule and make future additions easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The RadCam condition hardcodes both `device.name` and the `/stream_1` suffix; consider moving these identifiers into a configuration or constants module so they can be maintained without changing component logic.
- The visibility logic for Fake source and RadCam secondary streams is now duplicated; you could extract a shared helper (e.g. `shouldShowDevice(device, settings)`) to centralize the `has_active_stream || is_pirate_mode` rule and make future additions easier.
## Individual Comments
### Comment 1
<location> `core/frontend/src/components/video-manager/VideoManager.vue:140-137` </location>
<code_context>
return has_active_stream(device) || settings.is_pirate_mode
}
+
+ // Do not show RadCam's secondary stream
+ if (device.name === 'UnderwaterCam - IPCamera (UnderwaterCam)' && device.source.endsWith('/stream_1')) {
+ return has_active_stream(device) || settings.is_pirate_mode
+ }
return true
</code_context>
<issue_to_address>
**issue (bug_risk):** The condition and return value seem to contradict the comment about not showing RadCam's secondary stream.
This returns `has_active_stream(device) || settings.is_pirate_mode`, matching the "Fake source" behavior and still allowing the secondary stream to be shown when active or in pirate mode. If the goal is to always hide this secondary stream, this should return `false` (or similar) instead of delegating to `has_active_stream`. Otherwise, please adjust the comment to describe the intended nuanced behavior so it doesn’t mislead future readers.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -136,6 +136,11 @@ export default Vue.extend({ | |||
| if (device.name === 'Fake source') { | |||
| return has_active_stream(device) || settings.is_pirate_mode | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (bug_risk): The condition and return value seem to contradict the comment about not showing RadCam's secondary stream.
This returns has_active_stream(device) || settings.is_pirate_mode, matching the "Fake source" behavior and still allowing the secondary stream to be shown when active or in pirate mode. If the goal is to always hide this secondary stream, this should return false (or similar) instead of delegating to has_active_stream. Otherwise, please adjust the comment to describe the intended nuanced behavior so it doesn’t mislead future readers.
This is a backport of #3748 into 1.4
Following the same behavior as for Fake Sources: we only show when there's a configured stream or the user is in Pirate Mode.
Closes #3746
Summary by Sourcery
Bug Fixes: