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

[Rémi Denis-Courmont]

On Wed, 8 Sep 2010 09:57:17 +0200, 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.

We definitely don't want to expose vlc_keys.h to libvlc apps, if only
because it does not use a clean names space. But you can include vlc_keys.h
from libvlc code and write a flat table for mapping libvlc_navigate_t to
vlc_key.

> 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.

-- 
Rémi Denis-Courmont
http://www.remlab.net
http://fi.linkedin.com/in/remidenis