Alternative 'simplified' approach

[metaron-uk]

[janbar]

@metaron-uk , i added new commit with one of your fix and one more to keep track of epg based. All epg based have search type NoSearch as required in mythtv sources. I will check if now we can allow to update channel of an existing rule without breaking rule. That avoid to create an other rule as you requested. Also this feature exists already for rule "Record one" or "Record all" using overriding as i explain above, but if it runs why not.

[janbar]

The code is very tricky because we have to check the reversibility on each change made in functions FillTimerEntry / NewFromTimer. Else the big bug occurs and with an update you could break the recording rule. I don't want to be here when a user will break his stuff ... ;)

[janbar]

I am thinking to code a test tools to check reversibility ...

[metaron-uk]

@janbar - Some thoughts based on your comments:

1. Sorry for re-ordering SetCallsign and friends (newbie mistake). I can see making changes like this makes reviewing diffs hard. I just couldn't stop myself as the order was different for each timer type and sometimes there were duplicate calls to SetTitle. I will try harder to resist the temptation in future 😊 .
2. Regarding reversibility of 'MythVersionHelper::NewFromTimer'.
   Double abstraction of a PVR_TIMER via MythTimerEntry to MythRecordingRule makes it easier to deal with PVR_TIMER API changes, but causes a problem when both
   MythScheduleManager::UpdateTimer(const MythTimerEntry& entry) and
   MythScheduleManager::SubmitTimer(const MythTimerEntry& entry) call the same
   MythScheduleHelperXX::NewFromTimer(const MythTimerEntry& entry, bool withTemplate) function.
   Currently a MythTimerEntry doesn't distinguish between a new entry and an edited one.
   As an EPG based MythTimerEntry is guaranteed to have epgInfo only when first created, seriesid and programid etc... won't necessarily be available when edited later.
   Rather than make sure the process is reversible, maybe 'MythScheduleManager::UpdateTimer' should find the old MythRecordingRule, then call a new function:
   MythRecordingRule MythVersionHelper::UpdateRuleFromTimer(const MythTimerEntry& entry, const MythRecordingRule& oldrule) which returns a rule changed only in ways the UI supports for each Timer Type. This can then be passed to MythScheduleManager::UpdateRecordingRule.
3. I am starting to understand 'weekly'/'daily' rules. I know my code didn't implement a working solution for these. I will try not to waste your valuable time reviewing my hacks-in-progress in future 😊 .
4. I didn't realize 'this channel' only appeared in 0.27. That makes more sense.
5. I feel strongly that 'free text search' should have separate timer types from 'EPG guide selection' or 'Time & Channel Based' manual rules. I think trying to squeeze all three categories into one timer type makes the UI cluttered and confusing and doesn't match Mythtv / Mythweb philosophy anyhow.
6. Converting from an EPG to a manual type is easy, but the information to convert the other way is not necessarily available. This makes me think that there are up to 12 potential Timer Types (4*3)+Live TV to be supported:
   • Once Manual -- For 'Add Timer...'
   • Daily Manual -- For 'Add Timer...'
   • Weekly Manual -- For 'Add Timer...'
   • All Manual -- Not needed?
   • Once EPG -- For Films
   • Daily EPG -- For daily 'weather'
   • Weekly EPG -- For weekly 'current affairs'
   • All EPG -- For series/box sets
   • Once Freetext -- Not needed?
   • Daily Freetext -- Not needed?
   • Weekly Freetext -- Not needed?
   • All Freetext -- For 'Add Timer...' or partially pre-populated from EPG data.
   • This Showing -- For live TV 'record now' & possibly 'EPG record selection'
7. Probably this can be reduced to the following 9 (plus the upcoming and unhandled types):
   • Once Manual -- For 'Add Timer...'
   • Daily Manual -- For 'Add Timer...'
   • Weekly Manual -- For 'Add Timer...'
   • Once EPG -- For Films
   • Daily EPG -- For daily 'weather'
   • Weekly EPG -- For weekly 'current affairs'
   • All EPG -- For series/box sets
   • All Freetext -- For 'Add Timer...' or partially pre-populated from EPG data.
   • This Showing -- For live TV 'record now' & possibly 'EPG record selection'
8. Down-selection of an EPG type to a manual Once/Daily/Weekly type using the 'Any Time' radio button ought to be possible, but up-selection of a manual type without 'Subtitle' 'Description' 'ProgramID' 'SeriesID' etc... being populated probably won't work. As the Kodi UI doesn't present this to the user (currently), maybe a lack of this information in the backend rule could be used to distinguish between types?
9. I obviously have strong opinions and I want to help, but know this is your project at the end of the day. I'm available to help for a month or so on a semi-full-time basis, but then I'll be back to the day job and only able to dip in occasionally. Let me know in what form that help would be most welcome. 😀

[janbar]

I think we have probably same idea or resolution at same time. Probably because we are working on this feature just born. Your opinion and your work is very important and i take care of them. Until now you are the one of two guy to propose improvement. Thanks for that. Now i am trying to stabilize the big changes we have made (i cherry picked several of your changes) and i would like to build a test tool to validate the reversibility before merging more change. Also i would like you push a PR containing only the add for new timer TEXT_SEARH based on my last commit.

[janbar]

Until commit 8b0f1a1 , i check the validity of epg info for rule requires one (because they have it on create). The objective is to never check reversibility in NewFromTimer or FillTimerEntry, but only in the caller (here UpdateRecordingRule). Like that it allows to implement only tranformation rules in these functions and nothing else, to make to code as clear/clean/readable/testable as possible.

[janbar]

So you could push one PR with commit to add new timer TEXT_SEARCH. You can take as sample the commit 979aec4. Like that the new improvement can easily be tested without breaking others. Myself I am afraid to break all ;)

[janbar]

Finally your solution MythVersionHelper::UpdateRuleFromTimer could be the best. But it requires to maintain a third method. If we cannot avoid it then we will create it.

[metaron-uk]

Maybe MythRecordingRule MythVersionHelperXX::GetRuleFromTimer(const MythTimerEntry& entry, const MythRecordingRule& oldrule, bool withTemplate) would be a more elegant solution.
If oldrule is null, it knows to create a new one. If not, it knows to modify it based on the MythTimerEntry.
That way all the code to convert MythTimerEntry->MythRecordingRule stays in one place (per helper version) and is therefore easier to maintain.

One PR for TEXT_SEARCH rule coming up. 😃

Once this is done I will look at possible solutions for fitting Autoexpire / Record new+expire old / Number of episodes to keep into the "Lifetime" list. (I had some ideas when discussing this with Ksooo originally).
If I spot any bugs while I'm about it I'll let you know.
(I've just found out about "git add -p" so hopefully my commits won't be such a mess in future!)