Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add PVR_TIMER_TYPE_SUPPORTS_READONLY_DELETE flag #14507

Merged
merged 1 commit into from Oct 3, 2018

Conversation

@djp952
Copy link
Contributor

commented Oct 2, 2018

This PR adds a new PVR flag, PVR_TIMER_TYPE_SUPPORTS_READONLY_DELETE, and updates the PVR API to 5.10.3 (compatible with 5.10.0)

Description

The new flag is intended to allow PVRs to specify timer types that are generally read-only in nature but still allow deletion through Kodi. The existing PVR_TIMER_TYPE_IS_READONLY flag does not allow deletion when specified as a PVR timer type flag.

Motivation and Context

Admittedly this is a niche case that may only benefit my PVR client addon (https://github.com/djp952/pvr.hdhomerundvr). The motivation is that timers/timer rules may be generally read-only in nature in that the user cannot modify the parameters of the timer (rule), but that also excludes them from deleting the timer (rule) in the existing implementation. This proposed change adds a flag that indicates a read-only timer can still be deleted by the user.

How Has This Been Tested?

This has been tested using my own PVR client (https://github.com/djp952/pvr.hdhomerundvr) for the HDHomeRun DVR system, on Windows 10 x64 using the latest XBMC code available at the time of the PR. I set breakpoints at each place the existing PVRTimerType::IsReadOnly() is invoked and made a determination if my proposed function PVRTimerType::SupportsReadOnlyDelete() should be invoked to negate that result. I only found two places where this would be relevant - when deciding if the "Delete" option should be shown, and again when the Delete Timer operation is invoked.

To unit test the change, I incremented the PVR API to version 5.10.3 and added a handful of MANUAL timers via my PVR. I confirmed that I did not have to increment the PVR API version in my addon.xml file, and stepped through the aforementioned breakpoints. The Kodi UI presented me with Delete option(s) that were not available prior to the change, and they worked as expected. I also confirmed that the Edit option(s) that were now presented display a read-only view of the Timer (Rule) as expected.

It may go without saying, but I recommend ksooo review this PR and decide if it's too much of a niche case for inclusion or if there is a more ambitious plan in the works for future versions that would negate this change or otherwise render it useless.

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • None of the above (please explain below)

Checklist:

  • My code follows the Code Guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the Contributing document
  • I have added tests to cover my change
  • All new and existing tests passed
@ksooo
Copy link
Member

left a comment

just minors to change. functional changes look good.

@@ -497,7 +497,7 @@ namespace PVR
if (timer && (!item.GetEPGInfoTag() || !URIUtils::PathEquals(item.GetPath(), CPVRTimersPath::PATH_ADDTIMER)) && !timer->IsRecording())
{
const CPVRTimerTypePtr timerType(timer->GetTimerType());
return timerType && !timerType->IsReadOnly();
return timerType && (!timerType->IsReadOnly() || timerType->SupportsReadOnlyDelete());

This comment has been minimized.

Copy link
@ksooo

ksooo Oct 2, 2018

Member

wrong indentation

This comment has been minimized.

Copy link
@garbear

garbear Oct 2, 2018

Member

This comparison is used twice. Can you factor it out? Also deMorgan would be unhappy that you duplicated the logic but made it unclear that you had done so.

@@ -781,7 +781,7 @@ namespace PVR
return false;
}
}
else if (timer->HasTimerType() && timer->GetTimerType()->IsReadOnly())
else if (timer->HasTimerType() && (timer->GetTimerType()->IsReadOnly() && !timer->GetTimerType()->SupportsReadOnlyDelete()))

This comment has been minimized.

Copy link
@ksooo

ksooo Oct 2, 2018

Member

remove the additions brackets. they are not needed.

@ksooo

This comment has been minimized.

Copy link
Member

commented Oct 2, 2018

Note: As this is a non-breaking API change it's okay to include it for Leia.

@djp952

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2018

Requested changes made and squashed. Thank you for considering this change for inclusion, it helps me out more than one might expect!

@ksooo

This comment has been minimized.

Copy link
Member

commented Oct 2, 2018

This comparison is used twice. Can you factor it out? Also deMorgan would be unhappy that you duplicated the logic but made it unclear that you had done so.

@garbear is right here. Maybe you want to factor out

!timerType->IsReadOnly() || timerType->SupportsReadOnlyDelete()

to a new member function

bool CPVRTimerType::CanBeDeleted() const
@ksooo

This comment has been minimized.

Copy link
Member

commented Oct 2, 2018

it helps me out more than one might expect!

I would be happy if you could explain this a little bit more.

@djp952

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2018

I see what you guys want there, will take care of that tonight. Do you want just CanDelete() or also supplement with things like CanEdit()? I think just CanDelete() is the least invasive approach, I'd hate to miss something and break stuff on you.

As far as why this helps me out (TLDR warning), in my PVR it's difficult to map Timers and Timer Rules to the HDHomeRun DVR methodology, which is just a set of 'Recording Rules'. The rules themselves can be edited/deleted/modified, of course, but breaking the different kinds up into both Timers and Timer Rules got weird. An HDHR "Record Series" operation maps well into PVR_TIMER_TYPE_IS_REPEATING, but the HDHR "Record Once" operation doesn't. To make the UI work for the users as consistently as I could, I also define "Record Once" as PVR_TIMER_TYPE_IS_REPEATING, which separates the rule and the individual timer it creates out like the series rules. (The timers themselves are generated from a different backend search that flags EPG entries as having a recording rule). The catch for me is that the "Record Once" stuff effectively works out such that it doesn't make much sense to allow them to be edited, but you can of course delete them. With the existing PVR API, the user would need to go into Timers rather than Timer Rules to delete them, since the parent Timer Rule needs to be flagged as read-only and wouldn't allow deletion from Timer Rules. This proposed change allows the Timer Rule itself to be deleted without allowing editing.

Ultimately this just allows me to present a more consistent set of hacks for the users, they can work the same way for both types of rules I am able to support. If they want to delete a Recording Rule, they can always do it via Timer Rules rather than needing to know what kind of rule it was.

In reality with the way the HDHR DVR methodology works it's usually easier to just let it record what it wants and delete it after the fact. It's a strange beast.

@ksooo

This comment has been minimized.

Copy link
Member

commented Oct 2, 2018

CanDelete is sufficient, no need for CanEdit

@ksooo

This comment has been minimized.

Copy link
Member

commented Oct 2, 2018

And thanks for explaining your motivation for this change.

@djp952

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2018

Refactored to include a CanDelete method and used that where appropriate. LMK if you guys think "AllowsDelete" might be a better name to match with the existing Forbids/Requires member function names.

@ksooo

This comment has been minimized.

Copy link
Member

commented Oct 3, 2018

Yeah, AllowsDeleteis better.

Add PVR_TIMER_TYPE_SUPPORTS_READONLY_DELETE flag to allow an otherwis…
…e read-only timer to be deleted via Kodi
@djp952

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2018

Method renamed to AllowsDelete. Thanks!

@MartijnKaijser MartijnKaijser added this to the Leia 18.0-beta4 milestone Oct 3, 2018

@ksooo
ksooo approved these changes Oct 3, 2018
Copy link
Member

left a comment

Thanks.

@ksooo ksooo merged commit a42e9d5 into xbmc:master Oct 3, 2018

1 check was pending

default Found some time, building it now.
Details
@Rechi

This comment has been minimized.

Copy link
Member

commented Oct 3, 2018

@ksooo why have you merged the PR before the jenkins build has finished?

@ksooo

This comment has been minimized.

Copy link
Member

commented Oct 3, 2018

Because I have not seen that it was not finished.

@ksooo

This comment has been minimized.

Copy link
Member

commented Oct 3, 2018

I feel like we have a Big Brother here. ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.