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

Avoid incorrect disabling PVR addons (Alternate) #4761

Merged
merged 5 commits into from May 26, 2014
Merged

Avoid incorrect disabling PVR addons (Alternate) #4761

merged 5 commits into from May 26, 2014

Conversation

t-nelson
Copy link
Contributor

Alternate to #4757.

Trent Nelson added 3 commits May 21, 2014 11:37
AddonDB is not a reliable source for testing existence of an addon, use
AddonManager instead and tag clients as "known" in PVRDB.
@t-nelson
Copy link
Contributor Author

Heh...WTF is github up to now...

@t-nelson t-nelson changed the title Avoid incorrect disabling PVR addons Avoid incorrect disabling PVR addons (Alternate) May 21, 2014
@t-nelson
Copy link
Contributor Author

Ah, wrong branch. Sorry for the noise.

ie. Fixed now.

@opdenkamp
Copy link
Member

did you test this with a clean installation (removed ~/.xbmc before running)?
PVR add-ons need to be configured, or disabled by default when not configured.

@t-nelson
Copy link
Contributor Author

@opdenkamp Nope, top two should address.

I'll do some squashing before push.

@t-nelson
Copy link
Contributor Author

jenkins build this please

@MartijnKaijser MartijnKaijser added this to the Pending for inclusion milestone May 21, 2014
@t-nelson
Copy link
Contributor Author

@opdenkamp Did you give this one the OK on IRC? I can't remember... :/

@opdenkamp
Copy link
Member

if you tested those cases i mentioned, and tried installing a new add-on via a zip which gets disabled, then +1

t-nelson added a commit that referenced this pull request May 26, 2014
Avoid incorrect disabling PVR addons (Alternate)
@t-nelson t-nelson merged commit 274c4a5 into xbmc:master May 26, 2014
@MartijnKaijser MartijnKaijser modified the milestones: Helix 14.0-alpha1, Pending for inclusion May 26, 2014
@AlwinEsch
Copy link
Member

Has also add your correction to my ADSP Pull request #4402 which use the same way to load the add-ons, thank a lot t-nelson.

@t-nelson t-nelson deleted the alt_incorrect_pvr_addon_disable branch May 27, 2014 03:32
@stefansaraev
Copy link
Contributor

@t-nelson this breaks "reset pvr database". reverting last commit fixes it.

@t-nelson
Copy link
Contributor Author

"Breaks" how? I'm pretty sure I've used the reset thing since this went in and the database seemed pretty empty.

@stefansaraev
Copy link
Contributor

"breaks" like pvr plugin disabled, pvr database wiped, pvr plugin NOT (re)enabled.

@t-nelson
Copy link
Contributor Author

OK. I'll look into it on Monday.

@t-nelson
Copy link
Contributor Author

@opdenkamp I guess we need to clear m_clientMap from CPVRClients. Is there anything else we should/shouldn't consider wiping from memory on db reset?

@opdenkamp
Copy link
Member

the add-on shouldn't be automatically re-enabled when you clear the database. if it did that before, then that was a bug. it should only clear the pvr tables, not do anything with the add-on db

@stefansaraev
Copy link
Contributor

k then, let me explain it again, in depth..

my use case: add some (iptv) channels to vdr backend.

before this pr, 2 simple steps to reset and enjoy live tv:

  • do stuff on vdr's channels.conf, restart backend
  • settings -> live tv -> reset pvr database
  • main menu -> live tv. new channels there, all works as expected

after this pr, 4 not-so-simple steps, including an annoying "hey I am not runing" message, or "what where did TV go?":

  • do stuff on vdr's channels.conf, restart backend
  • settings -> live tv -> reset pvr database
  • main menu -> tv. meh. there is no tv entry in main menu at all. as there is no single pvr client addon runing

next:

  • go settings -> addons -> disabled addons -> vdr vnsi pvr client -> enable (because it is disabled!)
  • main menu -> tv - all good. I can enjoy xbmc ;)

or:

  • settings -> live tv > disable -> enable -> got message "no pvr clients enabled" -> ok -> list shows up -> vnsi -> enable
  • main menu -> tv ...

this is a change in behaviour. the one before this pr is what I am familiar with, and almost everyone I know that use xbmc + pvr.

the add-on shouldn't be automatically re-enabled when you clear the database. if it did that before,
then that was a bug. it should only clear the pvr tables, not do anything with the add-on db

so what do you mean here, excuse my ignorance but I dont understand:

  1. disabling all pvr clients on pvr database reset is a normal thing. they should not be re-enabled. if that happened before, that's a bug
    or
  2. people are NOT allowed to reset the pvr database while pvr manager is runing, they are expected to go live tv -> disable, then reset, then re-enable?

if 1. then see above the change-in-behaviour note, because that's what most users (at least those I know personaly, including me) are familiar with
if 2. the "reset pvr database" option should be greyed out if pvr manager is runing. in general that's ok and I feel it acceptable. I can prepare a patch and send PR if you think that's what "reset pvr database" is intended to do.

stefansaraev added a commit to stefansaraev/xbmc that referenced this pull request Oct 28, 2014
this reverts upstream commit b6bec7a (part of xbmc/pull/4761):

> From b6bec7a Mon Sep 17 00:00:00 2001
> From: Trent Nelson <trent.nelson@pivosgroup.com>
> Date: Wed, 21 May 2014 17:16:54 +0800
> Subject: [PATCH] [PVR] Make sure client addons are disabled first time we see
>  them.

xbmc/pull/4761 fixed a bug where newly added pvr clients were automaticaly
enabled and could not be disabled first time an user tries to disable em.

unfortunately, it introduces a regression, now "reset pvr database" is useless
and leaves the user with no pvr clients enabled at all.

however, the "oh-i-can-not-disable-pvr-addons" bug I can not reproduce
anymore, but this commit may re-introduce it for some users.
@stefansaraev stefansaraev mentioned this pull request Oct 28, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants