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

[settings] Serializer Interface #17062

Merged

Conversation

Montellese
Copy link
Member

Description

This PR introduces the ISettingsValueSerializerInterface and its first implementation CSettingsValueXmlSerializer which generates the exact same output as the currently builtin settings XML serializer.

Furthermore I noticed that the ISubSettings handling is currently integrated in the "generic" CSettingsManager but is actually only used in the Kodi "GUI settings" specific CSettings. It is not used for add-on settings or other settings in Kodi. Therefore it makes more sense to move this logic from CSettingsManager to CSettings. This also allows us to directly use the new ISettingsValueSerializerInterface in CSettingsManager instead of the hardcoded / builtin XML serialization.

I plan to do similar work for deserializing settings values and for deserializing settings definitions.

Motivation and Context

For python based scrapers we want to be able to pass the path-specific scraper settings to the python scraper. Since JSON is much closer to python than XML we need a serializer to turn settings into JSON. Therefore I wanted to introduce a clean interface to be able to implement both XML and JSON serialization of settings values. The PR for JSON serialization and scraper integration will follow.

How Has This Been Tested?

I compared the generated guisettings.xml with the original one and they are identical (apart from the order of the settings values).

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • None of the above (please explain below)

Checklist:

  • My code follows the Code Guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the Contributing document
  • I have added tests to cover my change
  • All new and existing tests passed

@Montellese Montellese added Component: Settings Type: Improvement non-breaking change which improves existing functionality v19 Matrix labels Dec 21, 2019
@Montellese Montellese added this to In progress in General (roadmap) via automation Dec 21, 2019
@Montellese Montellese added this to the Matrix 19.0-alpha 1 milestone Dec 21, 2019
@Montellese Montellese force-pushed the feature/settings_serializer_interface branch from 7bbafde to c834e01 Compare December 21, 2019 23:53
Copy link
Member

@Rechi Rechi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please follow the current code guidelines.

xbmc/settings/lib/ISettingsValueSerializer.h Outdated Show resolved Hide resolved
xbmc/settings/SettingsValueXmlSerializer.cpp Outdated Show resolved Hide resolved
xbmc/settings/SettingsValueXmlSerializer.cpp Outdated Show resolved Hide resolved
xbmc/settings/SettingsValueXmlSerializer.cpp Outdated Show resolved Hide resolved
xbmc/settings/SettingsValueXmlSerializer.cpp Outdated Show resolved Hide resolved
xbmc/settings/SettingsValueXmlSerializer.cpp Outdated Show resolved Hide resolved
xbmc/settings/SettingsValueXmlSerializer.cpp Outdated Show resolved Hide resolved
xbmc/settings/Settings.h Outdated Show resolved Hide resolved
@@ -92,7 +90,7 @@ class CSettingsManager : public ISettingCreator, public ISettingControlCreator,
\param root XML node
\return True if the setting values were successfully saved, false otherwise
*/
bool Save(TiXmlNode *root) const override;
bool Save(TiXmlNode *root) const;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change only removes the override keyword. Furthermore a following commit completely changes this line.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

E.g. if the commit which remove this line gets reverted the alignment will be wrong again.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes and if I either fix it in the same commit (which IMO is ugly) or in a cleanup commit and the whole PR is reverted it is also wrong again.

I completely agree that we should follow the coding guidelines in newly added code.
But as long as nobody has fixed all the existing code I'm not going to fix the coding style of every line I touch in existing code. That just makes it much more work to edit existing code and it puts the code into a completely messed up state coding style wise because some lines are fixed but most others are not which results in a mix of completely different coding styles throughout a file.

xbmc/settings/lib/SettingsManager.cpp Outdated Show resolved Hide resolved
@Montellese Montellese force-pushed the feature/settings_serializer_interface branch from c834e01 to 671aab1 Compare December 23, 2019 23:03
General (roadmap) automation moved this from In progress to Reviewer approved Dec 24, 2019
Copy link
Member

@garbear garbear left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Montellese Montellese force-pushed the feature/settings_serializer_interface branch from 671aab1 to cf8512a Compare December 25, 2019 09:15
@Montellese Montellese merged commit 596fb2f into xbmc:master Dec 31, 2019
General (roadmap) automation moved this from Reviewer approved to Done Dec 31, 2019
@Montellese Montellese deleted the feature/settings_serializer_interface branch December 31, 2019 23:51
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Jan 21, 2020
…izer_interface

[settings] Serializer Interface
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Settings Type: Improvement non-breaking change which improves existing functionality v19 Matrix
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants