Skip to content

fix: forward frame stepping position sync#795

Open
United600 wants to merge 2 commits intomainfrom
United600/step-forward-time
Open

fix: forward frame stepping position sync#795
United600 wants to merge 2 commits intomainfrom
United600/step-forward-time

Conversation

@United600
Copy link
Collaborator

StepForwardOneFrame now checks for position changes after advancing a frame and raises the PositionChanged event if the time has changed. Close #787.

@United600 United600 requested a review from Copilot March 4, 2026 14:00
@dosubot dosubot bot added size:XS This PR changes 0-9 lines, ignoring generated files. bug Something isn't working labels Mar 4, 2026
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

Fixes forward frame-stepping position desync by synchronizing _position and raising PositionChanged when stepping forward changes playback time.

Changes:

  • After NextFrame(), reads VlcPlayer.Time and converts it to a TimeSpan
  • Updates _position when changed and raises PositionChanged with old/new values

Comment on lines +521 to +523
var oldValue = _position;
_position = newValue;
PositionChanged?.Invoke(this, new ValueChangedEventArgs<TimeSpan>(newValue, oldValue));
Copy link

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

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

This introduces an inline _position update + PositionChanged raise pattern. If this class already has a single place that mutates _position (e.g., a Position setter, SetPosition(...), or a helper that raises PositionChanged), consider routing this through that shared path to keep event semantics consistent and prevent future drift between different position-update call sites.

Suggested change
var oldValue = _position;
_position = newValue;
PositionChanged?.Invoke(this, new ValueChangedEventArgs<TimeSpan>(newValue, oldValue));
Position = newValue;

Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Mar 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working lgtm This PR has been approved by a maintainer size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Going frame by frame starting from the currently displayed frame

3 participants