EAGLE-1601: Add 'changeable' flag to Field #995
Conversation
This file contains hidden or 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
…Display field changeable status in ParameterTable flags column.
Contributor
Reviewer's GuideAdds a runtime-only Sequence diagram for dragging an edge between applications with changeable-aware intermediate nodesequenceDiagram
actor User
participant Eagle
participant SourceNode as Node_src
participant SourcePort as Field_srcPort
participant DestNode as Node_dest
participant DestPort as Field_destPort
participant InterTemplate as Node_paletteTemplate
participant NewNode as Node_intermediary
participant IOField as Field_inputOutput
User->>Eagle: Drag edge from SourceNode to DestNode
activate Eagle
Eagle->>SourceNode: getPort()
SourceNode-->>Eagle: SourcePort
Eagle->>DestNode: findPortByMatchingType(type, [OutputPort|InputPort, InputOutput])
DestNode-->>Eagle: DestPort
alt intermediaryComponent exists
Eagle->>InterTemplate: clone()
InterTemplate-->>Eagle: intermediaryClone
Eagle->>Eagle: generateNodeId()
Eagle->>Eagle: addNodeToLogicalGraph(intermediaryClone, id, Default)
Eagle-->>Eagle: [NewNode]
else create default Data node
Eagle->>Eagle: new Node("Data", ...)
Eagle-->>Eagle: NewNode
end
Eagle->>NewNode: setPosition(x, y)
Eagle->>NewNode: setName(SourcePort.getDisplayText())
Eagle->>NewNode: findPortByMatchingType(SourcePort.getType(), [InputOutput])
NewNode-->>Eagle: IOField or null
alt IOField is null
Eagle-->>User: Error notification "Unable to find suitable port"
else IOField found
Eagle->>IOField: isChangeable()
alt IOField is changeable
Eagle->>IOField: setDisplayText(SourcePort.getDisplayText())
Eagle->>IOField: setDescription(SourcePort.getDescription())
else IOField not changeable
Eagle->>Eagle: Keep IOField name/description from palette
end
Eagle->>Eagle: create Edge(src, SourcePort, NewNode, IOField)
Eagle->>Eagle: create Edge(NewNode, IOField, DestNode, DestPort)
Eagle-->>User: Intermediate data node and edges created
end
deactivate Eagle
Class diagram for Field changeable flag and related Node/Eagle interactionsclassDiagram
class Field {
-ko.Observable~Node~ node
-ko.Observable~Map~ edges
-ko.Observable~boolean~ changeable
-ko.Observable~number~ inputX
-ko.Observable~number~ inputY
-ko.Observable~number~ outputX
-ko.Observable~number~ outputY
+isChangeable() boolean
+toggleChangeable() Field
+reset() Field
+clone() Field
+copyAttributesAndValuesTo(id FieldId, node Node) Field
+fromOJSJson(data any, changeable boolean) Field$
+fromOJSJsonPort(data any, changeable boolean) Field$
+fromV4Json(data any, changeable boolean) Field$
}
class Node {
+getFields() Field[]
+getInputPorts() Field[]
+getOutputPorts() Field[]
+findPortByMatchingType(type string, usages Daliuge.FieldUsage[]) Field
+setName(name string) void
+setPosition(x number, y number) void
+addField(field Field) void
+fromV4Json(nodeData any, errorsWarnings any, isPaletteNode boolean) Node$
+fromOJSJson(nodeData any, errorsWarnings any, isPaletteNode boolean) Node$
}
class Eagle {
+addNodeToLogicalGraph(node Node, id NodeId, mode Eagle.AddNodeMode) Promise~Node[]~
+logicalGraph() LogicalGraph
+handleEdgeDropBetweenApplications() void
}
class ParameterTable {
+getActiveColumnVisibility() any
+getNodeLockedState(field Field) boolean
+select(name string, column string, field Field, index number) void
+requestAddField(field Field) void
}
class Daliuge_FieldUsage {
<<enumeration>>
InputPort
OutputPort
InputOutput
NoPort
}
class Daliuge_FieldName {
<<enumeration>>
TRUE
FALSE
FILE_PATH
GROUP_START
GROUP_END
}
class Daliuge_Constants {
+branchTrueField Field
+branchFalseField Field
+groupStartField Field
+groupEndField Field
}
Field --> Node : node
Field --> "*" Edge : edges
Node "*" --> Field : fields
Eagle --> Node : creates/uses
ParameterTable --> Field : toggles changeable
Node --> Daliuge_FieldUsage : uses in findPortByMatchingType
Daliuge_Constants --> Field : palette fields
Daliuge_Constants --> Daliuge_FieldName
Daliuge_Constants --> Daliuge_FieldUsage
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The new
changeableflag is explicitly treated as runtime-only and is reset inreuse(), so any user toggles or palette-derived non-changeable state will not survive serialization/deserialization; consider whether this should be persisted or at least documented in code to avoid surprising behavior on reload. - In the intermediary data-node creation flow you now rely on
addNodeToLogicalGraph(...)[0]and then callsetPositionafterwards; ifaddNodeToLogicalGraphor downstream logic assumes the initial position argument, it may be cleaner to threaddataComponentPositionthrough rather than ignoring it and correcting later.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `changeable` flag is explicitly treated as runtime-only and is reset in `reuse()`, so any user toggles or palette-derived non-changeable state will not survive serialization/deserialization; consider whether this should be persisted or at least documented in code to avoid surprising behavior on reload.
- In the intermediary data-node creation flow you now rely on `addNodeToLogicalGraph(...)[0]` and then call `setPosition` afterwards; if `addNodeToLogicalGraph` or downstream logic assumes the initial position argument, it may be cleaner to thread `dataComponentPosition` through rather than ignoring it and correcting later.
## Individual Comments
### Comment 1
<location> `src/Eagle.ts:4577-4580` </location>
<code_context>
} else {
- intermediaryComponent = Utils.duplicateNode(intermediaryComponent);
+ //intermediaryComponent = Utils.duplicateNode(intermediaryComponent);
+ intermediaryComponent = intermediaryComponent.clone().setId(Utils.generateNodeId());
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Intermediary node ID is generated twice and may become inconsistent.
`intermediaryComponent` gets a new ID in `clone().setId(Utils.generateNodeId())`, and then a different ID is passed to `addNodeToLogicalGraph(intermediaryComponent, Utils.generateNodeId(), ...)`. Depending on how `setId` and `addNodeToLogicalGraph` work, this can desync the component’s internal ID from the graph’s ID or cause collisions. Consider generating the ID once and reusing it, or centralizing ID assignment in a single place.
</issue_to_address>
### Comment 2
<location> `src/Field.ts:483` </location>
<code_context>
this.isEvent(false);
this.node(null);
this.edges().clear();
+ this.changeable(true);
return this;
</code_context>
<issue_to_address>
**issue (bug_risk):** Resetting a field always makes it changeable, potentially overriding intended immutability.
`reset()` always sets `changeable` to `true`. If this is called on fields that should remain non-changeable (e.g., palette-derived/system fields), it will unexpectedly allow editing. Consider preserving the existing `changeable` value or providing a variant/parameter to control whether it should be restored.
</issue_to_address>
### Comment 3
<location> `src/Daliuge.ts:176` </location>
<code_context>
- export const branchYesField = new Field(null, FieldName.TRUE, "", "", "The affirmative output from a branch node", false, DataType.Object, false, [], false, FieldType.Component, FieldUsage.OutputPort);
- export const branchNoField = new Field(null, FieldName.FALSE, "", "", "he negative output from a branch node", false, DataType.Object, false, [], false, FieldType.Component, FieldUsage.OutputPort);
+ export const branchTrueField = new Field(null, FieldName.TRUE, "", "", "The affirmative output from a branch node", false, DataType.Object, false, [], false, FieldType.Component, FieldUsage.OutputPort);
+ export const branchFalseField = new Field(null, FieldName.FALSE, "", "", "he negative output from a branch node", false, DataType.Object, false, [], false, FieldType.Component, FieldUsage.OutputPort);
export const dropClassField = new Field(null, FieldName.DROP_CLASS, "", "", "", false, DataType.String, false, [], false, FieldType.Component, FieldUsage.NoPort);
</code_context>
<issue_to_address>
**nitpick (typo):** Minor typo in branch false field description string.
The description for `branchFalseField` is missing the initial `T` (currently "he negative output from a branch node"). Please correct this user-facing text while you’re updating this line.
```suggestion
export const branchFalseField = new Field(null, FieldName.FALSE, "", "", "The negative output from a branch node", false, DataType.Object, false, [], false, FieldType.Component, FieldUsage.OutputPort);
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
Added a 'changeable' flag to Field. This flag specifies whether the name (display text) of the Field can be changed. Many DALiuGE components will stop working if the name of certain fields is changed.
By default, changeable is true. But if the field is on a node that is part of a palette, then changeable is set to false. New fields added to Nodes by users will remain changeable.
I've added an icon in the "flags" column of the ParameterTable, to allow "expert" users to change the status of the flag. If "changeable" is false, then the input element in the "Attribute Name" column of the table is disabled.
I also fixed a related bug, where intermediate nodes, automatically inserted when the user drags an edge from Application to Application, are now more like the original Palette node, and not custom-modified to suit the edge.
Summary by Sourcery
Introduce a changeable flag on fields to control whether their display name can be edited and adjust node/edge handling and UI accordingly.
New Features:
Bug Fixes:
Enhancements:
Tests: