<p dir="ltr">Hey,</p>
<p dir="ltr">Le 15 nov. 2016 03:13, Pierre Ynard via vlc-devel <vlc-devel@videolan.org> a écrit :<br>
><br>
> I'm not sure why it was done that way in the first place,</p>
<p dir="ltr">The playlist code really should not have to pass the long name. It shokld be inferred from the instantiated SD.</p>
<p dir="ltr">This ugly hack is a compromise to avoid exposing the deficiency to the playlist code.</p>
<p dir="ltr">> but passing<br>
> the longname from the probe into the config chain so it gets passed back<br>
> along to the core and parsed back there, sounds like a very convulated<br>
> way of essentially passing two separate arguments.</p>
<p dir="ltr">Convoluted, yes and ugly too. Two arguments, not really: the second argument really should not exist at all.</p>
<p dir="ltr">> It introduces many<br>
> complications and just doesn't work correctly when loading SD from the<br>
> -S config option or the CLI.</p>
<p dir="ltr">Indeed.</p>
<p dir="ltr">> Interfaces have the longname from probing<br>
> and can pass it correctly with this, so this is the first step to get<br>
> rid of this backwards mechanism.</p>
<p dir="ltr">I think this is replacing a backward approach with another backward approach though. In my opinion, the long name should be passed from the instantiated module.</p>
<p dir="ltr">Perhaps the SD plahlist node creation could be deferred to the first invocation of the item_added callback? I don't really see any other way, except for letting interfaces take care (privately and independantly) of SD, which would break -S however.</p>
<p dir="ltr">Also the Is*Loaded test is busted and IMHO unfixable. Interfaces need to stop using it.</p>
<p dir="ltr">><br>
> diff --git a/include/vlc_playlist.h b/include/vlc_playlist.h<br>
> index 8ed337c..c2ee5bd 100644<br>
> --- a/include/vlc_playlist.h<br>
> +++ b/include/vlc_playlist.h<br>
> @@ -324,7 +324,7 @@ VLC_API int playlist_Import( playlist_t *p_playlist, const char *psz_file );<br>
> /********************** Services discovery ***********************/<br>
><br>
> /** Add a service discovery module */<br>
> -VLC_API int playlist_ServicesDiscoveryAdd(playlist_t *, const char *);<br>
> +VLC_API int playlist_ServicesDiscoveryAdd(playlist_t *, const char *, const char *);<br>
> /** Remove a services discovery module by name */<br>
> VLC_API int playlist_ServicesDiscoveryRemove(playlist_t *, const char *);<br>
> /** Check whether a given SD is loaded */<br>
> diff --git a/modules/gui/macosx/VLCMainWindow.m b/modules/gui/macosx/VLCMainWindow.m<br>
> index ceebbe7..d8bfd3a 100644<br>
> --- a/modules/gui/macosx/VLCMainWindow.m<br>
> +++ b/modules/gui/macosx/VLCMainWindow.m<br>
> @@ -1110,7 +1110,7 @@ static const float f_min_window_height = 307.;<br>
>          BOOL sd_loaded = playlist_IsServicesDiscoveryLoaded(p_playlist, [identifier UTF8String]);<br>
><br>
>          if (!sd_loaded)<br>
> -            playlist_ServicesDiscoveryAdd(p_playlist, [identifier UTF8String]);<br>
> +            playlist_ServicesDiscoveryAdd(p_playlist, [identifier UTF8String], NULL);<br>
>          else<br>
>              playlist_ServicesDiscoveryRemove(p_playlist, [identifier UTF8String]);<br>
>      }<br>
> @@ -1138,7 +1138,7 @@ static const float f_min_window_height = 307.;<br>
>      if ([item sdtype] > -1 && [[item identifier] length] > 0) {<br>
>          BOOL sd_loaded = playlist_IsServicesDiscoveryLoaded(p_playlist, [[item identifier] UTF8String]);<br>
>          if (!sd_loaded)<br>
> -            playlist_ServicesDiscoveryAdd(p_playlist, [[item identifier] UTF8String]);<br>
> +            playlist_ServicesDiscoveryAdd(p_playlist, [[item identifier] UTF8String], NULL);<br>
>      }<br>
><br>
>      [_categoryLabel setStringValue:[item title]];<br>
> diff --git a/modules/gui/qt/components/playlist/selector.cpp b/modules/gui/qt/components/playlist/selector.cpp<br>
> index 2761903..5c09da8 100644<br>
> --- a/modules/gui/qt/components/playlist/selector.cpp<br>
> +++ b/modules/gui/qt/components/playlist/selector.cpp<br>
> @@ -365,7 +365,7 @@ void PLSelector::setSource( QTreeWidgetItem *item )<br>
>          sd_loaded = playlist_IsServicesDiscoveryLoaded( THEPL, qtu( qs ) );<br>
>          if( !sd_loaded )<br>
>          {<br>
> -            if ( playlist_ServicesDiscoveryAdd( THEPL, qtu( qs ) ) != VLC_SUCCESS )<br>
> +            if ( playlist_ServicesDiscoveryAdd( THEPL, qtu( qs ), NULL ) != VLC_SUCCESS )<br>
>                  return ;<br>
><br>
>              services_discovery_descriptor_t *p_test = new services_discovery_descriptor_t;<br>
> diff --git a/modules/gui/qt/dialogs_provider.cpp b/modules/gui/qt/dialogs_provider.cpp<br>
> index 5951b65..55e91a2 100644<br>
> --- a/modules/gui/qt/dialogs_provider.cpp<br>
> +++ b/modules/gui/qt/dialogs_provider.cpp<br>
> @@ -825,7 +825,7 @@ void DialogsProvider::menuUpdateAction( QObject *data )<br>
> void DialogsProvider::SDMenuAction( const QString& data )<br>
> {<br>
>      if( !playlist_IsServicesDiscoveryLoaded( THEPL, qtu( data ) ) )<br>
> -        playlist_ServicesDiscoveryAdd( THEPL, qtu( data ) );<br>
> +        playlist_ServicesDiscoveryAdd( THEPL, qtu( data ), NULL );<br>
>      else<br>
>          playlist_ServicesDiscoveryRemove( THEPL, qtu( data ) );<br>
> }<br>
> diff --git a/modules/lua/libs/sd.c b/modules/lua/libs/sd.c<br>
> index 65c36c6..777fffe 100644<br>
> --- a/modules/lua/libs/sd.c<br>
> +++ b/modules/lua/libs/sd.c<br>
> @@ -152,7 +152,7 @@ static int vlclua_sd_add( lua_State *L )<br>
> {<br>
>      const char *psz_sd = luaL_checkstring( L, 1 );<br>
>      playlist_t *p_playlist = vlclua_get_playlist_internal( L );<br>
> -    int i_ret = playlist_ServicesDiscoveryAdd( p_playlist, psz_sd );<br>
> +    int i_ret = playlist_ServicesDiscoveryAdd( p_playlist, psz_sd, NULL );<br>
>      return vlclua_push_ret( L, i_ret );<br>
> }<br>
><br>
> diff --git a/src/playlist/engine.c b/src/playlist/engine.c<br>
> index 1f29ee4..41443f5 100644<br>
> --- a/src/playlist/engine.c<br>
> +++ b/src/playlist/engine.c<br>
> @@ -289,7 +289,7 @@ playlist_t *playlist_Create( vlc_object_t *p_parent )<br>
>      {<br>
>          char *s = mods, *m;<br>
>          while( (m = strsep( &s, " :," )) != NULL )<br>
> -            playlist_ServicesDiscoveryAdd( p_playlist, m );<br>
> +            playlist_ServicesDiscoveryAdd( p_playlist, m, NULL );<br>
>          free( mods );<br>
>      }<br>
><br>
> diff --git a/src/playlist/services_discovery.c b/src/playlist/services_discovery.c<br>
> index 0efdabe..3d9ab81 100644<br>
> --- a/src/playlist/services_discovery.c<br>
> +++ b/src/playlist/services_discovery.c<br>
> @@ -222,27 +222,33 @@ static void playlist_sd_item_removed(services_discovery_t *sd,<br>
>      PL_UNLOCK;<br>
> }<br>
><br>
> -int playlist_ServicesDiscoveryAdd(playlist_t *playlist, const char *chain)<br>
> +int playlist_ServicesDiscoveryAdd(playlist_t *playlist, const char *chain,<br>
> +                                  const char *longname)<br>
> {<br>
>      vlc_sd_internal_t *sds = malloc(sizeof (*sds) + strlen(chain) + 1);<br>
>      if (unlikely(sds == NULL))<br>
>          return VLC_ENOMEM;<br>
><br>
> -    /* Look for configuration chain "longname" */<br>
> -    const char *longname = "?";<br>
> -    config_chain_t *cfg;<br>
> -    char *name;<br>
> -<br>
> -    free(config_ChainCreate(&name, &cfg, chain));<br>
> -    msg_Dbg(playlist, "adding services_discovery %s...", name);<br>
> +    config_chain_t *cfg = NULL;<br>
> +    if (longname == NULL)<br>
> +    {<br>
> +        longname = "?";<br>
> +<br>
> +        /* Look for configuration chain "longname" */<br>
> +        char *name;<br>
> +        free(config_ChainCreate(&name, &cfg, chain));<br>
> +        free(name);<br>
> +<br>
> +        for (config_chain_t *p = cfg; p != NULL; p = p->p_next)<br>
> +            if (p->psz_name != NULL && !strcmp(p->psz_name, "longname"))<br>
> +            {<br>
> +                if (p->psz_value != NULL)<br>
> +                    longname = p->psz_value;<br>
> +                break;<br>
> +            }<br>
> +    }<br>
><br>
> -    for (config_chain_t *p = cfg; p != NULL; p = p->p_next)<br>
> -        if (p->psz_name != NULL && !strcmp(p->psz_name, "longname"))<br>
> -        {<br>
> -            if (p->psz_value != NULL)<br>
> -                longname = p->psz_value;<br>
> -            break;<br>
> -        }<br>
> +    msg_Dbg(playlist, "adding services_discovery %s '%s'...", chain, longname);<br>
><br>
>      playlist_Lock(playlist);<br>
>      sds->node = playlist_NodeCreate(playlist, longname, playlist->p_root,<br>
> @@ -251,7 +257,6 @@ int playlist_ServicesDiscoveryAdd(playlist_t *playlist, const char *chain)<br>
>      playlist_Unlock(playlist);<br>
><br>
>      config_ChainDestroy(cfg);<br>
> -    free(name);<br>
><br>
>      if (unlikely(sds->node == NULL))<br>
>      {<br>
><br>
> -- <br>
> Pierre Ynard<br>
> "Une âme dans un corps, c'est comme un dessin sur une feuille de papier."<br>
> _______________________________________________<br>
> vlc-devel mailing list<br>
> To unsubscribe or modify your subscription options:<br>
> https://mailman.videolan.org/listinfo/vlc-devel<br></p>