[vlc-devel] [PATCH 1/3] qt: use shared pointers in menu items

Hugo Beauzée-Luyssen hugo at beauzee.fr
Mon Mar 18 14:20:14 CET 2019


Hi,

I like the idea but why using QSharedPointer instead of vlc_shared_data_ptr or wrap_cptr?
I also agree with Thomas that this is not the best way to transition to the new UI seamlessly.

On Sun, Mar 17, 2019, at 10:56 AM, Rémi Denis-Courmont wrote:
> Use QSharedPointer shared pointers to reference VLC objects in
> automatically generated menus. This enables using a custom callback
> for releasing the reference to object of different types.
> ---
>  modules/gui/qt/menus.cpp | 100 ++++++++++++++++++++++++++-------------
>  modules/gui/qt/menus.hpp |  25 +++++-----
>  2 files changed, 80 insertions(+), 45 deletions(-)
> 
> diff --git a/modules/gui/qt/menus.cpp b/modules/gui/qt/menus.cpp
> index b102fa6902..1e7b34b386 100644
> --- a/modules/gui/qt/menus.cpp
> +++ b/modules/gui/qt/menus.cpp
> @@ -80,6 +80,35 @@ static QActionGroup *currentGroup;
>  QMenu *VLCMenuBar::recentsMenu = NULL;
>  RendererMenu *VLCMenuBar::rendererMenu = NULL;
>  
> +static void releaseDummy(vlc_object_t *)
> +{
> +}
> +
> +static QVLCObject wrap(playlist_t *pl)
> +{
> +    return QVLCObject(VLC_OBJECT(pl), releaseDummy);
> +}
> +
> +static QVLCObject wrap(intf_thread_t *intf)
> +{
> +    return QVLCObject(VLC_OBJECT(intf), releaseDummy);
> +}
> +
> +static QVLCObject wrap(input_thread_t *input)
> +{
> +    return QVLCObject(VLC_OBJECT(input), releaseDummy);
> +}
> +
> +static QVLCObject wrap(audio_output_t *aout)
> +{
> +    return QVLCObject(VLC_OBJECT(aout), releaseDummy);
> +}
> +
> +static QVLCObject wrap(vout_thread_t *vout)
> +{
> +    return QVLCObject(VLC_OBJECT(vout), releaseDummy);
> +}
> +
>  /**
>   * @brief Add static entries to DP in menus
>   **/
> @@ -209,7 +238,7 @@ static QAction * FindActionWithVar( QMenu *menu, 
> const char *psz_var )
>  #define PUSH_OBJVAR(object,var) \
>      do { \
>          varnames.append(var); \
> -        objects.append(VLC_OBJECT(object)); \
> +        objects.append(wrap(object)); \
>      } while (0)
>  
>  #define PUSH_VAR(var) PUSH_OBJVAR(p_object, var)
> @@ -217,7 +246,7 @@ static QAction * FindActionWithVar( QMenu *menu, 
> const char *psz_var )
>  #define PUSH_PLVAR(var) PUSH_OBJVAR(pl, var)
>  
>  static int InputAutoMenuBuilder( input_thread_t *p_input,
> -        QVector<vlc_object_t *> &objects, QVector<const char *> &varnames )
> +        QVector<QVLCObject> &objects, QVector<const char *> &varnames )
>  {
>      PUSH_INPUTVAR( "bookmark" );
>      PUSH_INPUTVAR( "title" );
> @@ -228,7 +257,7 @@ static int InputAutoMenuBuilder( input_thread_t *p_input,
>  }
>  
>  static int VideoAutoMenuBuilder( playlist_t *pl, input_thread_t *p_input,
> -        QVector<vlc_object_t *> &objects, QVector<const char *> &varnames )
> +        QVector<QVLCObject> &objects, QVector<const char *> &varnames )
>  {
>      vout_thread_t *p_object = p_input ? input_GetVout( p_input ) : NULL;
>  
> @@ -249,7 +278,7 @@ static int VideoAutoMenuBuilder( playlist_t *pl, 
> input_thread_t *p_input,
>  }
>  
>  static int SubsAutoMenuBuilder( input_thread_t *p_input,
> -        QVector<vlc_object_t *> &objects, QVector<const char *> &varnames )
> +        QVector<QVLCObject> &objects, QVector<const char *> &varnames )
>  {
>      PUSH_INPUTVAR( "spu-es" );
>  
> @@ -257,7 +286,7 @@ static int SubsAutoMenuBuilder( input_thread_t *p_input,
>  }
>  
>  static int AudioAutoMenuBuilder( input_thread_t *p_input,
> -        QVector<vlc_object_t *> &objects, QVector<const char *> &varnames )
> +        QVector<QVLCObject> &objects, QVector<const char *> &varnames )
>  {
>      audio_output_t *p_object = p_input ? input_GetAout( p_input ) : NULL;
>  
> @@ -553,10 +582,10 @@ QMenu *VLCMenuBar::ViewMenu( intf_thread_t 
> *p_intf, QMenu *current, MainInterfac
>   **/
>  QMenu *VLCMenuBar::InterfacesMenu( intf_thread_t *p_intf, QMenu 
> *current )
>  {
> -    QVector<vlc_object_t *> objects;
> +    QVector<QVLCObject> objects;
>      QVector<const char *> varnames;
>      varnames.append( "intf-add" );
> -    objects.append( VLC_OBJECT(p_intf) );
> +    objects.append( wrap(p_intf) );
>  
>      return Populate( current, varnames, objects );
>  }
> @@ -605,7 +634,7 @@ static inline void VolumeEntries( intf_thread_t 
> *p_intf, QMenu *current )
>   **/
>  QMenu *VLCMenuBar::AudioMenu( intf_thread_t *p_intf, QMenu * current )
>  {
> -    QVector<vlc_object_t *> objects;
> +    QVector<QVLCObject> objects;
>      QVector<const char *> varnames;
>      QMenu *audioDeviceMenu;
>      audio_output_t *p_aout;
> @@ -646,7 +675,7 @@ QMenu *VLCMenuBar::AudioMenu( intf_thread_t 
> *p_intf, QMenu * current )
>  QMenu *VLCMenuBar::SubtitleMenu( intf_thread_t *p_intf, QMenu 
> *current, bool b_popup )
>  {
>      input_thread_t *p_input;
> -    QVector<vlc_object_t *> objects;
> +    QVector<QVLCObject> objects;
>      QVector<const char *> varnames;
>  
>      if( current->isEmpty() || b_popup )
> @@ -670,7 +699,7 @@ QMenu *VLCMenuBar::SubtitleMenu( intf_thread_t 
> *p_intf, QMenu *current, bool b_p
>  QMenu *VLCMenuBar::VideoMenu( intf_thread_t *p_intf, QMenu *current )
>  {
>      input_thread_t *p_input;
> -    QVector<vlc_object_t *> objects;
> +    QVector<QVLCObject> objects;
>      QVector<const char *> varnames;
>  
>      if( current->isEmpty() )
> @@ -747,7 +776,7 @@ QMenu *VLCMenuBar::RebuildNavigMenu( intf_thread_t 
> *p_intf, QMenu *menu, bool b_
>  {
>      /* */
>      input_thread_t *p_object;
> -    QVector<vlc_object_t *> objects;
> +    QVector<QVLCObject> objects;
>      QVector<const char *> varnames;
>  
>      /* Get the input and hold it */
> @@ -802,7 +831,7 @@ QMenu *VLCMenuBar::HelpMenu( QWidget *parent )
>   *****************************************************************************/
>  #define POPUP_BOILERPLATE \
>      QMenu* menu;  \
> -    QVector<vlc_object_t *> objects; \
> +    QVector<QVLCObject> objects; \
>      QVector<const char *> varnames; \
>      input_thread_t *p_input = THEMIM->getInput();
>  
> @@ -1020,6 +1049,7 @@ QMenu* VLCMenuBar::PopupMenu( intf_thread_t 
> *p_intf, bool show )
>  
>      if( p_input )
>      {
> +        QVLCObject input = wrap(p_input);
>          QMenu *submenu;
>          vout_thread_t *p_vout = THEMIM->getVout();
>  
> @@ -1031,10 +1061,11 @@ QMenu* VLCMenuBar::PopupMenu( intf_thread_t 
> *p_intf, bool show )
>              b_isFullscreen = !( !val.b_bool );
>              if( b_isFullscreen )
>              {
> +                QVLCObject playlist = wrap(THEPL);
>                  val.b_bool = false;
>                  CreateAndConnect( menu, "fullscreen",
>                          qtr( "Leave Fullscreen" ),"" , ITEM_NORMAL,
> -                        VLC_OBJECT(THEPL), val, VLC_VAR_BOOL, 
> b_isFullscreen );
> +                        playlist, val, VLC_VAR_BOOL, b_isFullscreen );
>              }
>              vout_Release(p_vout);
>  
> @@ -1062,7 +1093,7 @@ QMenu* VLCMenuBar::PopupMenu( intf_thread_t 
> *p_intf, bool show )
>          submenu = new QMenu( menu );
>          action = menu->addMenu( SubtitleMenu( p_intf, submenu, true ) 
> );
>          action->setText( qtr( "Subti&tle") );
> -        UpdateItem( submenu, "spu-es", VLC_OBJECT(p_input), true );
> +        UpdateItem( submenu, "spu-es", input, true );
>  
>          /* Playback menu for chapters */
>          submenu = new QMenu( menu );
> @@ -1090,12 +1121,13 @@ QMenu* VLCMenuBar::PopupMenu( intf_thread_t 
> *p_intf, bool show )
>          /* In skins interface, append some items */
>          if( p_intf->p_sys->b_isDialogProvider )
>          {
> -            vlc_object_t* p_object = vlc_object_parent(p_intf);
> +            intf_thread_t *p_object =
> +                (intf_thread_t *)vlc_object_parent(p_intf);
>              submenu->setTitle( qtr( "Interface" ) );
>  
>              /* Open skin dialog box */
>              objects.clear(); varnames.clear();
> -            objects.append( p_object );
> +            objects.append( wrap(p_object) );
>              varnames.append( "intf-skins-interactive" );
>              Populate( submenu, varnames, objects );
>              QAction* action = submenu->actions().back();
> @@ -1103,7 +1135,7 @@ QMenu* VLCMenuBar::PopupMenu( intf_thread_t 
> *p_intf, bool show )
>  
>              /* list of skins available */
>              objects.clear(); varnames.clear();
> -            objects.append( p_object );
> +            objects.append( wrap(p_object) );
>              varnames.append( "intf-skins" );
>              Populate( submenu, varnames, objects );
>  
> @@ -1206,7 +1238,7 @@ void VLCMenuBar::updateSystrayMenu( MainInterface 
> *mi,
>   
> *************************************************************************/
>  QMenu * VLCMenuBar::Populate( QMenu *current,
>                                QVector< const char *> & varnames,
> -                              QVector<vlc_object_t *> & objects )
> +                              QVector<QVLCObject> & objects )
>  {
>      QMenu *menu = current;
>      assert( menu );
> @@ -1244,7 +1276,7 @@ static bool IsMenuEmpty( const char *psz_var, 
> vlc_object_t *p_object )
>  #define TEXT_OR_VAR qfue ( text ? text : psz_var )
>  
>  void VLCMenuBar::UpdateItem( QMenu *menu,
> -        const char *psz_var, vlc_object_t *p_object, bool b_submenu )
> +        const char *psz_var, QVLCObject& p_object, bool b_submenu )
>  {
>      vlc_value_t val;
>      char *text;
> @@ -1271,7 +1303,7 @@ void VLCMenuBar::UpdateItem( QMenu *menu,
>       || !strcmp( psz_var, "bookmark" ) )
>          i_type = VLC_VAR_INTEGER | VLC_VAR_HASCHOICE;
>      else
> -        i_type = var_Type( p_object, psz_var );
> +        i_type = var_Type( &*p_object, psz_var );

You probably want to use p_object.data() instead, ditto below.

>  
>      switch( i_type & VLC_VAR_TYPE )
>      {
> @@ -1289,7 +1321,7 @@ void VLCMenuBar::UpdateItem( QMenu *menu,
>      }
>  
>      /* Make sure we want to display the variable */
> -    if( menu->isEmpty() && IsMenuEmpty( psz_var, p_object ) )
> +    if( menu->isEmpty() && IsMenuEmpty( psz_var, &*p_object ) )
>      {
>          if( action )
>              action->setEnabled( false );
> @@ -1297,7 +1329,7 @@ void VLCMenuBar::UpdateItem( QMenu *menu,
>      }
>  
>      /* Get the descriptive name of the variable */
> -    if( var_Change( p_object, psz_var, VLC_VAR_GETTEXT, &text ) )
> +    if( var_Change( &*p_object, psz_var, VLC_VAR_GETTEXT, &text ) )
>          text = NULL;
>  
>      if( !action )
> @@ -1339,11 +1371,11 @@ void VLCMenuBar::UpdateItem( QMenu *menu,
>          case VLC_VAR_VOID:
>              val.i_int = 0;  // Prevent the copy of an uninitialized value
>              CreateAndConnect( menu, psz_var, TEXT_OR_VAR, "", ITEM_NORMAL,
> -                    p_object, val, i_type );
> +                              p_object, val, i_type );
>              break;
>  
>          case VLC_VAR_BOOL:
> -            var_Get( p_object, psz_var, &val );
> +            var_Get( &*p_object, psz_var, &val );
>              val.b_bool = !val.b_bool;
>              CreateAndConnect( menu, psz_var, TEXT_OR_VAR, "", 
> ITEM_CHECK,
>                      p_object, val, i_type, !val.b_bool );
> @@ -1369,7 +1401,7 @@ static bool CheckTitle( vlc_object_t *p_object, 
> const char *psz_var )
>  
>  
>  int VLCMenuBar::CreateChoicesMenu( QMenu *submenu, const char *psz_var,
> -                                   vlc_object_t *p_object )
> +                                   QVLCObject& p_object )
>  {
>      vlc_value_t val;
>      vlc_value_t *val_list;
> @@ -1378,10 +1410,10 @@ int VLCMenuBar::CreateChoicesMenu( QMenu 
> *submenu, const char *psz_var,
>      int i_type;
>  
>      /* Check the type of the object variable */
> -    i_type = var_Type( p_object, psz_var );
> +    i_type = var_Type( &*p_object, psz_var );
>  
>      /* Make sure we want to display the variable */
> -    if( submenu->isEmpty() && IsMenuEmpty( psz_var, p_object ) )
> +    if( submenu->isEmpty() && IsMenuEmpty( psz_var, &*p_object ) )
>          return VLC_EGENERIC;
>  
>      switch( i_type & VLC_VAR_TYPE )
> @@ -1397,7 +1429,7 @@ int VLCMenuBar::CreateChoicesMenu( QMenu 
> *submenu, const char *psz_var,
>              return VLC_EGENERIC;
>      }
>  
> -    if( var_Change( p_object, psz_var, VLC_VAR_GETCHOICES,
> +    if( var_Change( &*p_object, psz_var, VLC_VAR_GETCHOICES,
>                      &count, &val_list, &text_list ) < 0 )
>      {
>          return VLC_EGENERIC;
> @@ -1415,7 +1447,7 @@ int VLCMenuBar::CreateChoicesMenu( QMenu 
> *submenu, const char *psz_var,
>          switch( i_type & VLC_VAR_TYPE )
>          {
>              case VLC_VAR_STRING:
> -                var_Get( p_object, psz_var, &val );
> +                var_Get( &*p_object, psz_var, &val );
>                  another_val.psz_string = strdup( CURVAL.psz_string );
>                  menutext = qfue( CURTEXT ? CURTEXT : 
> another_val.psz_string );
>                  CreateAndConnect( submenu, psz_var, menutext, "", 
> RADIO_OR_COMMAND,
> @@ -1427,17 +1459,17 @@ int VLCMenuBar::CreateChoicesMenu( QMenu 
> *submenu, const char *psz_var,
>                  break;
>  
>              case VLC_VAR_INTEGER:
> -                var_Get( p_object, psz_var, &val );
> +                var_Get( &*p_object, psz_var, &val );
>                  if( CURTEXT ) menutext = qfue( CURTEXT );
>                  else menutext = QString::number( CURVAL.i_int );
>                  CreateAndConnect( submenu, psz_var, menutext, "", 
> RADIO_OR_COMMAND,
>                          p_object, CURVAL, i_type,
>                          ( CURVAL.i_int == val.i_int )
> -                        && CheckTitle( p_object, psz_var ) );
> +                        && CheckTitle( &*p_object, psz_var ) );
>                  break;
>  
>              case VLC_VAR_FLOAT:
> -                var_Get( p_object, psz_var, &val );
> +                var_Get( &*p_object, psz_var, &val );
>                  if( CURTEXT ) menutext = qfue( CURTEXT );
>                  else menutext.sprintf( "%.2f", CURVAL.f_float );
>                  CreateAndConnect( submenu, psz_var, menutext, "", 
> RADIO_OR_COMMAND,
> @@ -1465,7 +1497,7 @@ int VLCMenuBar::CreateChoicesMenu( QMenu 
> *submenu, const char *psz_var,
>  
>  void VLCMenuBar::CreateAndConnect( QMenu *menu, const char *psz_var,
>          const QString& text, const QString& help,
> -        int i_item_type, vlc_object_t *p_obj,
> +        int i_item_type, QVLCObject& p_obj,
>          vlc_value_t val, int i_val_type,
>          bool checked )
>  {
> @@ -1513,7 +1545,7 @@ void VLCMenuBar::CreateAndConnect( QMenu *menu, 
> const char *psz_var,
>  void VLCMenuBar::DoAction( QObject *data )
>  {
>      MenuItemData *itemData = qobject_cast<MenuItemData *>( data );
> -    vlc_object_t *p_object = itemData->p_obj;
> +    vlc_object_t *p_object =&*itemData->obj;
>      if( p_object == NULL ) return;
>      const char *var = itemData->psz_var;
>      vlc_value_t val = itemData->val;
> diff --git a/modules/gui/qt/menus.hpp b/modules/gui/qt/menus.hpp
> index 536e178c79..ca24c5f8e3 100644
> --- a/modules/gui/qt/menus.hpp
> +++ b/modules/gui/qt/menus.hpp
> @@ -31,18 +31,21 @@
>  #include <QObject>
>  #include <QMenu>
>  #include <QVector>
> +#include <QSharedPointer>
> +
> +typedef QSharedPointer<vlc_object_t> QVLCObject;
>  
>  class MenuItemData : public QObject
>  {
>      Q_OBJECT
>  
>  public:
> -    MenuItemData( QObject* parent, vlc_object_t *_p_obj, int _i_type,
> +    MenuItemData( QObject* parent, QVLCObject& obj, int _i_type,
>                    vlc_value_t _val, const char *_var ) : QObject( parent )
>      {
> -        p_obj = _p_obj;
> -        if( p_obj )
> -            vlc_object_hold( p_obj );
> +        this->obj = obj;
> +        if( obj )
> +            vlc_object_hold( &*obj );

Isn't the point of the "smart" pointers to avoid this and the associated release?

>          i_val_type = _i_type;
>          val = _val;
>          psz_var = strdup( _var );
> @@ -52,11 +55,11 @@ public:
>          free( psz_var );
>          if( ( i_val_type & VLC_VAR_TYPE) == VLC_VAR_STRING )
>              free( val.psz_string );
> -        if( p_obj )
> -            vlc_object_release( p_obj );
> +        if( obj )
> +            vlc_object_release( &*obj );
>      }
>  
> -    vlc_object_t *p_obj;
> +    QVLCObject obj;
>      vlc_value_t val;
>      char *psz_var;
>  
> @@ -143,13 +146,13 @@ private:
>  
>      /* Generic automenu methods */
>      static QMenu * Populate( QMenu *current,
> -                             QVector<const char*>&, QVector<vlc_object_t *>& );
> +                             QVector<const char*>&, QVector<QVLCObject>& );
>  
>      static void CreateAndConnect( QMenu *, const char *, const QString&,
> -                                  const QString&, int, vlc_object_t *,
> +                                  const QString&, int, QVLCObject& ,
>                                    vlc_value_t, int, bool c = false );
> -    static void UpdateItem( QMenu *, const char *, vlc_object_t *, bool );
> -    static int CreateChoicesMenu( QMenu *,const char *, vlc_object_t * );
> +    static void UpdateItem( QMenu *, const char *, QVLCObject&, bool );
> +    static int CreateChoicesMenu( QMenu *,const char *, QVLCObject& );
>      static void EnableStaticEntries( QMenu *, bool );
>  
>      /* recentMRL menu */
> -- 
> 2.20.1
> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel

-- 
  Hugo Beauzée-Luyssen
  hugo at beauzee.fr


More information about the vlc-devel mailing list