search

AdamsLair / duality

a 2D Game Development Framework
https://adamslair.github.io/duality
MIT License
1.41k stars 286 forks source link

Abstract away settings logic using composition #856

Closed Barsonax closed 4 years ago

Barsonax commented 4 years ago

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:

ilexp commented 4 years ago

Is this ready for review, or still WiP?

Barsonax commented 4 years ago

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

Barsonax commented 4 years ago

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 commented 4 years ago

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 😁