Skip to content

more settings cleanup #2445

Merged
merged 15 commits into from Apr 1, 2013

5 participants

@Montellese
Team Kodi member

This is more of the settings cleanup work I initially presented in #2190. It adds two new interfaces:

  • ISettingsHandler has methods that are called when settings are loaded/saved and allow other classes to react to those events (e.g. CRssManager can load RssFeeds.xml when guisettings.xml has been loaded). Classes that implement ISettingsHandler in this PR are CPlayerCoreFactory, CRssManager, CUPnPSettings and CMediaSourceSettings.
  • ISubSettings has methods to load and save settings into guisettings.xml allowing the code to be split up into more logical groups. Classes that implement ISubSettings in this PR are CSkinSettings, CViewSettings and CMediaSettings.

All the newly introduced classes each contain a set of methods that logically belong together and IMO should get their own class to be easier accessible and didn't really belong into CSettings in the first place.

There are more settings that IMO can and should be moved into seperate classes like all the display/GUI calibration, resolution information, profile access etc and I'll start working on those once this has gotten the green light.

@jmarshallnz
Team Kodi member

Looks fine with a quick scan.

I notice nothing is currently unregistering itself. Perhaps it could do so on app close?

@Montellese
Team Kodi member

Yeah I forgot to add the unregistering because we clean that array in the CSettings destructor anyway so it never was a problem. But I'll add the calls to keep it clean.

@theuni
Team Kodi member
theuni commented Mar 26, 2013

In general, it'd be helpful for android if we could avoid depending on the destructors to clean up too much at exit, since there are a few different notions of what "exit" actually means. dlopen'd libs are a good example of something that presents a problem (lib destruction at exit depends on the loader class still being alive, which leaves us in a very undefined state), and i imagine global settings could do the same.

I see you've agreed above, just figured i'd chime in with something anecdotal for some motivation :)

@theuni
Team Kodi member
theuni commented Mar 26, 2013

Btw, what's your plan for this PR? Is it in a state to be merged currently, even with major chunks missing? If so, I'd encourage you to go ahead and shove it in for april and deal with the fallout, then start working on the next steps. That should help with the overwhelmingness some.

If that's the case, imo it'd be worthwhile to declare the last day (or an extra day) of the merge window as yours, and make everyone work around you until you've gotten it in. @davilla suggested this for the unified-dependencies merge, and I think it worked out very well.

@Montellese
Team Kodi member

This PR does not really have any impact on behaviour or anything. It's just refactoring and moving stuff around. Ideally I'd like to throw it in the first day because I already have another branch which depends on this one with more refactoring etc. The faster I get these things in, the sooner I can go back to the actual settings refactoring work (damn that rebase is gonna be a pain :-/ ).

@jmarshallnz
Team Kodi member

I don't see why it can't go in April 1 - it'll need some xcode project love but that's about it.

@theuni
Team Kodi member
theuni commented Mar 26, 2013

Well, the concern there is that your PR has the potential to interrupt several others. If there's not much in the pipeline that touches this stuff, then of course that's moot.

@MartijnKaijser
Team Kodi member

imo this should indeed go in 1st
I think the other PRs that it might affect can be handled/adjusted more easily than fixing this back if you pull it in the last day.

@Montellese
Team Kodi member

I've added a commit with the necessary calls to the UnregisterFoo method as requested. I'll check the other PRs currently queued for April and see if there are any potential issues (ignoring project files).

@Montellese Montellese was assigned Mar 26, 2013
@Montellese
Team Kodi member

I just made sure this also compiles fine on linux. Had to add two small include fixes.

@Memphiz
Team Kodi member
Memphiz commented Apr 1, 2013

@Montellese this pr went gray when i was about to sync xcode projects - did a pr to your branch with the needed changes (compile tested osx/ios/atv2).

@Montellese Montellese merged commit cdd62b9 into xbmc:master Apr 1, 2013
@Montellese Montellese deleted the Montellese:settings_cleanup_3 branch Apr 1, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.