[PATCH 0/16] MR4046: windows.media.mediacontrol: Implement some IMusicDisplayProperties and IMusicDisplayProperties2 properties.

[Mohamad Al-Jaf (＠maljaf)]

Called by Roon.

[Rémi Bernon (＠rbernon)]

Rémi Bernon (@rbernon) commented about dlls/windows.media.mediacontrol/tests/mediacontrol.c:
>  hr = ISystemMediaTransportControls_get_DisplayUpdater( media_control_statics, &display_updater );
>  ok( hr == S_OK, "got hr %#lx.\n", hr );
>
> - hr = ISystemMediaTransportControlsDisplayUpdater_put_Type( display_updater, -1 );
> - todo_wine ok( hr == E_INVALIDARG, "got hr %#lx.\n", hr );
> - hr = ISystemMediaTransportControlsDisplayUpdater_put_Type( display_updater, 4 );
> - todo_wine ok( hr == E_INVALIDARG, "got hr %#lx.\n", hr );
> - hr = ISystemMediaTransportControlsDisplayUpdater_put_Type( display_updater, 1 );
> - todo_wine ok( hr == S_OK, "got hr %#lx.\n", hr );
> -
> - playback_type = -1;
> - hr = ISystemMediaTransportControlsDisplayUpdater_get_Type( display_updater, &playback_type );
> - todo_wine ok( hr == S_OK, "got hr %#lx.\n", hr );
> - todo_wine ok( playback_type == 1, "got playback_type %d.\n", playback_type );

Would be better to use MediaPlaybackType values here, except for the invalid ones.

[Rémi Bernon (＠rbernon)]

Rémi Bernon (@rbernon) commented about dlls/windows.media.mediacontrol/display.c:
> static HRESULT WINAPI music_properties_get_Title( IMusicDisplayProperties *iface, HSTRING *value )
>  {
> - FIXME( "iface %p, value %p stub!\n", iface, value );
> - return E_NOTIMPL;
> - struct music_properties *impl = impl_from_IMusicDisplayProperties( iface );
> - TRACE( "iface %p, value %p\n", iface, value );
> - return WindowsDuplicateString( impl->title, value );
>
> }
>
> static HRESULT WINAPI music_properties_put_Title( IMusicDisplayProperties *iface, HSTRING value )
>  {
> - FIXME( "iface %p, value %s stub!\n", iface, debugstr_hstring(value) );
> - return E_NOTIMPL;
> - struct music_properties *impl = impl_from_IMusicDisplayProperties( iface );
> - TRACE( "iface %p, value %p\n", iface, value );
> - return WindowsDuplicateString( value, &impl->title );

You need to release impl->title first if it was previously set.

[Rémi Bernon (＠rbernon)]

Rémi Bernon (@rbernon) commented about dlls/windows.media.mediacontrol/display.c:
> static HRESULT WINAPI music_properties_get_Artist( IMusicDisplayProperties *iface, HSTRING *value )
>  {
> - FIXME( "iface %p, value %p stub!\n", iface, value );
> - return E_NOTIMPL;
> - struct music_properties *impl = impl_from_IMusicDisplayProperties( iface );
> - TRACE( "iface %p, value %p\n", iface, value );
> - return WindowsDuplicateString( impl->artist, value );
>
> }
>
> static HRESULT WINAPI music_properties_put_Artist( IMusicDisplayProperties *iface, HSTRING value )
>  {
> - FIXME( "iface %p, value %s stub!\n", iface, debugstr_hstring(value) );
> - return E_NOTIMPL;
> - struct music_properties *impl = impl_from_IMusicDisplayProperties( iface );
> - TRACE( "iface %p, value %p\n", iface, value );
> - return WindowsDuplicateString( value, &impl->artist );

Same here, you need to call `WindowsDeleteString` first.

[Rémi Bernon (＠rbernon)]

Rémi Bernon (@rbernon) commented about dlls/windows.media.mediacontrol/display.c:
> static HRESULT STDMETHODCALLTYPE music_properties2_get_AlbumTitle( IMusicDisplayProperties2 *iface, HSTRING *value )
>  {
> - FIXME( "iface %p, value %p stub\n", iface, value );
> - return E_NOTIMPL;
> - struct music_properties *impl = impl_from_IMusicDisplayProperties2( iface );
> - TRACE( "iface %p, value %p\n", iface, value );
> - return WindowsDuplicateString( impl->album_title, value );
>
> }
>
> static HRESULT STDMETHODCALLTYPE music_properties2_put_AlbumTitle( IMusicDisplayProperties2 *iface, HSTRING value )
>  {
> - FIXME( "iface %p, value %s stub\n", iface, debugstr_hstring( value ) );
> - return E_NOTIMPL;
> - struct music_properties *impl = impl_from_IMusicDisplayProperties2( iface );
> - TRACE( "iface %p, value %p\n", iface, value );
> - return WindowsDuplicateString( value, &impl->album_title );

Ditto.

[Rémi Bernon (＠rbernon)]

Rémi Bernon (@rbernon) commented about dlls/windows.media.mediacontrol/tests/mediacontrol.c:
>  ok( ref == 1, "got ref %ld.\n", ref );
>  ref = ISystemMediaTransportControlsDisplayUpdater_Release( display_updater );
>  ok( ref == 1, "got ref %ld.\n", ref );
> - ref = ISystemMediaTransportControls_Release( media_control_statics );
> - ok( ref == 1 || broken(ref == 3) /* Win10 1507 */ || broken(ref == 2) /* Win10 1607 */, "got ref %ld.\n", ref );
> - ISystemMediaTransportControls_Release( media_control_statics );

Maybe that could be first in the MR, so that it gets fixed as soon as possible.

[Rémi Bernon (＠rbernon)]

Rémi Bernon (@rbernon) commented about dlls/windows.media.mediacontrol/display.c:
> - display_updater_Update,
>
> +};
> -
>
> +HRESULT media_control_create_display_updater( ISystemMediaTransportControls *iface, ISystemMediaTransportControlsDisplayUpdater **value )
> +{
> - struct display_updater *impl;
> -
> - if (!(impl = calloc( 1, sizeof(*impl) ))) return E_OUTOFMEMORY;
> -
> - impl->ISystemMediaTransportControlsDisplayUpdater_iface.lpVtbl = &display_updater_vtbl;
> - impl->ref = 2;
> -
> - *value = &impl->ISystemMediaTransportControlsDisplayUpdater_iface;
> - TRACE( "created ISystemMediaTransportControlsDisplayUpdater %p.\n", *value );
> - return S_OK;
>
> +}

I'm not sure it deserves a dedicated file, as it seems to be pretty much related to the `SystemMediaTransportControls` class, and the constructor could simply be inlined into `ISystemMediaTransportControls_get_DisplayUpdater` the same way you later do for `music_properties` and other subclases.

If you still prefer to keep it separate, I would name the file `display_updater.c`, and the constructor `display_updater_create`, and the iface parameter seems to be unused and should be removed.