<div dir="auto">Hi, Filip<div dir="auto"><br></div><div dir="auto">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. </div><div dir="auto"><br></div><div dir="auto">Best regards,</div><div dir="auto">Andrei Ciurea</div></div><div class="gmail_extra"><br><div class="gmail_quote">On 24 Feb 2017 9:11 pm, "Filip Roséen" <<a href="mailto:filip@atch.se">filip@atch.se</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><u></u>


  
  
  
  
  

<div>
<p>Hi Andrei,</p>
<p>On 2017-02-24 18:07, Andrei Ciurea wrote:</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> 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.</code></pre>
</blockquote>
<p>Best of luck!</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> I intend to start working at another bug, do you have any advice for me?</code></pre>
</blockquote>
<p>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.</p>
<p>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 <code>vlc-devel</code> (and <code>#videolan</code> @ <code>freenode</code>).</p>
<p>All the best,<br>
Filip</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> Best Regards,
 Andrei Ciurea

 2017-02-24 14:38 GMT+02:00 Filip Roséen <<a href="mailto:filip@atch.se" target="_blank">filip@atch.se</a>>:</code></pre>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> 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 ++++++++++++++++++++++++++++++<wbr>+++++++
   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_<wbr>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_<wbr>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_<wbr>LONGTEXT, false )
       add_key( "key-subtitle-track", KEY_SUBTITLE_TRACK,

 Best Regards,
 Filip Roséen

 ______________________________<wbr>_________________
 vlc-devel mailing list
 To unsubscribe or modify your subscription options:
 <a href="https://mailman.videolan.org/listinfo/vlc-devel" target="_blank">https://mailman.videolan.org/<wbr>listinfo/vlc-devel</a></code></pre>
</blockquote>
</blockquote>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> ______________________________<wbr>_________________
 vlc-devel mailing list
 To unsubscribe or modify your subscription options:
 <a href="https://mailman.videolan.org/listinfo/vlc-devel" target="_blank">https://mailman.videolan.org/<wbr>listinfo/vlc-devel</a></code></pre>
</blockquote>
</div>

<br>______________________________<wbr>_________________<br>
vlc-devel mailing list<br>
To unsubscribe or modify your subscription options:<br>
<a href="https://mailman.videolan.org/listinfo/vlc-devel" rel="noreferrer" target="_blank">https://mailman.videolan.org/<wbr>listinfo/vlc-devel</a><br></blockquote></div></div>