[vlc-devel] [PATCH] Add VLSub feature: Download multiple subtitles

[Hugo Beauzée-Luyssen]

Hi,

The patch looks ok to me, however a few remarks inline:

On Wed, Aug 12, 2020, at 3:49 PM, Alon Sirota wrote:
> All selected subtitles are now downloaded, instead of only one.
> 
> Change 'get_first_sel' function to return ALL selected subtitles
> instead of only one, and refactored name to 'get_all_sel'.
> 
> Change 'download_subtitles' function to iterate over 'get_all_sel'
> returned table instead of downloading the only subtitle from old 
> 'get_first_sel'.
> ---
>  share/lua/extensions/VLSub.lua | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/share/lua/extensions/VLSub.lua b/share/lua/extensions/VLSub.lua
> index 2edc73ecdc..ceb49e302d 100644
> --- a/share/lua/extensions/VLSub.lua
> +++ b/share/lua/extensions/VLSub.lua
> @@ -1429,12 +1429,15 @@ function display_subtitles()
>      lang["mess_no_res"])
>  end
>  
> -function get_first_sel(list)
> +  function get_all_sel(list)

You're changing the indentation of this function for no apparent reason

>    local selection = list:get_selection()
> +    local indexes = {}
> +    local i = 1 -- key counter
>    for index, name in pairs(selection) do
> -    return index
> +      indexes[i] = index

You could get rid of the local index by doing indexes[#indexes + 1]

> +      i = i + 1
>    end
> -  return 0
> +    return indexes
>  end
>  
>  function find_subtitle_in_archive(archivePath, subfileExt)
> @@ -1453,18 +1456,18 @@ function find_subtitle_in_archive(archivePath, 
> subfileExt)
>  end
>  
>  function download_subtitles()
> -  local index = get_first_sel(input_table["mainlist"])
> -
> -  if index == 0 then
> +    local indexes = get_all_sel(input_table["mainlist"])
> +    if #indexes == 0 then -- if no subtitles selected
>      setMessage(lang["mess_no_selection"])
>      return false
>    end
> -
>    openSub.actionLabel = lang["mess_downloading"]
>  
>    display_subtitles() -- reset selection
>  
> -  local item = openSub.itemStore[index]
> +  -- for each subtitle index

I'm not sure this comment is needed, the code is self-explanatory IMO

> +  for _, index in pairs(indexes) do

I think ipairs would be more suited for this iteration since it conserves the ordering

> +    item = openSub.itemStore[index]
>  
>    if openSub.option.downloadBehaviour == 'manual'
>    or not openSub.file.hasInput then
> @@ -1581,6 +1584,7 @@ function download_subtitles()
>  
>    setMessage(message)
>  end
> +end

Please add a 2ndcommit to reindent the content of the new block

>  
>  function dump_zip(url, dir, subfileName)
>    -- Dump zipped data in a temporary file

Regards,

-- 
  Hugo Beauzée-Luyssen
  hugo at beauzee.fr