web-settings-and-get_iplayer-defaults-harmonisation

[dinkypumpkin]

On 30/01/2013 22:39, Pete Beardmore wrote:
> #patch attached (hopefully!)

I reviewed this patch when it was submitted, but found some minor 
problems and put it aside in my "good intentions to follow up" folder. 
Jon's post prompted me to dig it out again.

First, some responses to the original post:

> having tried combinations of 'flashhd,flashhd1,flashhd2' in option files
> and in the webserver to no avail, i settled on 'best' (having finally
> resorted to reading instructions!). this was set in my options files
> under /srv/www/iplayer/.get_iplayer but still, via the web server (be it
> my apache or the included standalone), i could not get the underlying
> flvstreamer/rtmpdump processes to behave and show me that attempts at
> flashhd1 etc. quality streams were being made.

Any option set in the web pvr (and modes is set by default) will 
override the setting in the options file, just as command line options 
do.  In the absence of any information about what you were trying to do, 
I suspect that could have been the problem.  If you're having troubles 
like this, post your output here and we'll try to help.

> so mistake #1. although the instruction state that one should ensure
> HOME is set correctly, don't assume that that is for reading any options
> files which you might have created there
> (/srv/www/iplayer/get_iplayer/options) whilst trying out the
> 'get_iplayer' script. the web server doesn't read them!

That is simply wrong.  Don't extrapolate from a single data point to 
assert it doesn't work in general.  Besides, your patch would be useless 
for CGI deployments if the options file isn't read.  Without knowing how 
you configured your server I can only guess, but it appears you were 
missing a ".".  Normally, the path should be ".get_iplayer/options" 
inside the web server's HOME.

> mistake #2. ALWAYS use the --prefs-add fuctionality to add options to
> the options file to ensure you get the right format! for example, the
> entry..
>
> modes 'best,flashhd,flashhd1,flashhd2,flashvhigh'
>
> ..f@&Sed me over as i'd add quotes!

Again, we're here to help with these issues.  In most cases there is no 
reason web pvr users should be expected to use --prefs-add from the 
command line.  It kind of defeats the purpose of using the web pvr.

> 1. 20+ empty default is bad, particularly when i have to scroll through
> them all to find non get_iplayer related cookies on my (apache) server

Bad?  Its a trivial amount of data that very few users will ever look 
at.  There's no harm in whittling it down though.

> 2. i wanted to delete all get_iplayer related cookies and go back to the
> default set of columns

It's slightly misleading to refer to the reset values as "defaults". 
They're actually whatever is in your options file, plus get_iplayer's 
built-in defaults for any unset options.

> 3. i've made the effort to add 'excludecategory' / 'exlcudechannel'
> options in my options file, i don't want to change those is multiple
> places. and if i've set something as important as 'modes' for use with
> 'get_iplayer' then i really want that same setting used in the web front end

For any web pvr users reading this:  This patch doesn't make any 
functional change in how the options file is used with the web pvr.  The 
settings in the options file are always used by get_iplayer, even when 
it is invoked through the web pvr, subject to any overriden settings 
specified in the web pvr.  Cookie changes aside, what this patch does do 
is make settings from the options file *visible* by default in the web 
pvr, which seems like a good idea. Of course, that presupposes that 
there are settings to make visible.  I suspect most web pvr users don't 
bother with the options file.  This patch would will be most useful to 
people who mainly use the CLI (and options file) but use the web pvr 
sometimes as well.


Now to specifics:

1. This bit:

@@ -3362,10 +3364,10 @@ sub get_display_cols {
  	my %cols_status;

  	# Add some default headings for history mode
-	push @headings_default, 'mode' if $opt->{HISTORY}->{current};
+	push split(/,/, $opt_defaults{cols}), 'mode' if 
$opt->{HISTORY}->{current};

does not compile in perl < 5.14, and produces a runtime error in all 
versions.  I think the bug unfortunately slips through compilation with 
perl >= 5.14 because of the implicit array dereferencing added in perl 
5.14.  get_iplayer should be able to maintain perl 5.8 compatibility, 
though I suspect there aren't a whole lot of users with perl < 5.10.

Of more concern is this means you didn't test your patch with the 
Windows distribution of get_iplayer, based on perl 5.12 at present. 
That's a no-no for changes to the web pvr.   If list traffic is any 
gauge, Windows is where the most users favour it.  Not every Windows 
user employs the provided installer, but I expect most do. If you can't 
or won't test on Windows, let us know up front.

2. You shouldn't be hard-coding default values for metadata, subtitles, 
and thumbnail.  Any non-default setting (and by that I mean built-in 
defaults) should be a positive choice by the user.  Don't make people 
download things they may not want by default.  Hard-coding those options 
also makes it impossible to use those settings from the options file, 
which is the ostensible purpose of this patch.

I recognise that the "modes" option requires a hard-coded default 
because of how the web pvr works.  However, making "best" the default 
value is a bad idea in my book.  Nobody - especially a new user - is 
going to thank you for eating their time and bandwidth with HD downloads 
they didn't ask for.

3. You may not want to expose the configured value for your output path 
in the web pvr.  I'm thinking of the (admittedly rare) case where you 
have a CGI deployment of the web pvr with any sort of public exposure.

4. It would be a nice-to-have feature if the reset function actually 
invoked get_iplayer to re-read the options file.  That way you don't 
have to restart the web pvr server to make changes to it.  Admittedly, 
it's not something that would be used often, but it seems like a logical 
extension of the functionality.

5. If you resubmit, please do a check for any added extraneous 
whitespace (git diff --check) and remove it before creating the patch.

My overall take is that this patch is a solution in search of a problem. 
  However, it would be useful to some people, so if I get a working 
version that checks out, I'll commit it.