Implement backed (and deferred) config leaves to pick up on POJO mutations #82
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…g from POJO Also removed an outdated comment in ConfigLeafBuilder: the backing collection should always be a NodeCollection, which we assume to be safe.
zeroeightysix
added
bug
Pyrofab
reviewed
Jul 6, 2020
src/main/java/io/github/fablabsmc/fablabs/impl/fiber/annotation/BackedConfigLeaf.java
Outdated
Show resolved
Hide resolved
Pyrofab
requested changes
Jul 8, 2020
src/main/java/io/github/fablabsmc/fablabs/impl/fiber/annotation/AnnotatedSettingsImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/fablabsmc/fablabs/impl/fiber/annotation/BackedConfigLeaf.java
Outdated
Show resolved
Hide resolved
Comment on lines
+69
to
+103
| @Nullable | ||
| @Override | ||
| public ConfigBranch getParent() { | ||
| return parent; | ||
| } | ||
|
|
||
| @Override | ||
| public void attachTo(ConfigBranch parent) throws IllegalTreeStateException { | ||
| // Attaching/detaching requires careful inspection of whether or not the parent's items contains `this`. | ||
| // In the case of deferred leaves, the leaf in the parent's items is the deferred leaf and not the backing leaf. | ||
| // Thus, we can't defer attaching/detaching to the backing leaf, as it will think it's not part of the parent's items. | ||
| if (this.parent != null && this.parent != parent) { | ||
| throw new IllegalTreeStateException(this + " needs to be detached before changing the parent"); | ||
| } | ||
|
|
||
| // this node may have already been added by the collection | ||
| if (parent != null && !parent.getItems().contains(this)) { | ||
| parent.getItems().add(this); | ||
| } | ||
|
|
||
| this.parent = parent; | ||
| } | ||
|
|
||
| @Override | ||
| public void detach() { | ||
| // Note: infinite recursion between ConfigNode#detach() and NodeCollection#remove() could occur here, | ||
| // but the latter performs the actual collection removal before detaching | ||
| if (this.parent != null) { | ||
| // here, we also need to avoid triggering a ConcurrentModificationException | ||
| // thankfully, remove does not cause a CME if it's a no-op | ||
| this.parent.getItems().remove(this); | ||
| } | ||
|
|
||
| this.parent = null; | ||
| } |
There was a problem hiding this comment.
Couldn't we delegate those methods to the backing node and thus avoid duplicating the logic ?
There was a problem hiding this comment.
No: attachTo and detach will add / remove this from the parent. We want the backed node to be added to the collection, and not the backing node.
There was a problem hiding this comment.
Then we should use inheritance to avoid duplicating the code.
There was a problem hiding this comment.
this.parent does not refer to the same field in both contexts, and we can't use #getParent (because we also need the nonexistant #setParent).
The duplication of code here is ugly, I agree, but I don't think we can use inheritance here. It wouldn't really fit into the delegation pattern BackedConfigLeaf follows anyways.
src/main/java/io/github/fablabsmc/fablabs/impl/fiber/annotation/BackedConfigLeaf.java
Outdated
Show resolved
Hide resolved
Pyrofab
approved these changes
Jan 24, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #16
I apologise in advance for the atrocious design choices in this PR.
The annotation system currently nicely wraps around only the builder API: it uses no fiber internals. I wanted to maintain this while also being able to introduce an extra check in
getValue(checking the corresponding field's value).One way of achieving this would be to just introduce more methods to the builder to allow this kind of customisation (e.g. a method to change the type of leaf being built, so people can allow the builder to produce something else than a
ConfigLeafImpl). This kind of approach seems rather hacky and would reduce the amount of guarantees fiber has when it comes to what type of nodes we're dealing with.The approach I chose is to not modify API, but instead:
getValuecode we needed earlier.This also means we can:
AnnotatedSettingsImpladds, and implement them inBackedConfigLeafinstead.