[addons] add support for 'never' updating or checking for updates #4287

Merged
merged 7 commits into from Jun 10, 2014

Projects

None yet

6 participants

@jmarshallnz
Member

3 commits here. The first switches the existing boolean setting 'general.addonautoupdates' to an integer setting 'general.addonupdates'. There's no upgrade path here, so it will default to "auto update". @Montellese I don't think there's an easy upgrade path as the previous setting was boolean and is now gone? I don't think we can update boolean -> int in this way, right?

The second adds 'never' as an option. This essentially eliminates the repository update jobs. The repositories can then ONLY be updated via the context menu. I could potentially change this to force an update when the user clicks on "Get Add-ons".

The last adds the change to Confluence. Everything still works with a toggle button (i.e. other skins) but the never option is unavailable here.

One question is whether to split the strings up to use 'Auto updates: %s' instead of the current setup. I don't really see a major advantage in this though. Alternatively I could split the button into label1+label2 I guess. One concern I have is space on non-English languages.

@t-nelson
Contributor

Looks good.

I'm indifferent regarding the string split. Unless someone tries to bikeshed. In which case just leave it as it is.

jenkins build this please

@jmarshallnz
Member

@MartijnKaijser would appreciate some sanity checking (i.e. proper testing with more than one skin) here.

@MartijnKaijser
Member

first problem found.
set update to never, hit force refresh on repo and all add-ons update.

Edit:
also break skins as they all need to be updated.

@arnova arnova commented on an outdated diff Feb 28, 2014
xbmc/addons/Repository.cpp
@@ -276,7 +276,7 @@ bool CRepositoryUpdateJob::DoWork()
!database.IsAddonBlacklisted(newAddon->ID(),newAddon->Version().c_str()) &&
deps_met)
{
- if (CSettings::Get().GetBool("general.addonautoupdate") || addon->Type() >= ADDON_VIZ_LIBRARY)
+ if (CSettings::Get().GetBool("general.autoupdates") == AUTO_UPDATES_ON || addon->Type() >= ADDON_VIZ_LIBRARY)
@arnova
arnova Feb 28, 2014 Member

Shouldn't that be GetInt() ?

@arnova arnova and 1 other commented on an outdated diff Feb 28, 2014
xbmc/pvr/PVRManager.cpp
@@ -195,7 +195,7 @@ bool CPVRManager::InstallAddonAllowed(const std::string& strAddonId) const
void CPVRManager::MarkAsOutdated(const std::string& strAddonId, const std::string& strReferer)
{
- if (IsStarted() && CSettings::Get().GetBool("general.addonautoupdate"))
+ if (IsStarted() && CSettings::Get().GetBool("general.addonupdates") == AUTO_UPDATES_ON)
{
@jmarshallnz
jmarshallnz Feb 28, 2014 Member

Good spotting - will fix.

@Montellese
Member

Yeah you can't update the setting's value if you change its type. You could keep the old setting and then do an update for the new one but that would leave us with an unused/deprecated setting.

@Montellese
Member

@MartijnKaijser it shouldn't break skins as the code checks whether the control is a button (new behaviour) or not (old behaviour with a toggle) and works differently depending on that condition.

@MartijnKaijser
Member

ah ok. only saw that it was an on/off toggle in other skins. couldn't actually try it because it would update regardless

@MartijnKaijser
Member

would it btw still do a dependency check when some one updates from frodo->gotham on the already installed addons without fetching a new list?

@da-anda
Member
da-anda commented Feb 28, 2014

instead of hiding the update functionality in the context menu, how about a "check for updates" button right on AddonManager root page/list? So next to "enabled addons", "disabled addons", "get addons", "intall form zip" and "search". So same as the "available updates" entry which is conditionally displayed, the "check for updates" item would only be shown if setting is set to "never" (or maybe "manually"). Alternatively change the logic for the "available updates" item, so if auto-update is disabled change it's label to "check for updates" and when entering do all the update checks and show the result. In case of no updates don't enter the item/node but show an ok dialog "no updates found" (no kaitoast, because a kaitoast is hit and miss while a dialog will always be noticed, even if user left the room)

@jmarshallnz
Member

@da-anda good idea regarding Available -> Check for updates. I could show the latter if Available isn't there and the repos were last checked over 6 hours ago. Another thing to consider here is that if 'never' is on, we can almost guarantee we don't have the up to date list, so should always try and re-fetch when going into Available updates or Get Add-ons.

@jmarshallnz
Member

@MartijnKaijser no, it can't know there needs to be updated dependencies without fetching the repo.

@MartijnKaijser
Member

@jmarshallnz as long as it checks the local addons.xml file for existing dependencies else you may end up with a broken Frodo skin in Gotham without knowing it. This behaviour should already present i think without knowing anything from repo right?

@jmarshallnz
Member

We don't check anything except at install time.

@jmarshallnz
Member

Fixed the issue with the repo updating when it shouldn't, and switched "Available updates" to "Check for updates" when the repos need updating.

Also made the 'Get Add-ons' and 'Available updates/Check for updates' wait on repo updates as required.

@MartijnKaijser
Member

Tested it briefly and force update when never update was chosen indeed just showed a notify. Still all modules got updated so guess we can't get around adding those to the addon manager as well.
I think @da-anda had some remarks

@jmarshallnz
Member

Correct - my plan is to look at the modules next.

@jmarshallnz
Member

Changed the default to 'notify' rather than 'auto update'.

The idea is that we shouldn't alter the user's system unless they tell us to. They'll get broken and update notifications, but won't get auto-updates by default.

Arguments for and against in the add-ons thread in the forum please.

@da-anda
Member
da-anda commented Mar 2, 2014

The remark I had is that we should not hide the "check for updates" button. Users won't understand why the button is visible sometimes and sometimes not. Instead we always show the button but internally only perform the checks every 6 hours (or what we currently do). Or (if possible) show a "last check: dd-mm-yy h:m" secondary label on right hand side of the list (label2 or how that's called internally) and always grab the repo when user is entering that node (might be useful for skinners to check dev repo updates). Or a mixture of both (try to always fetch repo and only use a minimal buffer time like 5-10 min). This all ofc only if mode is set to 'disabled/manually'

@MartijnKaijser MartijnKaijser added this to the Pending for inclusion milestone Mar 2, 2014
@jmarshallnz
Member

The problem there is what to do when the user clicks on it - what could be done instead, is it says "Check for updates" if we last checked over 6 hours ago, and "Available updates" if we checked less than that, but still always show the button even if there are no available updates, with the 'last checked' time specified. And then force check in the latter case.

i.e. only one entry for both available and check, but always make sure it's shown, and always make sure we refresh the repo on entry, particularly under "check".

@da-anda
Member
da-anda commented Mar 2, 2014

in same way that users won't understand that the button disappears they also won't understand why it's label changes. Actually changing the label would also mean rearranging it because the list is sorted alphabetically - and depending on translation order will be different.
Oh, I just reallized that one might want to check for new updates (like to see if there is an update for a certain addon) while there are already other updates available. So we can't reused the "available updates" node but need an entirely independent button/node. And with this button we have two options - either always trigger a real update check or only do the real check in 6h cycles and when in between return last known state ("no (new) updates available").

@jmarshallnz
Member

Ok - will make it a separate node. When you click on it it will force the update, and then either go to Available Updates or refresh the list if none are available.

Not sure if I can easily add the last checked date (well, I can easily add it, but I'm not sure how much room we have to fit it in - we need both time and date, but it gets quite a long string with "Last checked: 12:30 PM, 31/12/2013". Suggestions welcome.

@jmarshallnz
Member

Ok, updated with the above. The "Check for updates (last updated 12/03/2013 12:30pm)" item is on top and is always displayed regardless of auto-update setting.

It works fine if you don't have many repos installed. More repos will ofc be a bit slower (more hashes to hit) but as it's a directory fetch, the busy is up.

We may wish to consider forcing a delay between checks.

@jmarshallnz
Member

Updated and dropped the controversial one. Ideally there'd be some additional cleanup in terms of the structure (plus whatever else is needed in terms of icons and the like).

@jmarshallnz
Member

@MartijnKaijser mind taking a look and seeing what (if anything) needs altering before this is merged. Doesn't give everything ofc, but at least gets all addons into the UI, allows update button in UI etc.

@MartijnKaijser
Member

Tried this some time ago and i'm ok with this so far. It does expend the list of categories even more which is already too long, but that can be separate PR to make that look simpler (like proposed here http://forum.xbmc.org/showthread.php?tid=191816&pid=1677295#pid1677295)

@MartijnKaijser
Member

jenkins build this please

@jmarshallnz jmarshallnz merged commit 8e0033a into xbmc:master Jun 10, 2014

1 check passed

default Merged build #849 succeeded in 1 hr 16 min
Details
@jmarshallnz jmarshallnz deleted the jmarshallnz:neverneverland branch Jun 10, 2014
@MartijnKaijser
Member

@jmarshallnz
i just noticed that the context menu on a repo contains "check for updates" twice now

@jeroenpardon jeroenpardon added a commit to jeroenpardon/skin.refocus that referenced this pull request Dec 24, 2014
@jeroenpardon jeroenpardon change id="5" ("Enable auto-updates") from radiobutton to button 0d898b5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment