[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 17:07:01 CET 2017
Hi, Filip.
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. I intend
to start working at another bug, do you have any advice for me?
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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20170224/503b9bfb/attachment.html>
More information about the vlc-devel
mailing list