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

Jump to bottom

[PVR] Support for Kodi Eventlog #8374

Merged
merged 2 commits into from Dec 6, 2015
Merged

[PVR] Support for Kodi Eventlog #8374

merged 2 commits into from Dec 6, 2015

Conversation

ksooo
Copy link
Member

@ksooo ksooo commented Nov 11, 2015

This PR extends Kodi eventlog (Jarvis feature) with some PVR events, namely

  • create/delete timers
  • start/stop recordings.

Ideas for other events are welcome.

@da-anda @Jalle19 mind taking a look

@ksooo ksooo added RFC PR submitted for gathering feedback Type: Improvement non-breaking change which improves existing functionality Component: PVR v16 Jarvis labels Nov 11, 2015
@ksooo ksooo added this to the Jarvis 16.0-beta1 milestone Nov 11, 2015
AlwinEsch
AlwinEsch reviewed Nov 11, 2015
View reviewed changes
xbmc/pvr/addons/PVRClients.h
* @return True if the client was found, false otherwise.
*/
bool GetClientName(int iClientId, std::string &strName) const;
bool GetClientName(int iClientId, std::string &strName, bool bFriendly = true) const;

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@FernetMenta
Copy link
Contributor

if I am not mistaken, the decision was that only fixes make it to Jarvis. All other have to wait for 17.0
we intend to branch 17.0 soon

@ksooo
Copy link
Member Author

ksooo commented Nov 11, 2015

@MartijnKaijser feel free to retarget thi PR, just thought that the new eventlog feature could need some love... At least my eventlog is almost always empty (except "Kodi sucessfully started") and therefore adds not too much value.

@MartijnKaijser
Copy link
Member

iirc we would still allow minor non-API "feature" changes during beta

@FernetMenta
Copy link
Contributor

I don't consider use of activity log as minor. This can have a big impact. It's not a self contained little feature

@ksooo
Copy link
Member Author

ksooo commented Nov 11, 2015

Event log is in master for quite some time now and has proven to work. I do not see any risk in writing some more log entries. We're not talking about mass data or use of untested functionality here.

@da-anda
Copy link
Member

da-anda commented Nov 11, 2015

some backends do really weird things with timer notifications if the backend has been away for a second and flood the user with tons of "timer removed", and "timer added" spam. Could be confusing to have that in the activity log, but the change is nice in general, thanks.

@FernetMenta
Copy link
Contributor

When you send an event, it makes use of several globals like window manager, thread messages, etc. Estimating risk is not easy. Even backports to Isengard have broken things though risk was considered low.
You just voted against this: #8270 which wasn't a fix either. In order to make decisions simple and avoid conflict I'd say ONLY fixes are allowed.

@Jalle19
Copy link
Member

Jalle19 commented Nov 11, 2015

@da-anda sadly tvheadend is one of those :-)

@ksooo
Copy link
Member Author

ksooo commented Nov 11, 2015

I voted against #8270 because it contained lots of changed code (compared to this PR) without any functional change. This PR is different - few code changes, but end-user relevant functional enhancements.

Currently, we're working towards first beta, but it is not yet released. In this phase smaller improvements are imo still okay. Once beta 1 is out, work mode should change and only fixes should be allowed. A beta is supposed not be functionally perfect, but feature complete - otherwise it would be a RC. This is how other projects/products I know work and this makes sense for me.

But as I said, I can live with not having this PR in Jarvis. I suggest let @MartijnKaijser have the final word on this.

@FernetMenta
Copy link
Contributor

A beta is supposed not be functionally perfect, but feature complete

an app like kodi is never feature complete. and the more we move to RERO this is even less important.

Note that for PVR related changes special care is required. You only tested this with a single backend (tvh), right? Did you test adding new timer on the backend and trigger update while playing video? I am sure there is a whole bunch of untested scenarios related to this change.

@ksooo
Copy link
Member Author

ksooo commented Nov 11, 2015

If you put it this way I'm of course doomed.

But maybe it was just dumb to make a difference between functional and non functional changes and to vote against a non functional change PR while still in alpha phase. Maybe this was a mistake but exactly this made myself pointless for this discussion I guess. :-(

Anyway, meanwhile my point of few is that there should be no early and late alpha phase wrt different rules for PR content. THIS would make things difficult to handle. Rules should change for different release cycle stages only - not while in a single phase.

@ksooo ksooo removed this from the Jarvis 16.0-beta1 milestone Nov 11, 2015
@ksooo ksooo added Type: Feature non-breaking change which adds functionality and removed Type: Improvement non-breaking change which improves existing functionality labels Nov 11, 2015
@FernetMenta
Copy link
Contributor

I thought we are in beta already. Am I wrong?

@ksooo
Copy link
Member Author

ksooo commented Nov 11, 2015

We released alpha 4 and working towards releasing beta 1, which is due Nov 15. ;-)

I thought this means we are still in alpha, but...

@FernetMenta
Copy link
Contributor

the log says BETA1, hence the current code is beta.

@ksooo
Copy link
Member Author

ksooo commented Nov 11, 2015

which log?

@FernetMenta
Copy link
Contributor

kodi.log
Starting Kodi (16.0-BETA1 Git:2015-11-10-7914216-dirty). Platform: OS X x86 64-bit

This means that we kind of release beta to the public Nov, 14th

@FernetMenta
Copy link
Contributor

this is what @MartijnKaijser sent out to all

Current Alpha feature window runs to 20 Oktober
Alpha 4 release on 1 November
After we'll start the beta windows for fixes only.

@MartijnKaijser
Copy link
Member

I have to agree with @FernetMenta here. It's certainly a nice feature however it's not even really used yet by other kodi parts. Having pvr start using this actively with variety of backends makes it a potential time bomb. Code base has improved massively over the past years but even 15 has proven that a tiny fix in pvr or skinning has unwanted side affects. As such caution is needed until we have a better grasp on it.
Indeed it's already in beta as well with target at 14th.

@ksooo
Copy link
Member Author

ksooo commented Nov 11, 2015

Okay, no problem with that - as I wrote earlier. All good. ;-)

@xhaggi
Copy link
Member

xhaggi commented Nov 13, 2015

@ksooo thanks

@MartijnKaijser MartijnKaijser modified the milestone: K***** 17.0-alpha1 Dec 4, 2015
@MartijnKaijser
Copy link
Member

jenkins build this please

MartijnKaijser added a commit that referenced this pull request Dec 6, 2015
@MartijnKaijser
Merge pull request #8374 from ksooo/pvr-eventlog
add4f65 
[PVR] Support for Kodi Eventlog
@MartijnKaijser MartijnKaijser merged commit add4f65 into xbmc:master Dec 6, 2015
@ksooo ksooo removed the RFC PR submitted for gathering feedback label Dec 7, 2015
@ksooo ksooo deleted the pvr-eventlog branch December 7, 2015 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Component: PVR Type: Feature non-breaking change which adds functionality v17 Krypton
Projects
None yet
Milestone
Krypton 17.0-alpha1
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@ksooo @FernetMenta @MartijnKaijser @da-anda @Jalle19 @xhaggi @AlwinEsch