Proposed update to return ITV Cache Functionality

[LFS1]

Description Two new features added

1) Re-enabled ITV PVR Functionality by independently creating the ITV.cache file needed by get-iplayer 
2) Created new display that allows the user to see new programmes that have been found on ITV & BBC. 

1-ITV PVR

New class in GetITVListings.m that is invoked by AppController in updateCache just before the regular get-iplayer index update occurs. GetITVListings runs as a separate thread alongside the get-iplayer update and a small progress indicator has been added to the right of the regular progress bar to show ITV progress.

GetITVListings (GIL) uses 2 files: itvprogrammes.gia and history.gia to create the itv.cache file that get-iplayer used to create. Neither file needs to exist for the first run and likewise they can be deleted at any time in which case they will automatically be re-built.

GIL operates by building a list of today's programmes from www.itv.com/shows, for each programme it can figure out latest version plus how many episodes are available. It then compares this to the programmes it found on the last run (which are loaded from itvlistings.gia) and determines which programmes have been added or have had episodes added, which programmes have remained unchanged and which programmes and or episodes have been deleted. For all programmes that are new or that have had multiple episode changed it also needs to load in the actual programme page from itv.com to get all of the individual episodes.

The above merge process then outputs the new itvprogrammes.gia file (in effect the carried forward file), the itv.cach file which can be used by get-iplayer for the rest of GIA and finally a new file, history.gia, is appended to with details of new programmes that were found (this file is input in to feature 2 above)

On first run GIL is quite slow as it has to visit the detailed episode page for every programme. However, once that first run is done, subsequent update are very quick as it is only need to figure out the delta since the last run.

There is a new menu item that has been added (Force ITV Cache) that forces a full re-load in case it gets out of wack

Final note; there is a 1 second sleep in between asking for each web page, I am not sure what the optimum value (if any) is to avoid hitting the ITV website too hard. seems to work fine with 1 second.

A few changes to the main nib file have been made to support the new progress display.

Main Files Changed AppController.h - new fields added AppController.m - As described above ExtendedShowInformationController - popup to advise not available for ITV programmes (couldn't get all of the data) typeArgumentForCacheUpdate - new arg 'includeITV' added determines whether the 'itv' argument is used as not needed when cache is being built (as now done by GIL) but needed all other times

Added GetITVListings.h GetITVListings.m itvplugin (put back)

2-New Programme Display

New display that can show all new programmes that have been found on a day by day basis. Software automatically culls it down to 3000 entries to keep it efficient.

Display is invoked from the main menu bar, which is greyed out while either the BBC index or the ITV index is being update. Thereafter available at all times.

It show a day by day account of new programmes that have been found; starting from today, and going backwards for about 1000 programmes. Filter is available by BBC and ITV. Also on the main settings screen under the BBC tab there is a list of all of the many BBC channels. This is a further filter (ie. on the main screen you can have BBC on or OFF; on the settings you can select at BBC channel level)

Data comes from the history.gia file mentioned above. There is also a new method at the end of AppController called updateBBCHistory called from AppCntrroller after both BBC and ITV indexes have been updated in getiPlayerUpdateFinished

This new code compares today's tv.cache file with yesterday's bbcprogrammes.gia file to figure out what is new and append it to the history.gia file. It then creates a new bbcprogrammes.gia file reflecting the current available programmes.

In the event that the bbcprogrammes.gia file does not exist, a new one is created and all programmes on that day are considered new.

Main files changes AppController - new UpdateBBCHistory method added plus code to invoke it

New Files ITVHistory.xib - new display ITVHistoryWidowController.m/h controller for new screen

Other Notes Had to add AppTransportSecuritySettings // AllowArbitoryLoads YES to main target properties to allow NSURL to work with none SSL links Also had to comment out the 2 scripts that are shown in get iplayer automator BUILD PHASES as i could not get it to build while they were enabled. Fixed a few very minor bugs affecting performance not functionality & also cleaned up a few un-used iboutlets

Have had on test for a while and seems to be working fine.

Thanks for your consideration

[dinkypumpkin]

Thanks. I can already see a few problems, but I'll wait until I have had a closer look before responding further

[dinkypumpkin]

Thanks again for the PR. Unfortunately, it cannot be accepted:

• You broke quick caching. If ITV caching is selected in preferences, the Quick Caching option is ignored and BBC caches are always updated by reindexing programme listings from the iPlayer site.
• Force ITV Cache Update triggers a BBC cache update as well. That shouldn't happen. This also falls foul of the quick caching problem above.
• The state of the Force ITV Cache Update menu item is not linked to the ITV caching option in preferences. It is stuck in whatever the state is at startup.
• Force ITV Cache Update creates duplicates of ITV programmes in the new programmes list.
• You broke the build. Those shell scripts are in the build phases for a reason. I know you have to disable code signing to work on the code, but it is not acceptable to submit broken code intentionally, even if you note it in the PR comments.
• You ignore radio programmes in your new programmes list. You should ignore podcasts, but not radio. Users should be able to filter new programmes from BBC TV and radio separately.
• You should create a separate preferences panel for the new programme channel filters. The filters have nothing to do with BBC recording formats, and there isn't room for TV and radio in the current location, anyway. Radio filters should be available for the national stations (incl. digital-only), with an additional choice for regional stations and another choice for local radio. The "Ignore News" option should be available for radio as well.
• The "timeadded" column in itv.cache should not be the air date, but the date the listing was added to cache. Those numbers should be integers as well - they are UNIX timestamps. The values in the "available" column - which is the air date - should be in ISO8601 format, as they are for BBC programmes. That also makes them sortable for loading the new programmes list. The old itv plugin should have done that, but it was abandoned long ago before being partially resuscitated by GiA.
• get_iplayer cache files should be opened as UTF-8. There aren't a lot of non-ASCII chars, but you should display them appropriately in the new programmes list.
• You said in the PR comments that the new programmes list limit is 3000, but your code uses 30000 as the limit. Presumably a typo, but only you know which one.

[LFS1]

Thanks for taking the time to do the review and provide this detailed feedback; much appreciated.

Tell me, if I can address the below concerns, will you accept a resubmission?

Regards

LFS

On Sep 4, 2016, at 11:25 AM, dinkypumpkin notifications@github.com wrote:

Thanks again for the PR. Unfortunately, it cannot be accepted:

You broke quick caching. If ITV caching is selected in preferences, the Quick Caching option is ignored and BBC caches are always updated by reindexing programme listings from the iPlayer site.

Force ITV Cache Update triggers a BBC cache update as well. That shouldn't happen. This also falls foul of the quick caching problem above.

The state of the Force ITV Cache Update menu item is not linked to the ITV caching option in preferences. It is stuck in whatever the state is at startup.

Force ITV Cache Update creates duplicates of ITV programmes in the new programmes list.

You broke the build. Those shell scripts are in the build phases for a reason. I know you have to disable code signing to work on the code, but it is not acceptable to submit broken code intentionally, even if you note it in the PR comments.

You ignore radio programmes in your new programmes list. You should ignore podcasts, but not radio. Users should be able to filter new programmes from BBC TV and radio separately.

You should create a separate preferences panel for the new programme channel filters. The filters have nothing to do with BBC recording formats, and there isn't room for TV and radio in the current location, anyway. Radio filters should be available for the national stations (incl. digital-only), with an additional choice for regional stations and another choice for local radio. The "Ignore News" option should be available for radio as well.

The "timeadded" column in itv.cache should not be the air date, but the date the listing was added to cache. Those numbers should be integers as well - they are UNIX timestamps. The values in the "available" column - which is the air date - should be in ISO8601 format, as they are for BBC programmes. That also makes them sortable for loading the new programmes list. The old itv plugin should have done that, but it was abandoned long ago before being partially resuscitated by GiA.

get_iplayer cache files should be opened as UTF-8. There aren't a lot of non-ASCII chars, but you should display them appropriately in the new programmes list.

You said in the PR comments that the new programmes list limit is 3000, but your code uses 30000 as the limit. Presumably a typo, but only you know for sure.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[dinkypumpkin]

Feel free to have another go. Use this PR and the same git branch. If you decide not to proceed, just close the PR.

[willson556]

@LFS1 My preference would be fixing up the ITV plugin for get-iplayer in order to allow quick caching on my server to work. It also seems like the most maintainable solution in the long run as it can re-use a lot of the existing get-iplayer code. Thoughts on that @dinkypumpkin?

EDIT: @LFS1 I just took a proper look at your PR and realized I misunderstood what you were doing: I see now that you did update the plugin. We could include all of your code to be able to make subsequent updates quicker but we could also just keep it simple and allow the majority of users to keep using quick caching. Quick caching was originally introduced to combat basically the same problem with 4oD when it was still around which was that building the cache took a very long time. As long as it takes less than a half hour to build the cache, I'm fine with just running it as a typical cache building operation in get_iplayer. I can then set it to run every hour or so on my server (BBC runs every 15 minutes IIRC). I suspect that almost every user of GiA is utilizing the quick caching feature already as it's the default and aside from occasional server outages, it works quite well.

[dinkypumpkin]

@willson556: You decide. I only care about ITV functionality to the extent that it isn't broken. I agree that re-jigging the ITV plugin would be the simpler solution, with fewer potential support headaches. OTOH, the chance of anyone but you doing that work is near zero. One bit of guidance if you go that route: use XML::LibXML for HTML parsing since it is installed with system Perl on OS X. Also keep in mind that full ITV cache updates via web scraping are very, very slow. If you don't create some kind of incremental update mechanism like @LFS1 implemented in this PR, you should add a note in the preferences tab that ITV updates will take a (relatively) long time without quick caching.

[dinkypumpkin]

@willson556: itv.plugin wasn't updated, but merely reinstated. It has to be present for get_iplayer search to work, but is otherwise useless in present form.

[LFS1]

On test; assuing that GIA is being run about one a day I am seeing cache update times in the 30 to 60 second region. A full cache reload takes 2 to 3 minutes. If I have understood you correctly @willson556, you are saying that you would run GIA on you local server at a reasonably frequent interval and then GIA could pull that from your server when operating in quick cache mode, otherwise it would just build the cache itself. If I have understood correctly & that's how you both advise I proceed, let me know and I'll modify accordingly. Also, if you could let me have a URL to test against. I have almost finished correcting all of the issues @dinkypumpkin pointed out so will soon be ready to go in either direction. Thanks

[willson556]

@dinkypumpkin Thanks for pointing that out!

@LFS1 I'm not entirely convinced that the complexity of the incremental download is worth it. For one, with a sleep time of 1ms instead of 1s, the cache update completed very quickly in my testing. And I'm not personally tremendously concerned about overloading ITV's servers as long as we deploy with quick caching as well.

Currently, my server is setup to simply run the get_iplayer script every 15 minutes with the profile directory to one accessible at http://tom-tech.com/get_iplayer/cache. I'm not opposed to the New Programmes part of the PR but I really do think that the indexing could/should be accomplished through a get_iplayer perl plugin.

And yes, the way indexing/caching should work is that if the user has Quick Caching enabled, the relevant cache files are simply downloaded from my server. Otherwise, GiA builds them locally using the get_iplayer script.

I do have some time to work on this project again so I don't mind implementing the ITV get_iplayer plugin myself.

[willson556]

As for the New Programmes functionality, I think users could find that useful so I would be willing to merge that part in if you decide not to take on the perl plugin.

[LFS1]

@willson556 & @dinkypumpkin; I have addressed all of the items identified in the original PR. Also I have modified the ITV listing code so that it is independent to the new programme display code and would now only need a few lines of code to be moved and added to make it use the Quick Cache feature if it is made available on the server. I have pushed everything up to the same branch but have not done a PR as want to leave it on test for a few days. Regrettably i have no knowledge of perl so i have nothing to offer in that respect. Thanks

[dinkypumpkin]

Thanks. I can already see a few minor issues, but I'll give it a more thorough look in the next few days.

[LFS1]

Have had it on test for the week and seems okay so I would put this forward for your review at this time. I look forward to getting your feedback. Thanks again.

[dinkypumpkin]

I'll need to run it a few more days to make sure the new programmes feature is doing what it should. It seems OK so far.

The radio filter preferences still need a little bit of work. I presume you are not in the UK, so some of this may not be obvious to you:

• This is radio, so we have "stations", not "channels"
• Use the proper station names. You said you used Wikipedia for regional station names, but you appear to have ignored that list when it came to national stations. All of the checkbox labels are wrong.
• Asian Network and World Service are both national stations, but only appear with local stations filter.
• Remove the "Show Digital Channels" option and give each national station a separate checkbox. There are only 11, so there is enough space. All national stations are available via DAB anyway, so analog/digital distinction isn't really meaningful. Also, the way you implemented the digital stations filter (e.g., Radio 4 Extra won't appear unless Radio 4 is selected) won't make sense to BBC radio listeners. That might just be a bug, but still - get rid of this option altogether.

The earlier problems with ITV cache updating appear to have been fixed. A few very minor issues remain:

• In AppController.updateCache, remove the line where you set the progress display to "Updating BBC Indexes...". You don't know that any BBC updates will occur at that point.
• In AppController.itvUpdateFinished, set didUpdate = YES. Otherwise you can get a spurious "AppController: Index was Up-To-Date" log message generated in getiPlayerUpdateFinished if only ITV caching is configured (or forced).
• Change the "Force ITV Cache Update" menu item label to "Force ITV Cache Reset". That will match your warning dialog and is a better description of what happens when menu item is invoked.

After the above changes are complete, stop work on this PR. If @willson556 makes good on his intention to revive itv.plugin, your ITV caching code won't be used. It's not worth wasting any more of your time on ITV caching until that issue is decided. As it stands, I still have a lot of surgery to perform to clean up the branch and separate the code so I can merge new programmes functionality. You will eventually need to do a forced push onto this PR branch in order to reset it to contain only ITV caching code, so there is no point in adding anything beyond the items above for now.

[LFS1]

Thanks for the code review and feedback. I'll proceed as advised and let you know once I've completed your changes.

On Sep 24, 2016, at 6:04 PM, dinkypumpkin notifications@github.com wrote:

I'll need to run it a few more days to make sure the new programmes feature is doing what it should. It seems OK so far.

The radio filter preferences still need a little bit of work. I presume you are not in the UK, so some of this may not be obvious to you:

This is radio, so we have "stations", not "channels"

Use the proper station names. You said you used Wikipedia for regional station names, but you appear to have ignored that list when it came to national stations. All of the checkbox labels are wrong.

Asian Network and World Service are both national stations, but only appear with local stations filter.

Remove the "Show Digital Channels" option and give each national station a separate checkbox. There are only 11, so there is enough space. All national stations are available via DAB anyway, so analog/digital distinction isn't really meaningful. Also, the way you implemented the digital stations filter (e.g., Radio 4 Extra won't appear unless Radio 4 is selected) won't make sense to BBC radio listeners. That might just be a bug, but still - git rid of this option altogether.

The earlier problems with ITV cache updating appear to have been fixed. A few very minor issues remain:

In AppController.updateCache, remove the line where you set the progress display to "Updating BBC Indexes...". You don't know that any BBC updates will occur at that point.

In AppController.itvUpdateFinished, set didUpdate = YES. Otherwise you can get a spurious "AppController: Index was Up-To-Date" log message generated in getiPlayerUpdateFinished if only ITV caching is configured (or forced).

Change the "Force ITV Cache Update" menu item label to "Force ITV Cache Reset". That will match your warning dialog and is a better description of what happens when menu item is invoked.

After the above changes are complete, stop work on this PR. If @willson556 makes good on his intention to revive itv.plugin, your ITV caching code won't be used. It's not worth wasting any more of our time on ITV caching until that issue is decided. As it stands, I still have a lot of surgery to perform to clean up the branch and separate the code so I can merge new programmes functionality. You will eventually need to do a forced push onto this PR branch in order to reset it to contain only ITV caching code, so there is no point in adding anything beyond the items above for now.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[LFS1]

All requested changes have been made; I have also updated the doco file point by point against your original list.

As requested, I will leave the ITV stuff alone for the time being.

Thanks again

[dinkypumpkin]

Thanks. I'll check it over later this week. However, it appears that this PR may have been overtaken by events: #601. If the old ITV feeds are still working in a week's time I'm going to revert my earlier removal of the old ITV caching code, but I'll squash into a single commit so you can easily remove again if necessary. Your approach - web scraping - is more fragile, but likely to be more adaptable. It's all or nothing with the old feeds, so perhaps not wise to bet the farm on them again. Again, that will be for you and @willson556 to sort out.

[LFS1]

Well who would have thought, after all this time, we've gone from nothing to 3 options! I'll wait to here back and of course happy to help in any way I can.

On Sep 26, 2016, at 11:33 AM, dinkypumpkin notifications@github.com wrote:

Thanks. I'll check it over later this week. However, it appears that this PR may have been overtaken by events: #601. If the old ITV feeds are still working in a week's time I'm going to revert my earlier removal of the old ITV caching code, but I'll squash into a single commit so you can easily remove again if necessary. Your approach - web scraping - is more fragile, but likely to be more adaptable. It's all or nothing with the old feeds, so perhaps not wise to bet the farm on them again. Again, that will be for you and @willson556 to sort out.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[dinkypumpkin]

Thanks. Everything seems to be working OK now, though you need to do a better job of proofreading. But don't make any more changes - I'll take care of the remaining items. I'll post again on this PR when I have reworked the code.

[dinkypumpkin]

This PR has now been spliced into master and I've posted a couple of pre-release builds: one with the old ITV indexing restored and one with features from this PR added. There is no ITV quick cache file being produced, so the old method is noticeably slower for now.

I made a number of changes, the principal one being that the ITV indexing method from this PR is now an optional feature that can be enabled from the Advanced tab in preferences. If the old ITV data feed goes AWOL again, everybody can just flip a switch and keep going, and we can hopefully avoid this rigamarole again.

If you find any bugs in the new code, post fixes in a fresh PR using a branch based on current master. Thanks again for the PR.