implement audio and video basic catalog components#802
Draft
andrewkolos wants to merge 16 commits intoflutter:mainfrom
Draft
implement audio and video basic catalog components#802andrewkolos wants to merge 16 commits intoflutter:mainfrom
andrewkolos wants to merge 16 commits intoflutter:mainfrom
Conversation
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces significant new functionality by implementing audioPlayer and video components, providing actual media playback capabilities across various platforms. However, a critical security concern exists: the handling of URLs from GenUI messages lacks validation, potentially leading to Client-Side Request Forgery or unauthorized access to local files. Strict URL scheme validation and domain allow-listing are recommended. Additionally, general feedback addresses improving asynchronous operations and optimizing logging practices.
packages/genui/lib/src/catalog/basic_catalog_widgets/video.dart
Outdated
Show resolved
Hide resolved
packages/genui/lib/src/catalog/basic_catalog_widgets/video.dart
Outdated
Show resolved
Hide resolved
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.
Description
See prior discussion in proposal: https://docs.google.com/document/d/1cRPCuuGLmUYY-PGFXlf7mIhdOuM2rypfP1Cw3nj_myM/edit?usp=sharing
Closes #738. Closes #392. Closes #393. Closes #746.
Implements the
audioPlayerandvideocomponents in the basic catalog.audioPlayeris implemented simply, using a slider as the progress indicator/scrubbing control. The component also has your typical play/pause button, reading of the playhead position (e.g. 01:23), length of the audio file (e.g. 02:25), and a basic volume slider.videoPlayeris likewise simple. Not supported on Linux due to the lack of a lightweight, turnkey video rendering package available on pub; instead, a placeholder is shown to warn developers targeting Linux, who can write their own implementation.Dependencies used and their sizes
audioplayersis used for audio streaming support.video_playeris used for video. TL;DR these 0.5-2MB to the app bundle.I did some crude before-and-after testing to see how the bundle_size of an app could change with these two new dependencies introduced.
I used examples/catalog_gallery as the test app.
I used the following package versions:
Here is the data:
Other things to note
Introducing dependencies carries other burdens other than unnecessarily inflating bundle sizes for those that do not have interest in the audio and video components. At the time of writing, one can be seen just by building for macOS:
swift compiler warnings
If such drawbacks prove to be troublesome in the long term, we can move audio and video components into a separate package and deprecate the existing ones.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-devrel channel on Discord.