Skip to content

fix: Peek.UI persists as a media player after closed & redundant SMTC…#46899

Open
zheng-fan wants to merge 1 commit intomicrosoft:mainfrom
zheng-fan:fix-Peek-media-control
Open

fix: Peek.UI persists as a media player after closed & redundant SMTC…#46899
zheng-fan wants to merge 1 commit intomicrosoft:mainfrom
zheng-fan:fix-Peek-media-control

Conversation

@zheng-fan
Copy link
Copy Markdown

… control for audio preview

Summary of the Pull Request

[Peek] Fix #26755 and remove redundant SMTC control for audio preview.

  1. If it is not a media file, the Hardware Media Key UI will not be displayed.
  2. When navigating between files using the arrow keys, only media files display the Hardware Media Key UI.
  3. After the Peek window is closed, the Hardware Media Key UI disappears.

PR Checklist

  • Communication: I've discussed this with core contributors already. If the work hasn't been agreed, this work might be rejected
  • Tests: Added/updated and all pass
  • Localization: All end-user-facing strings can be localized
  • Dev docs: Added/updated
  • New binaries: Added on the required places
  • Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Manually verified: When peeking at media and non-media files, navigating between different types of files using the arrow keys, and closing the Peek window, the Hardware Media Key UI all works as expected.

@zheng-fan
Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

@github-actions

This comment has been minimized.

Copy link
Copy Markdown
Collaborator

@daverayment daverayment left a comment

Choose a reason for hiding this comment

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

The FilePreview.xaml.cs changes don't take into account the error fallback case in Previewer_PropertyChanged(). If the last active previewer was an IVideoPreviewer that failed to load, the media transport controls would stay active after the fallback to the default previewer.

A cleaner and more concise fix would be to add an OnPreviewerChanged() partial method (suggested after the existing OnPreviewerChanging()):

    partial void OnPreviewerChanged(IPreviewer? value)
    {
        // Ensure the media transport controls are only present when viewing video media.
        VideoPreview.MediaPlayer.CommandManager.IsEnabled = value is IVideoPreviewer;
    }

This fires for every Previewer = ... assignment - the null path, the normal load path, and the error fallback in Previewer_PropertyChanged(). The IsEnabled lines in OnItemPropertyChanged() can be removed as they're redundant. The longer comment can go, too - the one-liner above is enough to explain why we're controlling IsEnabled here.

(Sorry if this comes across as "please remove all your code and replace it with the above"! Your idea was absolutely correct, it was just that error case that was the problem.)

You were right to make explicit the AudioControl's media transport control behaviour. Great catch!

@zheng-fan zheng-fan force-pushed the fix-Peek-media-control branch from ab8344b to 4e681b3 Compare April 12, 2026 03:38
@zheng-fan
Copy link
Copy Markdown
Author

Thanks for pointing this out! I've revised the code using the method you suggested using OnPreviewerChanged.

Earlier, I added a lot of comments here mainly because I was worried that subsequent developers might be confused: since audio and image files can also be classified as media , why aren't they integrated with SMTC? I'm new to this project, so I'm more than happy to follow your advice!

@zheng-fan zheng-fan requested a review from daverayment April 12, 2026 06:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Peek] Peek.UI persists as a media player after closed

3 participants