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

support saving skin settings as addon data instead of in guisettings.xml #7359

Merged
merged 4 commits into from Jul 8, 2015

Conversation

@Montellese
Copy link
Member

Montellese commented Jun 28, 2015

These commits change the location where skin settings are stored. Instead of saving all skin settings from all skins in guisettings.xml (and making it rather huge) the settings are now stored in the skin's addon_data folder in settings.xml like any other addon with two differences:

  • skins don't require a settings.xml file with a definition for each setting
  • the format of the generated settings.xml slightly differs from the one used by all other addons (due to the previous point)

The migration of existing skin settings happens whenever a skin is loaded. We then look for skin settings matching the loaded skin in guisettings.xml and transfer them to the skin-specific settings.xml file. This is done because not all skins with skin settings in guisettings.xml might still be installed. This also means that guisettings.xml is only really cleaned up when loading the different skins.

@xhaggi

This comment has been minimized.

Copy link
Member

xhaggi commented Jun 28, 2015

+100 for this approach

@ronie

This comment has been minimized.

Copy link
Member

ronie commented Jun 28, 2015

yup, great stuff!

trying to build it on linux, but it's giving me a headache:
http://xbmclogs.com/pit7dwozp

@MartijnKaijser

This comment has been minimized.

Copy link
Member

MartijnKaijser commented Jun 28, 2015

without any doubt, awesome :)
One side note, this makes it impossible to roll-back to previous version and retain skin settings for that version as it's gone from guisettings.xml (if i understood correctly).

@Montellese

This comment has been minimized.

Copy link
Member Author

Montellese commented Jun 30, 2015

jenkins build this please

1 similar comment
@MartijnKaijser

This comment has been minimized.

Copy link
Member

MartijnKaijser commented Jul 4, 2015

jenkins build this please

@MartijnKaijser MartijnKaijser added this to the Isengard 16.0-alpha1 milestone Jul 4, 2015

void CSkinInfo::SetString(int setting, const string &label)
{
auto& it = m_strings.find(setting);

This comment has been minimized.

Copy link
@mkortstiege

mkortstiege Jul 5, 2015

Member

auto&& should make the other platforms build. same in CSkinInfo::SetBool

@Montellese Montellese force-pushed the Montellese:skin_settings branch from 12e25fc to 5072ead Jul 6, 2015
@Montellese

This comment has been minimized.

Copy link
Member Author

Montellese commented Jul 6, 2015

Updated. jenkins build this please

@Montellese

This comment has been minimized.

Copy link
Member Author

Montellese commented Jul 6, 2015

Other platforms now build fine as well. OSX-64 failure is unrelated (but maybe something is wrong with the slave because it failed with the same error on two builds in a row now).

@MartijnKaijser

This comment has been minimized.

Copy link
Member

MartijnKaijser commented Jul 6, 2015

happens sometimes and it's some weird jenkins issue that it can't pull from git. OSX32 is fine so i'm sure osx64 is ok as well

@mkortstiege

This comment has been minimized.

Copy link
Member

mkortstiege commented Jul 6, 2015

happens sometimes and it's some weird jenkins issue that it can't pull from git. OSX32 is fine so i'm sure osx64 is ok as well

Jep. Was building correctly this morning for OSX64.

@MartijnKaijser

This comment has been minimized.

Copy link
Member

MartijnKaijser commented Jul 7, 2015

Good for merge?

@mkortstiege

This comment has been minimized.

Copy link
Member

mkortstiege commented Jul 7, 2015

No objections from my side. Very welcome change ;)

@ronie

This comment has been minimized.

Copy link
Member

ronie commented Jul 7, 2015

tested and works fine for me.

MartijnKaijser added a commit that referenced this pull request Jul 8, 2015
support saving skin settings as addon data instead of in guisettings.xml
@MartijnKaijser MartijnKaijser merged commit 92c64e5 into xbmc:master Jul 8, 2015
1 check failed
1 check failed
default Merged build finished.
Details
@Montellese Montellese deleted the Montellese:skin_settings branch Jul 8, 2015
@MilhouseVH

This comment has been minimized.

Copy link
Contributor

MilhouseVH commented Jul 8, 2015

This PR is causing a major problem when running builds of the latest master (2efc445).

On Ubuntu, start without a .kodi folder (so that Kodi creates it) and kodi will loop trying to load guisettings.xml: http://sprunge.us/iKcZ

On OpenELEC (RPi2), the new test build #0708 will start using the existing guisettings.xml, but then it writes out a new guisettings.xml without any skin.confluence tags. Next time kodi.bin is started, Kodi goes into an infinite loop failing to read guisettings.xml: http://sprunge.us/Vadb

The last entries are:

00:19:08 T:139740317305024  NOTICE: Loaded playercorefactory configuration
00:19:08 T:139740317305024  NOTICE: Loading player core factory settings from special://masterprofile/playercorefactory.xml.
00:19:08 T:139740317305024  NOTICE: special://masterprofile/playercorefactory.xml does not exist. Skipping.
00:19:08 T:139740317305024   ERROR: CSettings: unable to load settings from special://masterprofile/guisettings.xml, creating new default settings

before looping.

Without PR7359, both Ubuntu and OpenELEC builds start (and restart) without a problem.

@ronie

This comment has been minimized.

Copy link
Member

ronie commented Jul 8, 2015

^ confirmed

@popcornmix

This comment has been minimized.

Copy link
Member

popcornmix commented Jul 9, 2015

@Montellese

This comment has been minimized.

Copy link
Member Author

Montellese commented Jul 10, 2015

On Ubuntu, start without a .kodi folder (so that Kodi creates it) and kodi will loop trying to load guisettings.xml: http://sprunge.us/iKcZ

Is this also happening on other platforms? I haven't tried with an empty or no userdata directory yet. Will give it a try on the weekend.

On OpenELEC (RPi2), the new test build #708 will start using the existing guisettings.xml, but then it writes out a new guisettings.xml without any skin.confluence tags.

Yes that's what is supposed to happen because those settings have been written to the settings.xml in confluences addon data directory.

Next time kodi.bin is started, Kodi goes into an infinite loop failing to read guisettings.xml: http://sprunge.us/Vadb

Obviously that's not supposed to happen but it worked fine for me last time I tried it.

I won't have time to look at this until either tomorrow afternoon or sunday afternoon.

@Paxxi

This comment has been minimized.

Copy link
Member

Paxxi commented Jul 10, 2015

@Montellese happens in windows as well, I started my test build and it ran fine, restarted it and endless recursions in Load.

@ace20022

This comment has been minimized.

Copy link
Member

ace20022 commented Jul 10, 2015

@Montellese

This comment has been minimized.

Copy link
Member Author

Montellese commented Jul 10, 2015

@ace20022: No it's still needed because we can only migrate the settings of a skin that is active. All other skin settings stay in guisettings.xml. I chose to do that because I have a lot of skin settings for skins that I don't have installed anymore. So those can only be migrated once the skin is installed again.

@stefansaraev

This comment has been minimized.

Copy link
Contributor

stefansaraev commented Apr 26, 2016

there are some (not crash and burn, but still a bit annoying to the user) issues with this change.

before, skin settings were (by luck) always saved in guisetting.xml on exit. now, skins act as regular addons, and their settings seem to be saved late, sadly, it happens quite often that kodi crashes or deadlocks on exit (be it due to bug in core or misbehaving addons), so skins settings are not always saved.

another issue, is skincolor and skintheme options are still part of guisettings.xml, so they are immediately lost on skin change.

EDIT: for reference: MrMC/mrmc@1bf053e

@Montellese

This comment has been minimized.

Copy link
Member Author

Montellese commented Apr 26, 2016

Well TBH the former isn't really an issue of this PR but of whatever is causing the crashes. And IIRC we already discussed the fix from MrMC and it was rejected because it could introduce serious performance issues when a lot of skin settings change in a short time. If saving the skin settings at the same time as guisettings increases the chance of skin settings being saved in case of a shutdown crash we can easily add a call to manually save skin settings right after saving guisettings. That would give (almost) the same saving on shutdown behaviour as before this PR without the chance of introducing random performance issues.

And the latter isn't an issue of this PR either because the problem already existed before this PR. The skincolor and skintheme settings are (unfortunately) completely separate from the skin setting so they always get reset to their default value when the skin changes.

@stefansaraev

This comment has been minimized.

Copy link
Contributor

stefansaraev commented Apr 26, 2016

yes. I know. it's just side effect uncovered by this change.

If saving the skin settings at the same time as guisettings increases the chance of skin settings being saved in case of a shutdown crash we can easily add a call to manually save skin settings right after saving guisettings

yep, I'll see if I can do something about this tomorrow (if you are not faster than me)

And the latter isn't an issue of this PR either because the problem already existed before this PR ..

thats right. but I think it makes sense to make skincolor and skinthem skin specific, so they are saved to userdata. as they (seem to be) skin specific :)

@MartijnKaijser

This comment has been minimized.

Copy link
Member

MartijnKaijser commented Apr 26, 2016

Saving settings on leaving the setting section/window might be enough. This way you save all changes in one go and don't wait till shutdown

@Montellese

This comment has been minimized.

Copy link
Member Author

Montellese commented Apr 27, 2016

@MartijnKaijser if that were the case nobody would be complaining right now. AFAIK skins can change their settings at any time. And skin settings don't only contain the settings you see in the skin settings dialog but also internal stuff like "do we show fanart in this movie view?" and stuff like that. So the settings can change at any time and completely independent of guisettings. That's why an additional call to saving the skin settings during shutdown might make sense.

@MartijnKaijser

This comment has been minimized.

Copy link
Member

MartijnKaijser commented Aug 7, 2017

@Montellese this is probably some old fallout from this change https://forum.kodi.tv/showthread.php?tid=319552

Well "fallout" is a hard word cause it was already discussed here as well :)

@xhaggi

This comment has been minimized.

Copy link
Member

xhaggi commented Aug 8, 2017

I've created a PR for that #12648

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