Skip to content
This repository was archived by the owner on Jul 11, 2025. It is now read-only.

Abstract away settings logic using composition - #856

Merged
Rick-van-Dam merged 11 commits into
masterfrom
feature/settingscontainer
Jul 11, 2020
Merged

Rick-van-Dam merged 11 commits into
masterfrom
feature/settingscontainer

Conversation

@Rick-van-Dam

@Rick-van-Dam Rick-van-Dam commented Jun 29, 2020 •

Copy link
Copy Markdown
Member

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.

@Rick-van-Dam Rick-van-Dam changed the title Feature/settingscontainer Abstract away settings logic using composition Jun 29, 2020
@Rick-van-Dam
Rick-van-Dam requested review from SirePi and ilexp and removed request for SirePi June 29, 2020 15:55
@ilexp

ilexp commented Jul 6, 2020

Copy link
Copy Markdown
Member

Is this ready for review, or still WiP?

@Rick-van-Dam

Copy link
Copy Markdown
Member Author

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

@Rick-van-Dam
Rick-van-Dam force-pushed the feature/settingscontainer branch from 8e3842c to 04431be Compare July 6, 2020 15:16
@Rick-van-Dam
Rick-van-Dam marked this pull request as ready for review July 6, 2020 20:54
@Rick-van-Dam
Rick-van-Dam force-pushed the feature/settingscontainer branch from 1e61d71 to 48d7eb0 Compare July 9, 2020 11:13

@ilexp ilexp left a comment

Copy link
Copy Markdown
Member

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.

@Rick-van-Dam

Rick-van-Dam commented Jul 10, 2020 •

Copy link
Copy Markdown
Member Author

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

ilexp commented Jul 11, 2020 •

Copy link
Copy Markdown
Member

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 😁

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Refactor/rethink how duality handles settings

2 participants