-
Notifications
You must be signed in to change notification settings - Fork 405
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
base: main
Are you sure you want to change the base?
Conversation
@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 @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. |
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.
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 |
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.
I would say those Options
should be immutable. So we can refactor it to be a record and remove the setter
s
@@ -19,6 +19,8 @@ namespace CommunityToolkit.Maui.Core.Views; | |||
/// </summary> | |||
public partial class MediaManager | |||
{ | |||
internal static MediaOptions DefaultMediaOptions { get; set; } = new(); |
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.
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?
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.
Agreed. Let's make these { get; private set;}
for now to follow the same architecture in CommunityToolkit.Maui.Options
:
Maui/src/CommunityToolkit.Maui/Options.cs
Lines 19 to 22 in d6d6e18
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.
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.
Do we want to add bindable properties so we can use this at runtime for changes with MediaSource
?
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.
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;
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
PR Checklist
approved
(bug) orChampioned
(feature/proposal)main
at time of PRAdditional information