[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 20:11:29 CET 2017
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20170224/0582a798/attachment.html>
More information about the vlc-devel
mailing list