[vlc-devel] [PATCH 1/2] Extend libvlc Media Player API for DVD menu navigation

[Pierre d'Herbemont]

On Wed, Sep 8, 2010 at 2:22 PM, Arnaud VALLAT <rno.rno at gmail.com> wrote:
> On Wed, Sep 8, 2010 at 9:57 AM, Arnaud VALLAT <rno.rno at gmail.com> wrote:
>> Hello,
>>
>> 2010/9/7 Rémi Denis-Courmont <remi at remlab.net>:
>>>
>>> On Tue,  7 Sep 2010 14:41:36 +0200, Arnaud Vallat <rno.rno at gmail.com>
>>> wrote:
>>>> +    switch (navigate)
>>>> +      {
>>>> +      case libvlc_navigate_up:
>>>> +     i_key = vlc_GetActionId("key-nav-up");
>>>> +     break;
>>>> +
>>>> +      case libvlc_navigate_down:
>>>> +     i_key = vlc_GetActionId("key-nav-down");
>>>> +     break;
>>>> +
>>>> +      case libvlc_navigate_left:
>>>> +     i_key = vlc_GetActionId("key-nav-left");
>>>> +     break;
>>>> +
>>>> +      case libvlc_navigate_right:
>>>> +     i_key = vlc_GetActionId("key-nav-right");
>>>> +     break;
>>>
>>> Oh no. First you convert an integer to a string using a linear search
>>> (switch statement).
>>> Then you convert the string back to an integer using a binary search
>>> (vlc_GetActionId).
>>>
>>> How about a static constant table that maps your libvlc enum directly to
>>> vlc_key? It's much simpler and it is O(1).
>>
>> First of all: thanks for the review :-)
>>
>> Yes I can do that but vlc_keys are defined in vlc_keys.h which is not part
>> of libvlc headers. However I can write proper function for each navigation
>> which would take only libvlc_media_player_t as parameter. And then, those
>> functions will directly use vlc_keys value for each direction.
>
> Are you ok with that?

Using #define for function is rather unusual in libvlc headers.
Keeping preprocessor tricks to the minimum is also a way to ensure
headers readability. Sometimes.

Pierre.