-
Notifications
You must be signed in to change notification settings - Fork 484
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
Adds functionality to flet_video to know what song is playing #3772
Conversation
Reviewer's Guide by SourceryThis pull request introduces a new feature to the flet_video package that allows the application to know which song is currently playing. The changes include adding a new event handler for track changes in both the Dart and Python parts of the codebase. The _onTrackChanged method and onTrackChanged attribute were added to the Dart code, while the on_track_changed parameter and corresponding getter and setter were added to the Python code. These changes ensure that the current track index is available and can be handled appropriately. File-Level Changes
Tips
|
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 @syleishere - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
@@ -68,6 +68,12 @@ class _VideoControlState extends State<VideoControl> with FletStoreMixin { | |||
.triggerControlEvent(widget.control.id, "completed", message ?? ""); | |||
} | |||
|
|||
void _onTrackChanged(String? message) { |
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.
suggestion: Consider extracting a common method for event handling
The _onTrackChanged
method is very similar to _onCompleted
. Consider extracting a common method to handle these events, reducing code duplication and improving maintainability.
void _triggerControlEvent(String eventName, String? message) {
debugPrint("Video $eventName: $message");
widget.backend.triggerControlEvent(widget.control.id, eventName, message ?? "");
}
void _onTrackChanged(String? message) {
_triggerControlEvent("trackChanged", message);
}
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.
Done
Description
Adds event on_track_changed to know current song index that is currently playing.
Type of change
Checklist:
[x ] I signed the CLA.
[ x] My code follows the style guidelines of this project
[ x] I have performed a self-review of my own code
[ x] I have commented my code, particularly in hard-to-understand areas
[ x] My changes generate no new warnings
[ x] New and existing tests pass locally with my changes
I have made corresponding changes to the documentation (if applicable)
Additional details
With this new feature you will always know what track currently is playing. Returns an Int of what number in your playlist is currently playing.
I will submit a PR to update documentation for this feature on website.
Summary by Sourcery
Introduce the on_track_changed event to flet_video to track the current song index in the playlist and update the documentation accordingly.
New Features:
Documentation: