[vlc-devel] [PATCH v2 01/11] core: move all actions handling in actions.c

[Hugo Beauzée-Luyssen]

On Thu, Aug 10, 2017, at 06:01 PM, Rémi Denis-Courmont wrote:
> Le torstaina 10. elokuuta 2017, 17.35.59 EEST Hugo Beauzée-Luyssen a
> écrit :
> > > > diff --git a/include/vlc_actions.h b/include/vlc_actions.h
> > > > index 404c1662a8..0995d3db5c 100644
> > > > --- a/include/vlc_actions.h
> > > > +++ b/include/vlc_actions.h
> > > > @@ -241,6 +241,7 @@ typedef enum vlc_action_id {
> > > > 
> > > >      ACTIONID_VIEWPOINT_FOV_OUT,
> > > >      ACTIONID_VIEWPOINT_ROLL_CLOCK,
> > > >      ACTIONID_VIEWPOINT_ROLL_ANTICLOCK,
> > > > 
> > > > +    ACTIONID_VIEWPOINT_UPDATE, /* Arg 1: vlc_viewpoint_t* */
> > > 
> > > The whole point (as you said yourself) of actions is to factor the
> > > calling
> > > code. Indeed, it enables callers to map a key code or a string to an
> > > action ID
> > > without knowing/caring what it actually means.
> > > 
> > > What exactly is the point of variadic actions then?
> > 
> > To specify a context. It is also safe to pass none, and a default one
> > will be inferred
> 
> I am referring to VIEWPOINT_UPDATE needing a viewpoint structure. Actions
> are 
> mapped from key codes. So there may be a context (vout window, aout or
> input) 
> from which the key press was captured. But I don´t see how any other
> parameter 
> can be generated in general, and a viewpoint structure in particular.
> 
> > (also granted, this is done in next patch, but I'll gladly merge the 2)
> > Since the context differs from action to action, I'm not sure how to do
> > this without a variadic function, or N functions with different
> > prototype per action category. A variadic function also eases the user
> > from not providing a context when the default one is enough.
> 
> You can´t have a default with variadic parameters. You need to know if
> there 
> will be a first variadic parameter, and if so, of what type, from the
> fixed 
> parameters. And none of the three fixed parameters allow that.
> 

This is what patch 2 does. I can rebase 1 & 2 together if you feel it
should be done that way.

> > > >      /* Combo Actions */
> > > >      ACTIONID_COMBO_VOL_FOV_UP,
> > > >      ACTIONID_COMBO_VOL_FOV_DOWN,
> > > > 
> > > > @@ -272,4 +273,18 @@ VLC_API const char* const*
> > > > 
> > > >  vlc_actions_get_key_names(vlc_object_t *p_obj);
> > > >  #define vlc_actions_get_key_names(x)
> > > > 
> > > > vlc_actions_get_key_names(VLC_OBJECT(x))
> > > > 
> > > > +/**
> > > > + * Execute an action
> > > > + *
> > > > + * \param a valid vlc object
> > > > + * \param i_action action to execute
> > > > + * \param b_notify notify a new state via the OSD of the current vout
> > > > + * \return VLC_SUCCESS or a VLC error
> > > > + */
> > > > +VLC_API int
> > > > +vlc_actions_do(vlc_object_t *p_obj, vlc_action_id_t i_action,
> > > > +               bool b_notify, ...);
> > > > +#define vlc_actions_do(a,b,c,...) vlc_actions_do(VLC_OBJECT(a),b,c, \
> > > > +    ##__VA_ARGS__)
> > > 
> > > As far as I am aware, this macro violates ISO C.
> > 
> > I suppose you are referring to the fact that the macro is one multiple
> > lines?
> > I'm genuinely interested in the source for this, as this is not
> > how I understand §6.10.3.5
> 
> Previous paragraph:
> 
> (...) "there shall be *more* arguments in the invocation than there are 
> parameters in the macro definition (excluding the ...)."
> (emphasis mine)
> 

Fair enough, I didn't think the ##__VA_ARGS was a GCC extension.
I'll submit a new version that let the number of args be part of the
variable arguments. Mandating the number of parameters to be provided
will be guaranteed by the actual function's prototype, so it should work
out fine.

> -- 
> 雷米‧德尼-库尔蒙
> https://www.remlab.net/
> 
> _______________________________________________
> 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