Opened 8 years ago
Closed 8 years ago
Last modified 6 years ago
#12720 closed Patch - Feature (Fixed)
PR for Services API Adding SetSavedBookmark and GetSavedBookmark to Dvr Services PR #117
Reported by: | Owned by: | Bill Meek | |
---|---|---|---|
Priority: | minor | Milestone: | 0.28.1 |
Component: | MythTV - Services API - Backend | Version: | Master Head |
Severity: | medium | Keywords: | |
Cc: | Ticket locked: | no |
Description
https://github.com/MythTV/mythtv/pull/117
This PR adds two functions GetSavedBookmark? and SetSavedBookmark? they were done largely in the style of #12090. I thought about adding the ability to pull the full markup (in fact I am not sure the other GetRecordedCutList? shouldn't do this and let the user sort the types out) but this still left how best to set a bookmark without adding a more complex markup setting function. I think this simplified version of just Get/Set? bookmark adds the remaining missing hunk to the services api for markup, no it doesn't add the metadata of #11491 but that probably should come from elsewhere. Both functions take very similar args to #12090. I did refactor the QueryKeyFramePosition? and QueryKeyFrameDuration? main function code into one function, as I was adding two more (QueryPositionKeyFrame?,QueryDurationKeyFrame?) and they were nearly identical I figured I would slim down the code. If you prefer 4 functions with duplicate code however I can update the code for that.
Attachments (1)
Change History (8)
comment:1 Changed 8 years ago by
Milestone: | unknown → 0.29 |
---|
comment:2 Changed 8 years ago by
Milestone: | 0.29 → 0.28.1 |
---|---|
Owner: | set to Bill Meek |
Status: | new → assigned |
Changed 8 years ago by
Attachment: | 12720-changes.patch added |
---|
comment:3 Changed 8 years ago by
Mitch, this looks great. The attached changes apply and build OK on my system (they should, pretty minor.) Pls apply to your PR.
comment:4 Changed 8 years ago by
Sorry I didn't get any notification about the ticket update. I have rebased and applied the formatting change.
comment:5 Changed 8 years ago by
Looks good to me. Can someone do the PR, I don't believe I've got permissions. At least it doesn't give me the option to merge.
https://github.com/MythTV/mythtv/pull/117
Thanks
comment:6 Changed 8 years ago by
Resolution: | → Fixed |
---|---|
Status: | assigned → closed |
Fixed in master c4831026eb157c3ede860a12e7c86fd0bd14b9ee/mythtv and fixes/0.28.0 57c1afbf7c498ac3b09d782ce3f3b76382b64fd7/mythtv
comment:7 Changed 6 years ago by
Owner: | changed from Bill Meek to Bill Meek |
---|
Mostly whitespace/braces/80 column changes, test for - offset. Version number/lowercase param args need authoritative answer