Skip to content
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

Add initial media options class and extension #1905

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

bijington
Copy link
Contributor

Description of Change

The aim of this PR is to highlight how we can initially make MediaElement more configurable but we should consider how this could enable developers to configure any of our internal defaults classes.

Linked Issues

  • Fixes #

PR Checklist

  • Has a linked Issue, and the Issue has been approved(bug) or Championed (feature/proposal)
  • Has tests (if omitted, state reason in description)
  • Has samples (if omitted, state reason in description)
  • Rebased on top of main at time of PR
  • Changes adhere to coding standard
  • Documentation created or updated: https://github.com/MicrosoftDocs/CommunityToolkit/pulls

Additional information

@bijington
Copy link
Contributor Author

@ne0rrmatrix this was my initial thinking, I have noticed that we don't currently set these values when controlling playback within our codebase but I believe your PR #1782 does.

@VladislavAntonyuk what are your thoughts on expanding this out of a MediaOptions class to enable support for also controlling the SpeechToText AVAudioSession values?

@everyone what do you think to expanding our options classes to allow for configuring all defaults within the toolkit? We already have classes that represent the defaults e.g. TouchBehaviorDefaults if we make this configurable it could make the usage of the controls simpler.

Copy link
Member

@pictos pictos left a comment

Choose a reason for hiding this comment

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

I really like the idea of extending our features using these Options objects. I believe it will help devs to customize the behavior of the feature easily.


namespace CommunityToolkit.Maui;

partial class MediaOptions
Copy link
Member

Choose a reason for hiding this comment

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

I would say those Options should be immutable. So we can refactor it to be a record and remove the setters

@@ -19,6 +19,8 @@ namespace CommunityToolkit.Maui.Core.Views;
/// </summary>
public partial class MediaManager
{
internal static MediaOptions DefaultMediaOptions { get; set; } = new();
Copy link
Member

Choose a reason for hiding this comment

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

Here's is fine to be mutable, since users may want to change how options based on the MediaSource. But, it does make sense on the current implementation? The MediaOptions is only configurable on AppBuilder, so it shouldn't change after app startup, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed. Let's make these { get; private set;} for now to follow the same architecture in CommunityToolkit.Maui.Options:

internal static bool ShouldSuppressExceptionsInAnimations { get; private set; }
internal static bool ShouldSuppressExceptionsInConverters { get; private set; }
internal static bool ShouldSuppressExceptionsInBehaviors { get; private set; }
internal static bool ShouldEnableSnackbarOnWindows { get; private set; }

We can always make it a public set; in the future without breaking changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to add bindable properties so we can use this at runtime for changes with MediaSource ?

@dotnet-policy-service dotnet-policy-service bot added stale The author has not responded in over 30 days help wanted This proposal has been approved and is ready to be implemented labels Jun 28, 2024

Choose a reason for hiding this comment

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

Copilot reviewed 4 out of 4 changed files in this pull request and generated no suggestions.

Comments skipped due to low confidence (3)

src/CommunityToolkit.Maui.MediaElement/Options/MediaOptions.macios.cs:10

  • The behavior of the 'Category' property in 'MediaOptions' is not covered by tests. Please add tests to ensure its correct behavior.
public AVAudioSessionCategory Category { get; set; } = AVAudioSessionCategory.Record;

src/CommunityToolkit.Maui.MediaElement/Options/MediaOptions.macios.cs:15

  • The behavior of the 'Mode' property in 'MediaOptions' is not covered by tests. Please add tests to ensure its correct behavior.
public AVAudioSessionMode Mode { get; set; } = default;

src/CommunityToolkit.Maui.MediaElement/Options/MediaOptions.macios.cs:20

  • The behavior of the 'CategoryOptions' property in 'MediaOptions' is not covered by tests. Please add tests to ensure its correct behavior.
public AVAudioSessionCategoryOptions CategoryOptions { get; set; } = default;
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted This proposal has been approved and is ready to be implemented stale The author has not responded in over 30 days
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants