Skip to content

Conversation

@United600
Copy link
Collaborator

No description provided.

@United600 United600 requested a review from Copilot August 5, 2025 14:44
@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Aug 5, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the timing offset properties for audio and subtitle synchronization by moving them from custom implementation in the ViewModel to proper interface-based properties in the media player architecture.

  • Extracted timing offset properties from PlayerControlsViewModel to the IMediaPlayer interface
  • Replaced manual property handling with ObservableProperty attributes for automatic change notifications
  • Added proper event handling for timing offset changes in VlcMediaPlayer

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
PlayerControlsViewModel.cs Removed custom timing offset properties and replaced with ObservableProperty fields and change handlers
VlcMediaPlayer.cs Added proper AudioTimingOffset and SubtitleTimingOffset properties with event notifications
IMediaPlayer.cs Added timing offset properties and events to the interface definition

Comment on lines 145 to 151
public double SubtitleTimingOffset
{
get => VlcPlayer.SpuDelay / 1000.0;
set
{
double oldValue = VlcPlayer.SpuDelay / 1000.0;
if (Math.Abs(oldValue - value) > 0.0001)
Copy link

Copilot AI Aug 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The magic number 0.0001 should be extracted to a named constant to improve maintainability and make the tolerance value explicit.

Suggested change
public double SubtitleTimingOffset
{
get => VlcPlayer.SpuDelay / 1000.0;
set
{
double oldValue = VlcPlayer.SpuDelay / 1000.0;
if (Math.Abs(oldValue - value) > 0.0001)
private const double SubtitleTimingOffsetTolerance = 0.0001;
public double SubtitleTimingOffset
{
get => VlcPlayer.SpuDelay / 1000.0;
set
{
double oldValue = VlcPlayer.SpuDelay / 1000.0;
if (Math.Abs(oldValue - value) > SubtitleTimingOffsetTolerance)

Copilot uses AI. Check for mistakes.
set
{
double oldValue = VlcPlayer.AudioDelay / 1000.0;
if (Math.Abs(oldValue - value) > 0.0001)
Copy link

Copilot AI Aug 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The magic number 0.0001 should be extracted to a named constant to improve maintainability and make the tolerance value explicit. Consider using the same constant as in the SubtitleTimingOffset property.

Copilot uses AI. Check for mistakes.
@huynhsontung
Copy link
Owner

There is actually a reason I didn't want to include the timing offset properties in the IMediaPlayer interface. I want IMediaPlayer to track the UWP MediaPlayer class as much as possible. This way, we can be sure that any player backend we implement can follow the UWP standard. This is the assumption that enables FFmpeg integration. (#333)

@United600
Copy link
Collaborator Author

There is actually a reason I didn't want to include the timing offset properties in the IMediaPlayer interface. I want IMediaPlayer to track the UWP MediaPlayer class as much as possible. This way, we can be sure that any player backend we implement can follow the UWP standard. This is the assumption that enables FFmpeg integration. (#333)

Makes sense. MediaPlayerElement also I guess.

@United600 United600 marked this pull request as draft September 22, 2025 15:09
add helper methods for time delay conversions
@United600 United600 marked this pull request as ready for review November 3, 2025 19:05
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Nov 5, 2025
@huynhsontung huynhsontung merged commit 4a3260b into main Nov 9, 2025
4 checks passed
@United600 United600 deleted the United600/offset branch November 9, 2025 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants