[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
Filip Roséen
filip at atch.se
Fri Feb 24 13:38:07 CET 2017
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20170224/7ced7aeb/attachment.html>
More information about the vlc-devel
mailing list