<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
<meta http-equiv="Content-Style-Type" content="text/css" />
<meta name="generator" content="pandoc" />
<title></title>
<style type="text/css">code{white-space: pre;}</style>
</head>
<body>
<p>Hi Andrei,</p>
<p>I am glad you figured out how to create a patch and sent it to <code>vlc-devel</code>, I was about to ping you on IRC a few hours ago to see how you were doing (but I forgot).</p>
<p>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).</p>
<p>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).</p>
<p>On 2017-02-24 13:59, Andrei Ciurea wrote:</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> ---
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(+)
</code></pre>
</blockquote>
<p>Please revise the commit message so that it includes a description, and a more suitable title.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> 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</code></pre>
</blockquote>
<p>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.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +#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</code></pre>
</blockquote>
<p>See remark about leftover comments.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + case ACTIONID_VIDEO_TRACK:
+ if( p_input )
+ {
+ vlc_value_t val, list, list2;
+ int i_count, i;</code></pre>
</blockquote>
<p>There is no need to seperate declaration and initialization ( <code>i_count</code>, and <code>i</code>) 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).</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + 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 )
+ {</code></pre>
</blockquote>
<p>To increase readability you could use <code>list.p_list->i_count</code> in the <em>if-condition</em>, and then declare a variable within the <em>if-body</em> for easier access (making it clear that <code>i_count</code> is only used in this scope).</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + 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;</code></pre>
</blockquote>
<p>The implementation under <code>case ACTIONID_VIDEO_TRACK</code> only differs from the already present <code>case ACTIONID_AUDIO_TRACK</code> in terms of what variable is being accessed, and the strings used for diagnostics.</p>
<p>Have you considered refactoring so that both cases uses the same implementation?</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> 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</code></pre>
</blockquote>
<p>See remark about leftover comments.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + { 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</code></pre>
</blockquote>
<p>See remark about leftover comments.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + { "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</code></pre>
</blockquote>
<p>See remark about leftover comments.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + 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,</code></pre>
</blockquote>
<p>Best Regards,<br />
Filip Roséen</p>
</body>
</html>