-
Notifications
You must be signed in to change notification settings - Fork 287
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
Conversation
Is this ready for review, or still WiP? |
Still WIP as I have to change the extension to .xml and do my own review |
8e3842c
to
04431be
Compare
1e61d71
to
48d7eb0
Compare
There was a problem hiding this 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.
Agree it clutters the code a bit but thats also because its used through |
I disagree on that one, I think there's a balance to be struck here. But that's a topic for another time 😁 |
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 aISettingsContainer
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:
DualityApp
orDualityEditorApp
anymore if we pass the container as a reference. This would actually remove the dependency onDualityApp
orDualityEditorApp
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.