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

Abstract away settings logic using composition #856

Merged
merged 11 commits into from
Jul 11, 2020

Conversation

Barsonax
Copy link
Member

@Barsonax Barsonax commented Jun 29, 2020

Settings are now just POCO classes that are wrapped by the generic SettingsContainer class which contains the serialization logic and a changed event. It also implements a ISettingsContainer interface This cleans this logic up and allows us to treat settings in a more generic way (such as here https://github.com/AdamsLair/duality/pull/856/files#diff-3a2fbc8f3e471d6698b637e54ca23cffR998).

Also updated the naming of settings files according to #855.

Further possible improvements:

  • We can treat the settings container as a reference to the settings and no longer strictly need to go through DualityApp or DualityEditorApp anymore if we pass the container as a reference. This would actually remove the dependency on DualityApp or DualityEditorApp for alot of code which is a good thing. We might wanna do this in a separate PR to avoid inflating the scope of this PR though.

@Barsonax Barsonax changed the title Feature/settingscontainer Abstract away settings logic using composition Jun 29, 2020
@Barsonax Barsonax requested review from SirePi and ilexp and removed request for SirePi June 29, 2020 15:55
@ilexp
Copy link
Member

ilexp commented Jul 6, 2020

Is this ready for review, or still WiP?

@Barsonax
Copy link
Member Author

Barsonax commented Jul 6, 2020

Still WIP as I have to change the extension to .xml and do my own review

@Barsonax Barsonax marked this pull request as ready for review July 6, 2020 20:54
Copy link
Member

@ilexp ilexp left a comment

Choose a reason for hiding this comment

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

Added some minor change requests for the current implementation.

I'm a little bothered by the sudden influx of all the .Value indirections here, but due to the lack of serializer support for re-using the same instance, there's probably no way around that container design for now, as discussed in #855.

Source/Core/Duality/Utility/SettingsContainer.cs Outdated Show resolved Hide resolved
Source/Core/Duality/Utility/SettingsContainer.cs Outdated Show resolved Hide resolved
Source/Core/Duality/Utility/ISettingsContainer.cs Outdated Show resolved Hide resolved
Source/Core/Duality/Utility/SettingsContainer.cs Outdated Show resolved Hide resolved
@Barsonax
Copy link
Member Author

Barsonax commented Jul 10, 2020

Added some minor change requests for the current implementation.

I'm a little bothered by the sudden influx of all the .Value indirections here, but due to the lack of serializer support for re-using the same instance, there's probably no way around that container design for now, as discussed in #855.

Agree it clutters the code a bit but thats also because its used through DualityApp which leads to a quite long name (and being static state also causes alot of implicit lifetime dependencies). Ideally you just want to give the code that needs it a reference to the settingscontainer and no longer depend on any static class anymore. Will be hard to do this throughout the code base without something like dependency injection though.

@ilexp
Copy link
Member

ilexp commented Jul 11, 2020

Ideally you just want to give the code that needs it a reference to the settingscontainer and no longer depend on any static class anymore.

I disagree on that one, I think there's a balance to be struck here. But that's a topic for another time 😁

@Barsonax Barsonax merged commit 50a0009 into master Jul 11, 2020
@ilexp ilexp linked an issue Jul 13, 2020 that may be closed by this pull request
@Barsonax Barsonax deleted the feature/settingscontainer branch July 15, 2020 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Refactor/rethink how duality handles settings
2 participants