[vlc-devel] [PATCH] appletrailers.lua: Fix script for website changes

[Marvin Scholz]

On 26 Oct 2016, at 17:24, Pierre Ynard wrote:

>> -    return vlc.access == "http"
>> -        and string.match( vlc.path, "trailers.apple.com" )
>> -        and string.match( vlc.path, "web.inc" )
>> +    return (vlc.access == "http" or vlc.access == "https")
>> +        and string.match( vlc.path, 
>> "^trailers%.apple%.com/trailers/.+/.+" )
>
> Isn't this script supposed to work in cooperation with the 
> appletrailers
> service discovery script? I think that's where the web.inc comes from.
> Although it's a great feature to fix it to work with the HTML page
> itself too.

Yes, but that script does not work anymore either, afaik. Or if it does
would not return matching urls anyway, because the old web.inc links are
not used anymore it seems. The JSON it queries contains no web.inc links
anymore, but those that this new version of the script handles, so it
should work just fine with the other script.

>
>> +-- JSON parser lazy initialization
>> +local json = nil
>
> Uuh apparently it's used in only one place in the script so you don't
> really need that complexity.

Probably. I copied that over from another script.

>> +    json["parse_url"] = function(url)
>> +        local stream = vlc.stream(url)
>> +        local string = ""
>> +        local line   = ""
>> +
>> +        repeat
>> +            line = stream:readline()
>> +            string = string..line
>> +        until line ~= nil
>
> vlc.stream() can fail for any number of reasons, it returns nil and 
> then
> you hit a bug calling nil:readline()
>
>> +        local movietitle = lookup_keys(info, 
>> "details/locale/en/movie_title")
>> +        local desc       = lookup_keys(info, 
>> "details/locale/en/synopsis")
>
> Is the "en" locale always available?

I think so, yes. "en" is the only entry in locale and it did not change
depending on the Accept-Language.

>
>> +            item["name"]        = movietitle .. " (" .. 
>> clip["title"]  .. ")"
>
> You should test that these fields exist, or else concatenation will
> throw an error.

Ok

>
>> +-- Get the user-preferred quality src
>> +function get_preferred_src(clip)
>> +    local resolution = vlc.var.inherit(nil, "preferred-resolution")
>> +    if resolution == -1 then
>> +        return lookup_keys(clip, 
>> "versions/enus/sizes/hd1080/srcAlt")
>> +    end
>> +    if resolution >= 1080 then
>> +        return lookup_keys(clip, 
>> "versions/enus/sizes/hd1080/srcAlt")
>> +    end
>> +    if resolution >= 720 then
>> +        return lookup_keys(clip, "versions/enus/sizes/hd720/srcAlt")
>> +    end
>> +    return lookup_keys(clip, "versions/enus/sizes/sd/srcAlt")
>> +end
>
> Not very flexible and future-proof, but okay. You can at least `or` 
> the
> -1 and 1080 conditions.

Indeed but I could not come up with a more flexible one for now.

>
> But really, anything that fixes it into something working is great,
> thanks!
>
> -- 
> Pierre Ynard
> "Une âme dans un corps, c'est comme un dessin sur une feuille de 
> papier."

I will prepare a new version of the patch, thanks for the review!