harvinders commented 5 years ago

Proposal: Previous and Next track buttons should be disabled in MediaTransportControls if at the start or end of a playlist.

Summary

It is currently possible to show or hide next and previous track buttons in MediaTransportControls, by setting IsNextTrackButtonVisible and IsPreviousTrackButtonVisible however, when at the start or end of the playlist these buttons are not disabled.

Rationale

This functionality should be out of box, as it is in SMTC. The SMTC disables these buttons and we should be consistent in MediaTransportControls.
There is no way to disable these buttons in MediaTransportControls.
In fact, we should extend the MediaPlaybackCommandManager functionality to MediaTransportControls, which would save duplicate efforts.

From Integrate with the System Media Transport Controls

The following example illustrates a scenario where the app wants to disable the Next command after the user has clicked through five items in the playlist, perhaps requiring some user interaction before continuing playing content. Each ## the NextReceived event is raised, a counter is incremented. Once the counter reaches the target number, the EnablingRule for the Next command is set to Never, which disables the command.
int _nextPressCount = 0;
private void CommandManager_NextReceived(MediaPlaybackCommandManager sender, MediaPlaybackCommandManagerNextReceivedEventArgs args)
{
_nextPressCount++;
if (_nextPressCount > 5)
{
sender.NextBehavior.EnablingRule = MediaCommandEnablingRule.Never;
// Perform app tasks while the Next button is disabled
}
}
The only other way to solve the problem is to create a derived custom control, which would create its own issues, i.e. difficult to maintain as this control evolves (style changes).

Scope

Capability	Priority
This proposal will allow developers to get a good default media playing experience with playlists	Must
This proposal will allow end developers to reduce duplicate efforts	Should
This proposal will allow developers to also display title and artist name, just as SMTC does	Could

Important Notes

Stackoverflow Question - How to disable next and previous buttons in MediaTransportControls (UWP)

Open Questions

shaheedmalik commented 5 years ago

I disagree with this. Those buttons are useful for going back to the beginning or end of the playlist.

harvinders commented 5 years ago

@shaheedmalik If the first items is getting played then there is no previous and hence previous button should be disabled. Similarly when last item is getting played there is no next and hence next should be disabled.

shaheedmalik commented 5 years ago

You do know people use previous to jump to the end of the list, right? I do this all the time with playlists.

harvinders commented 5 years ago

As of now if we are at the first item, pressing the previous button has no impact, same is the case with next when at the end.

What @shaheedmalik has suggested is interesting. I think it make sense to be consistent between SMTC and the MediaTransportControls. SMTC does not behave the way as per this new suggestion.

@mrlacey, @mdtauk, @chigy care to weigh in? (I understand we have to wait for WinUI 3.0 before anything can be done).

mrlacey commented 5 years ago

To my mind, disabling the "go to first" button when already at the first position (or the "go to end" button when already at the position) makes sense.

Having a button disabled when it will do nothing is almost always preferable to having the button be enabled but seemingly do nothing when pressed. This is a strong argument for the original proposal.

I've never known the option of being at the start of a playlist and pressing 'back' to get to the end. This may be something that some apps do but is not advertised and relies on other ways of discovering the functionality. I have known (and worked on) apps that use dynamically generated playlists and so skipping from the front "back" to the end isn't possible.

So, the original proposal makes sense but there is at least one valid scenario it would prevent. An alternative would be to make it possible to control the disabling of the buttons by adding [dependency]properties for IsNextTrackButtonEnabled and IsPreviousTrackButtonEnabled. This wouldn't make the desired behavior automatic but it would make it possible for the apps that want it.

shaheedmalik commented 5 years ago

To my mind, disabling the "go to first" button when already at the first position (or the "go to end" button when already at the position) makes sense.

Having a button disabled when it will do nothing is almost always preferable to having the button be enabled but seemingly do nothing when pressed. This is a strong argument for the original proposal.

I've never known the option of being at the start of a playlist and pressing 'back' to get to the end. This may be something that some apps do but is not advertised and relies on other ways of discovering the functionality. I have known (and worked on) apps that use dynamically generated playlists and so skipping from the front "back" to the end isn't possible.

The fact you never known a feature means you never used Groove. This has been standard behavior going all the way back to at least Xbox Music on Windows 8.1. https://imgur.com/a/pqmXkf2

Dynamically generated playlists were always disabled when there was nothing to skip to. It's a non-issue.

mrlacey commented 5 years ago

The fact you never known a feature means you never used Groove.

No. I have used Groove (and occasionally still do.) It means I never discovered that feature.

This has been standard behavior going all the way back to at least Xbox Music on Windows 8.1.

That may be the case, but it doesn't mean it's well known. I find this functionality to be very counter-intuitive. To me, it seems equivalent to saying "If I want to go forward to the very end, I go back from the start." That makes sense with something that loops but not all playlists, in all apps, do.

It can be very dangerous (but is also sadly very common) to assume how other people use software, or their knowledge of features, based on the way you use it and the knowledge you have. This is why it's so important to test assumptions and, where possible, gather feature usage metrics.

Dynamically generated playlists were always disabled when there was nothing to skip to. It's a non-issue.

It is an issue, because to know this, the control needs to know if the playlist is dynamically generated and able to be navigated in this way. While an app might be able to know this, the control can't based on the list of tracks potentially being an enumerable. At a minimum, the control would therefore need a new property to specify if the playlist can loop.

I think it would be easier to explain a new property that determines if the previous&next buttons are automatically disabled if at the start/end of the list than introducing some new kind of collection that can determine this or a separate property to indicate if navigation should loop.

I'm agreeing that the scenario/functionality you mentioned is something that should be supported (even though I didn't know it existed previously--I'm still learning.) This issue is about changing the default control that will be used in many apps in the future. What apps currently do can be useful background but what specific apps do or do not do and all the different scenarios they need to handle doesn't necessarily mean that's what an updated version of this control can or should do the same.

ryandemopoulos commented 5 years ago

@harvinders @shaheedmalik @mrlacey Good discussion, and based on the commentary, I'd propose refining the Scope to be the following:

Capability	Priority
Preserve the simplicity of providing a default Media Transport Controls experience associated with MediaPlayerElement	Must
Address the issue where our default Previous/Next buttons can be pressed at the start/end of a playlist, but they do nothing.	Must
Provide an easy mechanism to allow the Previous/Next buttons to wrap around the beginning/end of a playlist, if a developer desires this behavior	Should

Everyone: Does this look like a good list? Notice that I didn't prioritize the last one as highly as the first two items, on purpose. (feel like it's more of a "should" than an absolute must)

@harvinders: Missing from my newly suggested scoping table above is a reference to the title and artist name displaying as SMTC does. Could you provide a picture/screenshot of this? (I don't think there's been any discussion of this item, including in your original filing of this issue)

shaheedmalik commented 5 years ago

Address the issue where our default Previous/Next buttons can be pressed at the start/end of a playlist, but they do nothing. Must Provide an easy mechanism to allow the Previous/Next buttons to wrap around the beginning/end of a playlist, if a developer desires this behavior Should

All the music apps I have used have this already. Groove, when there is no playlist, pressing Previous, defaults to restarting the song from the beginning. Groove, when there isn't next song, goes to the top of the playlist. Groove, when there is a playlist, restarts the song, and if the button is pressed again in the first couple of seconds, goes to the bottom of the playlist.

Spotify, when there is no playlist, pressing Previous, defaults to restarting the song from the beginning. Groove, when there isn't next song, goes to the top of the playlist. Spotify, when there isn't next song goes to the top of the playlist.

This is expected behavior for music and transport apps. This shouldn't be taken away because someone who isn't familiar enough with the feature doesn't see the point of said feature.

harvinders commented 5 years ago

@ryandemopoulos

Use case 1 - Display title

The use case is to be able to display the Title and Artist name whilst the item is played. Currently the control does not provide a placeholder (something like header template equivalent) to add this information.

Well it is not so difficult to write something like

    <Grid Background="{ThemeResource SystemControlBackgroundBaseMediumLowBrush}">
        <Grid.RowDefinitions>
            <RowDefinition Height="Auto" />
            <RowDefinition Height="Auto" />
        </Grid.RowDefinitions>
        <Grid.ColumnDefinitions>
            <ColumnDefinition Width="*" />
            <ColumnDefinition Width="Auto" />
        </Grid.ColumnDefinitions>
        <TextBlock
            x:Name="Title"
            Grid.Column="0"
            Margin="{ThemeResource SmallMargin}"
            VerticalAlignment="Bottom"
            Foreground="{ThemeResource SystemControlForegroundBaseMediumHighBrush}"
            Style="{ThemeResource SubtitleTextBlockStyle}" />
        <TextBlock
            x:Name="Author"
            Grid.Column="1"
            Margin="{ThemeResource SmallMargin}"
            VerticalAlignment="Bottom"
            Foreground="{ThemeResource SystemControlForegroundBaseMediumBrush}"
            Style="{ThemeResource CaptionTextBlockStyle}" />
        <MediaPlayerElement
            x:Name="Player"
            Grid.Row="1"
            Grid.Column="0"
            Grid.ColumnSpan="2"
            VerticalAlignment="Bottom"
            AreTransportControlsEnabled="True"
            RelativePanel.AlignBottomWithPanel="True">
            <MediaPlayerElement.TransportControls>
                <MediaTransportControls
                    IsFullWindowButtonVisible="False"
                    IsNextTrackButtonVisible="True"
                    IsPreviousTrackButtonVisible="True"
                    IsZoomButtonVisible="False" />
            </MediaPlayerElement.TransportControls>
        </MediaPlayerElement>
    </Grid>

However, it seems that MediaTransportControls has background set to #99FFFFFF. If the whole Grid Background is set, as in the example above the result looks odd.

The correct way would be to set the Background of Row#1 also as #99FFFFFF, which is a magic number. Maybe in future it would be an acyrilic brush.

I believe, we can do better. One way could be to expose a HeaderTemplate for the control. But what if someone want's it on the right or left? Do we add HeaderLocation?

Use case 2 - PosterSource

Another use case is related to PosterSource (not mentioned in original post) and developer experience. As a developer, I have already created a playlist and set music properties including Thumbnail. These properties are only honored by the SMTC and not MediaTransportControls. I can understand why music title and artist names are not displayed (where would you put them, top?right? left?, maybe something like HeaderTemplate and HeaderLocation could solve this). But what is more surprising is that Thumbnail is only shown in SMTC and not MediaPlayerElement's PosterSource.

Developer experience, setting PosterSource One can argue that there is always an option to set PosterSource. However, this require maintaining a parallel playlist with information like poster image. Developers have to subscribe to the real playlist events and whenever the item change, refer to the duplicate playlist for the poster image. The maintenance, would start to become more involved, if we now need to add an item in existing playlist, developer have to add it in both. All I am asking is does it not make sense to use the poster info already present in the playlist to be used in PosterSource.

Thumbnail should support `UniformToFill`.

I noticed that when setting Thumbnail with an image of aspect ratio of not 1:1, the same aspect ratio is used in SMTC display, which does not look good. Maybe there should be an option of UniformToFill, like we have for Image control.

microsoft / microsoft-ui-xaml