Bugzilla – Bug 6903
Remove any trace of Favorites if the plugin is disabled
Last modified: 2009-09-08 09:27:08 UTC
we'll have to make sure that any Favorites feature is removed from any interface when the Favorites plugin is disabled. Currently Jive will still display a Favorites menu (which does nothing), add to Favorites etc. even if the plugin is disabled. Mostly work on the CLI, I guess. Ben - I can probably take care of the SC side of things. Don't know how much time I'll have over the weekend, though.
Removing the Add to Favorites item in the lists is probably as simple as this: Index: /Users/mh/Documents/workspace/SC7.0/Slim/Control/Queries.pm =================================================================== --- /Users/mh/Documents/workspace/SC7.0/Slim/Control/Queries.pm (revision 17075) +++ /Users/mh/Documents/workspace/SC7.0/Slim/Control/Queries.pm (working copy) @@ -4397,7 +4397,7 @@ my $start = $args{'start'}; my $includeArt = defined($args{'includeArt'}) ? 1 : 0; - return ($chunkCount, $listCount) unless $loopname && $favorites; + return ($chunkCount, $listCount) unless $loopname && $favorites && Slim::Utils::PluginManager->isEnabled('Slim::Plugin::Favorites::Plugin'); if ($start == 0) {
More items to look at: - main menu - radio - services - plugins Will see what I can do tonight.
I don't think it's that simple, you'll still have a call later in the query to _jiveAddToFavorites, so you'll need to enclose that in an if{} clause too, right? The favorites main menu item sent via Jive.pm should be easy enough to not include if it's disabled
I think I'd personally argue that since Favourites is so entwined through the code that the direction should be to make it not a plugin. The editor can stay as one, but the rest is in Utils and many templates. If for no other reason, it serves as a bad example of a plugin for this reason as it gives an incorrect impression of what a plugin can and should be doing.
No disagreement from Michael or I on that front. It's just not going to happen before 7.0. Michael: the only other spot other than Queries.pm and Jive.pm that is sending favorites links to Jive is XMLBrowser. Cover those three files and I think you are done.
See also bug 6614
I've been thinking of whether favorites should be a plugin for a while. The original idea (pre jive) was that the opml implementation of favorites is a plugin, buts its wrapped in Slim::Utils::Favorites so there's a common api for the rest of the server which does not need to know which implementation is being used. You would be able to swap implementations or disable favorites and the rest of the server accesses it via Slim::Utils::Favorites. However we have leaked the cli code directly into the favorites plugin which breaks this... Two options: 1) continue with the split and more formally use Slim::Utils::Favorites as the api for the rest of the server 2) move much of favorites into the server itself including all the opml file handling etc. I would actually move the web interface and only leave the optional opml editor as a plugin Tend to favor the second, but this assume we are happy with opml as the implementation.. Will need to be post 7.0 though.
Why are we solving this problem? Does anybody really need to disable Favorites?
People are going to disable it if it's an option, and just like the large amount of people that had Digial Inputs disabled for the TP's and saw their Jive's not working right when it was disabled, it's going to create some UI confusion on Jive unless that's accounted for. We don't need to solve the "is favorites a plugin" issue right now, but we do need Jive to behave properly when it's disabled.
Add something to the plugin install.xml to allow a plugin to not be disabled and set that?
Ok, as a short term solution I'm going to add a <forceEnable> element to the install.xml. If it's set, it won't be disabled, checkbox in web page will be removed. Any objections? Better name for that element? I hope to have a patch soon.
Created attachment 2798 [details] add <enforced> tag to install.xml This patch enables "enforced" plugins during initialization, if they had been disabled, disables the checkbox on the settings page, and blocks an attempt to disable the plugin. Let me know what you think about it.
checked in as change 17116 Leaving this bug open for future work.
I'm impressed :) Very creative thinking! It's a good step. I hadn't really sorted it out when I suggested Slim/Plugin for core "plugins" but it really does make sense to have them enforceable since they are supported core functions. When we look at this later, we should think about which others might be 'optional' and work out a better handling so that users who have no need for that feature can disable it (might also be an idea for the Buttons/* stuff if users now have the ability to end up with entire networks of players with no playerUI. Imagine the memory savings, especially if we split the strings up?
The enforced plugins are well established by now. I doubt this bug as described in the subject will ever be fixed.