-
Notifications
You must be signed in to change notification settings - Fork 67
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
feat: YouTube player respects timestamps startAt #1620
base: develop
Are you sure you want to change the base?
Conversation
- Implement timestamp extraction for YouTube video URLs - Automatically set video start time from URL parameters - Support multiple timestamp formats (t=, start=, time notation) - Add custom method to parse timestamps in seconds
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 thanks so much for taking on this issue and fixing it! I just left a few minor comments.
} | ||
|
||
int _convertTimeComponentsToSeconds(RegExpMatch match) { | ||
int hours = match.group(1) != null ? int.parse(match.group(1)!.replaceAll('h', '')) : 0; |
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.
You might be able to use RegEx groups so that one of the matches exactly matches the number without the unit (h
, s
, etc.).
int minutes = match.group(2) != null ? int.parse(match.group(2)!.replaceAll('m', '')) : 0; | ||
int seconds = match.group(3) != null ? int.parse(match.group(3)!.replaceAll('s', '')) : 0; | ||
|
||
return hours * 3600 + minutes * 60 + seconds; |
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.
You could do this as Duration(hours: hours, minutes: minutes, seconds: seconds).inSeconds
to avoid "magic numbers".
} | ||
|
||
int _convertTimeStringToSeconds(String time) { | ||
final regex = RegExp(r'(\d+h)?(\d+m)?(\d+s)?'); |
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.
Can you define this RegEx just once and reuse it?
@@ -334,6 +334,14 @@ packages: | |||
url: "https://pub.dev" | |||
source: hosted | |||
version: "0.17.3" | |||
cupertino_icons: |
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.
Ideally, all the pubspec.lock
changes should be reverted unless there was an explicit update to the packages!
Pull Request Description
Issue Being Fixed
Issue Number: #1619
Screenshots / Recordings
thunder_yt_player_start_at.mp4
Checklist
semanticLabel
s where applicable for accessibility?