Conversation
While we do still support the previous more manual method of setting this up, we only want people to use the BoxOut method now, which is much easier to document.
The long term plan here is to remove Reference entirely, with SubGraph being able to support both referencing and local modes.
The new additions to SubGraphTest are all copied from ReferenceTest, but adapted to use Box nodes for both exporting and referencing. This gives us test coverage for a time when Reference no longer exists.
- Replace `isReferencePlug()` with `isReferenceable()`, supporting nodes too. - Use `isReferenceable()` in `exportReference()` instead of duplicating logic. - Remove code for dealing with archaic `.grf` files with plugs parented under `user`. These date from prior to version 0.18.0.0.
And prefer `pathlib` over ad-hoc string handling.
johnhaddon
force-pushed
the
referenceRefactorPR1
branch
from
0a4ec00 to
c8b0813
Compare
danieldresser-ie
left a comment
danieldresser-ie
left a comment
There was a problem hiding this comment.
There is quite a bit of code here, a lot of which is just shuffled around. I haven't gone through all the code in SubGraph that came from Reference to make sure that it hasn't been modified while being moved ... but in general, all of this seems to make sense. I haven't really thought through the details of the implications of moving towards just Box, but I know you have ... and I'm not seeing any problems here.
Is there anything specific you'd like me to stare a bit harder at?
One oddity is that the whole API is now public on SubGraph itself, but we only really want to expose part of it on Reference, and in future we want Box to be the only class there is. ... But potentially I could do that with protected methods on SubGraph and then make them visible publicly on a selective basis in Box and Reference. Do you think that's worth it?
I don't feel too strongly about the visibility of these functions, though I probably would like to see some clear documentation describing their current role in the header ( ie. currently some of them should only be called on Reference or Box, with a migration planned away from Reference in the long term ). But maybe having those sorts of caveats be part of of the API documentation would feel ugly to you, and you would prefer to make the functions protected?
| void loadReference( const std::filesystem::path &fileName ); | ||
|
|
||
| /// Returns the referenced file. If `isReference()` is false, returns an | ||
| /// empty path. |
There was a problem hiding this comment.
This comment ends up being a bit strange in the commit history, because the isReference() function referenced here doesn't exist until a later commit ... but it seems like it does make sense by the end of this PR.
2 participants
This represents phase 1 of the work needed for #5664. It consolidates all the existing functionality of Box and Reference into the shared SubGraph base class, tidying up a few things along the way. This part is reasonably straightforward, and other than the API additions doesn't change anything from a user perspective.
One oddity is that the whole API is now public on SubGraph itself, but we only really want to expose part of it on Reference, and in future we want Box to be the only class there is. I've mainly done it this way because I want to be able to see the final API all documented in one place rather than scattered between classes, so I can see where we're headed. But potentially I could do that with protected methods on SubGraph and then make them visible publicly on a selective basis in Box and Reference. Do you think that's worth it?
The next phase will be to add
localise()andconvertToReference()methods to SubGraph, and expose them in the Box UI so that users can swap between the two states interactively. I've made a start on this and there appear to be enough subtleties around serialisation and reference edits that I think getting this phase 1 merged first might be beneficial. At the very least I want the full test run that CI provides so I can be sure it's in a working state right now.