Skip to content
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

Disable editing of properties of nodes inside included workflows in the property editor #899

Open
glopesdev opened this issue May 9, 2022 · 5 comments · Fixed by #1165
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@glopesdev
Copy link
Member

glopesdev commented May 9, 2022

Although included workflows are read-only and cannot be modified, the property grid still allows editing of internal properties. For consistency and to prevent accidental misunderstanding all internal operators of included workflows should be marked with the read-only attribute in the property grid.

@glopesdev glopesdev added the bug Something isn't working label May 9, 2022
@glopesdev glopesdev added this to the 2.7.0 milestone May 9, 2022
@glopesdev
Copy link
Member Author

glopesdev commented Jun 11, 2022

Following subsequent testing and evaluation of this feature during rapid-prototyping of real applications, there is a case to be made that it might be premature to close this possibility right now. Although only externalized properties are serialized in the workflow, it might be necessary on occasion to change temporary parameters inside the nested workflow for calibration purposes.

Relevant cases requiring this capability:

  1. Property editors for computer vision modules, such as ROI calibration or warp, often require as their input context the original image stream, and might not work when externalized and manipulated from outside. There is a case to be made here for allowing reversion of editors to internal node sources (c.f. Revise constraints on MemberSelectorEditor #863), but this will inevitably be a longer discussion.
  2. When the included workflow encapsulates cold-sources such as FileCapture, it might be useful to temporarily manipulate video position for calibration purposes of other downstream properties, even though we might not want to ultimately expose such properties in the reusable include workflow interface.

Possible workarounds and design considerations:

  • One straightforward extension to resolve 1. would be to selectively allow externalized properties to be edited internally. Since externalized properties are indeed serialized, there is no harm in allowing them to be changed internally using the context-relevant editor since the end result will be identical as if we were editing the properties externally. However, this approach will significantly complicate the implementation strategy adopted in a164adc for this feature.
  • Another possibility to resolve 1. would be to revert all property editors to always use the immediately proximal context even when externalized in a group workflow. While this would guarantee the applicability of the editor, it does limit severely the existing possibility of expanding the editing context. This is especially useful in the context of the above discussed image processing editors. An obvious example here is the RoiActivity operator. It is common with this operator for the developer to want to select ROIs in the original unfiltered image, even though the RoiActivity itself is working on binarized, post-threshold images. This is important to visually confirm the boundaries of areas which might not even be visible at all after binarization, and is currently made possible simply by grouping the node before the threshold and externalizing the Regions property at the top-level of the group. When the property is externalized in this way, the editor can use the top-level input to the node as an expanded context for editing, which will work fine as long as the input is compatible. Of course this is heavily context-dependent, and there is no guarantee of intended meaning without developer knowledge, even taking into consideration compatibility of types.
  • There is currently no good workaround for 2. Trying to avoid editing internal properties is bound to be tedious and error-prone, involving carefully turning include workflows back into write-only groups, exposing properties, or testing internal calibrations, and re-saving. Without advanced knowledge of Bonsai include workflow rules, it will be very hard to do this without inadvertently corrupting internal settings of existing includes, which one could say is even worse to understand than accidentally editing read-only properties. This limitation is enough to make this feature very impractical under time-constrained development, and is bound to lead to frustration, especially in situations where an experienced developer can see that a short-term modification would have settled a calibration issue.

@glopesdev glopesdev added the help wanted Extra attention is needed label Jun 11, 2022
@glopesdev
Copy link
Member Author

glopesdev commented Jun 11, 2022

Another possibility to resolve 1. would be to revert all property editors to always use the immediately proximal context even when externalized in a group workflow. While this would guarantee the applicability of the editor, it does limit severely the existing possibility of expanding the editing context.

Another existing related approach to expand editing context is to create property sources from the relevant properties, connect the desired context input to them, and map their output to the original node. Since editors are carried over from original properties to property sources, the editor is still available while the input is effectively redirected. The redirected input in this case can be used by the editor even if it only relies strictly on proximal context.

The main disadvantage of relying on such an approach is that one has to pay the cost of constantly assigning properties even when not editing, since value propagation depends on mapping the output of the property source to the original node, and the property source has to be triggered by the context input for the editor trick to work. However, this might only be relevant in a minority of cases, and there is room for future language feature improvements that would make it possible to provide the context without necessarily triggering value propagation.

glopesdev added a commit that referenced this issue Jun 11, 2022
@glopesdev glopesdev reopened this Jun 11, 2022
@glopesdev glopesdev removed this from the 2.7.0 milestone Jun 11, 2022
@glopesdev
Copy link
Member Author

There is an easier criterion to adopt that already would reduce the most significant structural problems with editing include workflow implementations, which is to assume that properties marked with the DesignOnlyAttribute are not possible to edit when nested inside an include workflow.

While it does require manual annotation of properties, it already would prevent changing the path of included nodes inside included workflows for example, which can be really confusing to debug if spread across different instances of the same include. This already works well for design vs runtime editing so it shouldn't be hard to generalize.

@jerlich
Copy link

jerlich commented Jun 2, 2023

FYI - I think we experienced a related issue today. We didn't notice that a workflow was a read-only extension and tried to edit a property that was not externalised. We could edit the property in the editor, but there was no change to any file on saving. This led to some confusion.

@glopesdev
Copy link
Member Author

glopesdev commented Jun 3, 2023

@jerlich Indeed I agree this discussion is not closed and leads to confusion, so will reopen.

@glopesdev glopesdev reopened this Jun 3, 2023
@glopesdev glopesdev removed this from the 2.7.2 milestone Sep 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants