[vlc-devel] [PATCH] Fixed libvlc_vlm_show_media to return a string representation of a vlm media again

[Alexander Bethke]

Hi,

thanks for the comments, I gonna work over the code soon (as soon as I 
have time).

Regards, Alex



Laurent Aimar wrote:
> Hi,
>
> On Wed, Mar 11, 2009, Alexander Bethke wrote:
>   
>> +static vlm_media_instance_t *libvlc_vlm_get_media_instance( libvlc_instance_t *p_instance,
>> +                                                            const char *psz_name,
>> +                                                            int i_minstance_idx,
>> +                                                            libvlc_exception_t *p_exception )
>> +{
>> +    vlm_t *p_vlm;
>> +    vlm_media_instance_t **pp_minstance;
>> +    vlm_media_instance_t *p_minstance;
>> +    int i_minstance;
>> +    int64_t id;
>> +
>> +    VLM_RET(p_vlm, NULL);
>> +
>> +    if( vlm_Control( p_vlm, VLM_GET_MEDIA_ID, psz_name, &id ) ||
>> +        vlm_Control( p_vlm, VLM_GET_MEDIA_INSTANCES, id, &pp_minstance, &i_minstance ) )
>> +    {
>> +        libvlc_exception_raise( p_exception, "Unable to get %s instances", psz_name );
>> +        return NULL;
>> +    }
>> +    p_minstance = NULL;
>> +    if( i_minstance_idx >= 0 && i_minstance_idx < i_minstance )
>> +    {
>> +        p_minstance = pp_minstance[i_minstance_idx];
>> +        TAB_REMOVE( i_minstance, pp_minstance, p_minstance );
>> +    }
>> +    while( i_minstance > 0 )
>> +        vlm_media_instance_Delete( pp_minstance[--i_minstance] );
>>     
> It would be cleaner to use an extra variable like
> for( int i = 0; i < i_minstance; i++ )
>     vlm_media_instance_Delete( pp_minstance[i] );
> as i_minstance is the number of element.
> (Cosmetic)
>
>   
>>  /* local function to be used in libvlc_vlm_show_media only */
>>  static char* recurse_answer( char* psz_prefix, vlm_message_t *p_answer ) {
>>      char* psz_childprefix;
>> -    char* psz_response="";
>> +    char* psz_response;
>>      char* response_tmp;
>>      int i;
>>      vlm_message_t *aw_child, **paw_child;
>>  
>> +    asprintf( &psz_response, "");
>>     
> You need to check asprintf return value.
>   
>>      asprintf( &psz_childprefix, "%s%s.", psz_prefix, p_answer->psz_name );
>>     
>  And while you are at it, if you could add it for existing asprintf it
> would be cool :)
>
>   
>>  
>>      if ( p_answer->i_child )
>> @@ -65,96 +118,40 @@ static char* recurse_answer( char* psz_prefix, vlm_message_t *p_answer ) {
>>      return psz_response;
>>  }
>>  
>> -char* libvlc_vlm_show_media( libvlc_instance_t *p_instance, char *psz_name,
>> +/*  Returning a string representation of the media I find should be changed to
>> +    either refactoring this to return some sort of struct or or to add more
>> +    libvlc_vlm_get_media_instance_x methods to make available all information
>> +    that you get out of the string representation.
>> +    -Alexander Bethke */
>> +char* libvlc_vlm_show_media( libvlc_instance_t *p_instance,
>> +                             const char *psz_name,
>>                               libvlc_exception_t *p_exception )
>>  {
>>      char *psz_message;
>>      vlm_message_t *answer;
>>      char *psz_response;
>> +    vlm_t *p_vlm;
>> +
>> +    VLM_RET(p_vlm, NULL);
>>  
>> -    CHECK_VLM;
>>      asprintf( &psz_message, "show %s", psz_name );
>>      asprintf( &psz_response, "", psz_name );
>>     
>  Same.
>   
>> -    vlm_ExecuteCommand( p_instance->p_vlm, psz_message, &answer );
>> +    vlm_ExecuteCommand( p_vlm, psz_message, &answer );
>>     
> vlm_ExecuteCommand has a return value that need checking.
>
>   
>>      if( answer->psz_value )
>>      {
>>          libvlc_exception_raise( p_exception, "Unable to call show %s: %s",
>>                                  psz_name, answer->psz_value );
>>      }
>> -    else
>> -    {
>> +    else {
>>     
>  It would be better if you keep the { placement coherent.
>
>   
>>          if ( answer->child )
>>          {
>>              psz_response = recurse_answer( "", answer );
>>          }
>>      }
>> +
>>      free( psz_message );
>>      return(psz_response );
>>  }
>>     
>
> Regards,
>
>