[Backport] Fix thumbnails on vehicle setup page#3854
Open
joaoantoniocardoso wants to merge 2 commits intobluerobotics:1.4from
Open
[Backport] Fix thumbnails on vehicle setup page#3854joaoantoniocardoso wants to merge 2 commits intobluerobotics:1.4from
joaoantoniocardoso wants to merge 2 commits intobluerobotics:1.4from
Conversation
Automatically starts continuous thumbnail fetching on mount when both autoplay and register are true. Used in the vehicle setup page tooltip so thumbnails appear on hover without manual interaction.
…with streams Filter out video devices that have no streams configured from the vehicle setup overview, so duplicate sources from the same camera don't appear.
Reviewer's guide (collapsed on small PRs)Reviewer's GuideBackports a fix to only register and update video thumbnails for devices with valid streams and supported encoders, and adds optional autoplay behavior for registered video thumbnails on the vehicle setup page. Sequence diagram for video thumbnail registration and autoplaysequenceDiagram
actor VehicleSetupUser
participant VideoOverview
participant VideoThumbnail
participant VideoService as Video
VehicleSetupUser->>VideoOverview: Open vehicle setup page
VideoOverview->>VideoOverview: Compute valid_devices
VideoOverview->>VideoOverview: Filter by has_supported_encode(device)
VideoOverview->>VideoOverview: Filter by available_streams_from_device not empty
VideoOverview->>VideoThumbnail: Create with source, width, register, autoplay
activate VideoThumbnail
VideoThumbnail->>VideoThumbnail: mounted()
alt autoplay and register are true
VideoThumbnail->>VideoThumbnail: continuous_mode = true
VideoThumbnail->>VideoService: startGetThumbnailForDevice(source)
else autoplay is false or register is false
VideoThumbnail->>VideoThumbnail: Wait for manual updates only
end
deactivate VideoThumbnail
Class diagram for updated VideoThumbnail and VideoOverview componentsclassDiagram
class VideoOverview {
+devices
+video
+streams_for_device
+computed valid_devices
}
class VideoThumbnail {
+String source
+String width
+Boolean register
+Boolean disabled
+Boolean autoplay
+Boolean continuous_mode
+Function updateThumbnail()
+mounted()
+beforeDestroy()
}
class VideoService {
+startGetThumbnailForDevice(source)
}
VideoOverview --> VideoThumbnail : uses
VideoThumbnail --> VideoService : calls
%% Template relationships
VideoOverview : passes source
VideoOverview : passes width 280
VideoOverview : passes register streams_for_device[device.name] !== undefined
VideoOverview : passes autoplay true
%% Lifecycle behavior
VideoThumbnail : mounted()
VideoThumbnail : if autoplay && register then
VideoThumbnail : continuous_mode = true
VideoThumbnail : video.startGetThumbnailForDevice(source)
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.
Hey - I've left some high level feedback:
- When
autoplayis enabled,mountedstartsvideo.startGetThumbnailForDevice(this.source)butbeforeDestroyonly clears timers; consider explicitly stopping the continuous thumbnail stream (if a corresponding stop API exists) to avoid background work or leaks when the component is destroyed. - In
VideoOverview.vue,registeris now driven bystreams_for_device[device.name] !== undefinedwhile the device list is filtered withavailable_streams_from_device(video.available_streams, device); it may be worth ensuring these two data sources stay consistent so that a device cannot pass the filter but still haveregisterresolve to false.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When `autoplay` is enabled, `mounted` starts `video.startGetThumbnailForDevice(this.source)` but `beforeDestroy` only clears timers; consider explicitly stopping the continuous thumbnail stream (if a corresponding stop API exists) to avoid background work or leaks when the component is destroyed.
- In `VideoOverview.vue`, `register` is now driven by `streams_for_device[device.name] !== undefined` while the device list is filtered with `available_streams_from_device(video.available_streams, device)`; it may be worth ensuring these two data sources stay consistent so that a device cannot pass the filter but still have `register` resolve to false.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backport of #3853 into 1.4
Summary by Sourcery
Enable automatic video thumbnail updates only for devices with available streams and supported encoders.
New Features:
Bug Fixes:
Summary by Sourcery
Ensure video thumbnails on the vehicle setup page only register and autoplay for devices with valid streams and supported encoders.
New Features:
Bug Fixes:
Enhancements: