[vlc-devel] [PATCH] [PATCH] This patch contains the files I modified to resolve the bug: https://trac.videolan.org/vlc/ticket/5708 The shortcat i used is "/" Thank you

Andrei Ciurea andrei.ciurea96 at gmail.com
Fri Feb 24 20:59:08 CET 2017


Hi, Filip

Thank you. I hope that my work will be helpful here at VLC. Regarding the
bugs I chose a few that I thought would be entry-level and I will start
working at one of them this weekend.

Best regards,
Andrei Ciurea

On 24 Feb 2017 9:11 pm, "Filip Roséen" <filip at atch.se> wrote:

> Hi Andrei,
>
> On 2017-02-24 18:07, Andrei Ciurea wrote:
>
>  Thank you for review. The comments were for me to be sure that i remember
>  what i've changed (at a search in the whole package). I'll take good care
>  next time and i will make the changes and send this patch again.
>
> Best of luck!
>
>  I intend to start working at another bug, do you have any advice for me?
>
> I have been meaning to compile an “entry-level list of bugs” for so so
> long, but I have not gotten around to it; with that in mind I sadly do not
> have any particular bug to recommend.
>
> Given that it seems that you have a lot of interest in contributing to
> VLC, I’d like to take the opportunity to welcome you onto vlc-devel (and
> #videolan @ freenode).
>
> All the best,
> Filip
>
>  Best Regards,
>  Andrei Ciurea
>
>  2017-02-24 14:38 GMT+02:00 Filip Roséen <filip at atch.se>:
>
>  Hi Andrei,
>
>  I am glad you figured out how to create a patch and sent it to vlc-devel,
>  I was about to ping you on IRC a few hours ago to see how you were doing
>  (but I forgot).
>
>  In order increase the chance of merging the changes some slight clean-up
>  (mostly regarding comments) is recommended (see the rest of this extremely
>  minor review).
>
>  I am aware that you simply copied the implementation related to cycling
>  audio-tracks, so my comments regarding style and such are just for future
>  reference (especially if someone decides to refactor the implementation as
>  mentioned).
>
>  On 2017-02-24 13:59, Andrei Ciurea wrote:
>
>   ---
>    include/vlc_keys.h        |  3 +++
>    modules/control/hotkeys.c | 37 +++++++++++++++++++++++++++++++++++++
>    src/config/keys.c         |  4 ++++
>    src/libvlc-module.c       |  7 +++++++
>    4 files changed, 51 insertions(+)
>
>
>  Please revise the commit message so that it includes a description, and a
>  more suitable title.
>
>   diff --git a/include/vlc_keys.h b/include/vlc_keys.h
>   index 8d8df53..bb1f18e 100644
>   --- a/include/vlc_keys.h
>   +++ b/include/vlc_keys.h
>   @@ -100,6 +100,8 @@
>    #define KEY_ZOOM_OUT         0x00610000
>    #define KEY_BRIGHTNESS_UP    0x00620000
>    #define KEY_BRIGHTNESS_DOWN  0x00630000
>   + //my mod. asignin' sa key for video_track
>
>  I assume the above comment to be an unfortunate leftover, but please
>  remove such from the final patch as they have no value inside the codebase.
>
>   +#define KEY_MEDIA_VIDEO      0x00640000
>
>    #define KEY_MOUSEWHEELUP     0x00F00000
>    #define KEY_MOUSEWHEELDOWN   0x00F10000
>   @@ -171,6 +173,7 @@ typedef enum vlc_action {
>        ACTIONID_SUBPOS_UP,
>        ACTIONID_SUBPOS_DOWN,
>        ACTIONID_AUDIO_TRACK,
>   +    ACTIONID_VIDEO_TRACK,
>        ACTIONID_SUBTITLE_TRACK,
>        ACTIONID_SUBTITLE_TOGGLE,
>        ACTIONID_SUBTITLE_TEXT_SCALE_NORMAL,
>   diff --git a/modules/control/hotkeys.c b/modules/control/hotkeys.c
>   index 4e4f368..3338a1f 100644
>   --- a/modules/control/hotkeys.c
>   +++ b/modules/control/hotkeys.c
>   @@ -737,6 +737,43 @@ static int PutAction( intf_thread_t *p_intf, input_thread_t *p_input,
>                    var_FreeList( &list, &list2 );
>                }
>                break;
>   +            //my mod
>
>  See remark about leftover comments.
>
>   +        case ACTIONID_VIDEO_TRACK:
>   +         if( p_input )
>   +            {
>   +                vlc_value_t val, list, list2;
>   +                int i_count, i;
>
>  There is no need to seperate declaration and initialization ( i_count,
>  and i) like in the above. Just do declaration and initialization in one
>  step from the moment you need the variable (increases readability and
>  heavily simplifies refactoring).
>
>   +                var_Get( p_input, "video-es", &val );
>   +                var_Change( p_input, "video-es", VLC_VAR_GETCHOICES,
>   +                            &list, &list2 );
>   +                i_count = list.p_list->i_count;
>   +                if( i_count > 1 )
>   +                {
>
>  To increase readability you could use list.p_list->i_count in the
>  *if-condition*, and then declare a variable within the *if-body* for
>  easier access (making it clear that i_count is only used in this scope).
>
>   +                    for( i = 0; i < i_count; i++ )
>   +                    {
>   +                        if( val.i_int == list.p_list->p_values[i].i_int )
>   +                        {
>   +                            break;
>   +                        }
>   +                    }
>   +                    /* value of video-es was not in choices list */
>   +                    if( i == i_count )
>   +                    {
>   +                        msg_Warn( p_input,
>   +                                  "invalid current video track, selecting 0" );
>   +                        i = 0;
>   +                    }
>   +                    else if( i == i_count - 1 )
>   +                        i = 1;
>   +                    else
>   +                        i++;
>   +                    var_Set( p_input, "video-es", list.p_list->p_values[i] );
>   +                    DisplayMessage( p_vout, _("Video track: %s"),
>   +                                    list2.p_list->p_values[i].psz_string );
>   +                }
>   +                var_FreeList( &list, &list2 );
>   +            }
>   +            break;
>
>  The implementation under case ACTIONID_VIDEO_TRACK only differs from the
>  already present case ACTIONID_AUDIO_TRACK in terms of what variable is
>  being accessed, and the strings used for diagnostics.
>
>  Have you considered refactoring so that both cases uses the same
>  implementation?
>
>            case ACTIONID_SUBTITLE_TRACK:
>                if( p_input )
>                {
>   diff --git a/src/config/keys.c b/src/config/keys.c
>   index 336466b..3d28b26 100644
>   --- a/src/config/keys.c
>   +++ b/src/config/keys.c
>   @@ -86,6 +86,8 @@ static const struct key_descriptor_s vlc_keys[] =
>        { N_("Left"),              KEY_LEFT              },
>        { N_("Media Angle"),       KEY_MEDIA_ANGLE       },
>        { N_("Media Audio Track"), KEY_MEDIA_AUDIO       },
>   +    //my mod asign key descriptor and vlc_key
>
>  See remark about leftover comments.
>
>   +    { N_("Media Video Track"), KEY_MEDIA_VIDEO       },
>        { N_("Media Forward"),     KEY_MEDIA_FORWARD     },
>        { N_("Media Menu"),        KEY_MEDIA_MENU        },
>        { N_("Media Next Frame"),  KEY_MEDIA_FRAME_NEXT  },
>   @@ -273,6 +275,8 @@ static const struct action actions[] =
>        /* *MUST* be sorted (ASCII order) */
>        { "aspect-ratio", ACTIONID_ASPECT_RATIO, },
>        { "audio-track", ACTIONID_AUDIO_TRACK, },
>   +    //my mod. asigning a actionid to video track
>
>  See remark about leftover comments.
>
>   +    { "video-track", ACTIONID_VIDEO_TRACK, },
>        { "audiodelay-down", ACTIONID_AUDIODELAY_DOWN, },
>        { "audiodelay-up", ACTIONID_AUDIODELAY_UP, },
>        { "audiodevice-cycle", ACTIONID_AUDIODEVICE_CYCLE, },
>   diff --git a/src/libvlc-module.c b/src/libvlc-module.c
>   index 6d60e80..b2e6988 100644
>   --- a/src/libvlc-module.c
>   +++ b/src/libvlc-module.c
>   @@ -1357,6 +1357,8 @@ static const char *const mouse_wheel_texts[] = {
>
>    #define AUDIO_TRACK_KEY_TEXT N_("Cycle audio track")
>    #define AUDIO_TRACK_KEY_LONGTEXT N_("Cycle through the available audio tracks(languages).")
>   +#define VIDEO_TRACK_KEY_TEXT N_("Cycle video track")
>   +#define VIDEO_TRACK_KEY_LONGTEXT N_("Cycle through the available video tracks.")
>    #define SUBTITLE_TRACK_KEY_TEXT N_("Cycle subtitle track")
>    #define SUBTITLE_TRACK_KEY_LONGTEXT N_("Cycle through the available subtitle tracks.")
>    #define SUBTITLE_TOGGLE_KEY_TEXT N_("Toggle subtitles")
>   @@ -2198,6 +2200,7 @@ vlc_module_begin ()
>    #   define KEY_AUDIODELAY_UP      "g"
>    #   define KEY_AUDIODELAY_DOWN    "f"
>    #   define KEY_AUDIO_TRACK        "l"
>   +#   define KEY_VIDEO_TRACK        "/"
>    #   define KEY_SUBTITLE_TRACK     "s"
>    #   define KEY_SUBTITLE_TOGGLE    "Shift+s"
>    #   define KEY_PROGRAM_SID_NEXT   "x"
>   @@ -2340,6 +2343,7 @@ vlc_module_begin ()
>    #   define KEY_LOOP               "l"
>
>    #   define KEY_AUDIO_TRACK        "b"
>   +#   define KEY_VIDEO_TRACK        "/"
>    #   define KEY_SUBTITLE_TRACK     "v"
>    #   define KEY_SUBTITLE_TOGGLE    "Shift+v"
>    #   define KEY_PROGRAM_SID_NEXT   "x"
>   @@ -2512,6 +2516,9 @@ vlc_module_begin ()
>                 AUDIODELAY_DOWN_KEY_TEXT, AUDIODELAY_DOWN_KEY_LONGTEXT, true )
>        add_key( "key-audio-track", KEY_AUDIO_TRACK, AUDIO_TRACK_KEY_TEXT,
>                 AUDIO_TRACK_KEY_LONGTEXT, false )
>   +             //my mod
>
>  See remark about leftover comments.
>
>   +    add_key( "key-video-track", KEY_VIDEO_TRACK, VIDEO_TRACK_KEY_TEXT,
>   +             VIDEO_TRACK_KEY_LONGTEXT, false )
>        add_key( "key-audiodevice-cycle", KEY_AUDIODEVICE_CYCLE, AUDI_DEVICE_CYCLE_KEY_TEXT,
>                 AUDI_DEVICE_CYCLE_KEY_LONGTEXT, false )
>        add_key( "key-subtitle-track", KEY_SUBTITLE_TRACK,
>
>  Best Regards,
>  Filip Roséen
>
>  _______________________________________________
>  vlc-devel mailing list
>  To unsubscribe or modify your subscription options:
>  https://mailman.videolan.org/listinfo/vlc-devel
>
>  _______________________________________________
>  vlc-devel mailing list
>  To unsubscribe or modify your subscription options:
>  https://mailman.videolan.org/listinfo/vlc-devel
>
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20170224/b045ce81/attachment-0001.html>


More information about the vlc-devel mailing list