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

add backward compatibility for AppearanceSettings #10265

Merged
merged 1 commit into from Aug 14, 2016

Conversation

@ronie
Copy link
Member

commented Aug 10, 2016

allow users with a jarvis compatible skin (which is still using AppearanceSettings) to navigate to InterfaceSettings in order to switch skins.

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Aug 10, 2016

Shouldn't those skin be auto marked broken and changed to Estuary as well?

@ronie

This comment has been minimized.

Copy link
Member Author

commented Aug 10, 2016

that would be best, but i don't think we have such logic in our code...

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Aug 10, 2016

iirc it worked with v15 -> v16. iirc @tamland also did a rescan of all deps when addon db was updated to new version to catch these issues.

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Aug 10, 2016

Seems indeed the dependency check which happened on addon DB upgrade is missing/non functioning.

@@ -332,7 +332,8 @@ static const ActionMapping windows[] =
{ "pvrsettings" , WINDOW_SETTINGS_MYPVR },
{ "playersettings" , WINDOW_SETTINGS_PLAYER },
{ "mediasettings" , WINDOW_SETTINGS_MEDIA },
{ "interfacesettings" , WINDOW_SETTINGS_INTERFACE },
{ "interfacesettings" , WINDOW_SETTINGS_INTERFACE },
{ "appearancesettings" , WINDOW_SETTINGS_INTERFACE }, // backward compat

This comment has been minimized.

Copy link
@MartijnKaijser

MartijnKaijser Aug 10, 2016

Member

could you add the version to it as well?

@tamland

This comment has been minimized.

Copy link
Member

commented Aug 11, 2016

There's never been any db upgrade checks.. What we have is an invalidation of the stored repository content when the repository addon version differ. If auto updates are on, it will trigger an update, which triggers a dependency check for addons in the repo and show a notification. (That last part I consider a bug). Afaik nothing's changed in this area.

That wont help if there's no update or you cant navigate to change it, so I'm wondering, since we break skin every release anyway, if we should just revert to default skin on upgrade. Otherwise this (and all future breaking changes) will have to stay in for as long as we want support upgrading from..

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Aug 11, 2016

Well it's big issue that dependencies are not checked again on upgrade for every installed Addon. I've been saying this for years already but is was always waved away. This is not only a skin issue but everything else as well. Always default back to default skin is not an option and it doesn't fix the issue if some pvr Addon or whatever should become incompatible.

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Aug 11, 2016

Actually i can tell you why it used to work. Back with v16 and before we just created the addons.xml files per addon branch and Kodi itself would gather all these XML files into a big list and from that filter out relevant addons. This method as such also contained skins or whatever from previous Kodi version, If there was no new version available it used the old version and compared it to the dependency on upgrade. That caused it to become "broken".
Nowdays we prefilter all addons on the server and only provide a full compatible list. As such the Kodi install has no idea the current installed version has become incompatible as it's no longer in the addons.xml list which is uses to check for compatibility issues.

@tamland

This comment has been minimized.

Copy link
Member

commented Aug 12, 2016

Yeah, makes sense. As I've said, that thing is broken by design and has never done any proper dependency checking, so no surprises there.

@ronie ronie force-pushed the ronie:bw-appearance branch from 6b34f81 to e57f573 Aug 12, 2016

@ronie

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2016

added version number as requested.

@MartijnKaijser MartijnKaijser merged commit 961d012 into xbmc:master Aug 14, 2016

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@MartijnKaijser MartijnKaijser added this to the Krypton 17.0-beta1 milestone Aug 14, 2016

@ronie ronie deleted the ronie:bw-appearance branch Oct 15, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.