Yicong-Huang commented on code in PR #6437:
URL: https://github.com/apache/texera/pull/6437#discussion_r3887775622
##########
frontend/src/app/workspace/service/joint-ui/joint-ui.service.ts:
##########
@@ -1080,3 +1080,27 @@ export function fromJointPaperEvent<T extends keyof
joint.dia.Paper.EventMap = k
(handler, signal) => paper.off(eventName as string, handler, context) //
removeHandler
);
}
+
+/**
+ * Observable of a JointJS graph event that reports a cell, emitting that cell.
+ *
+ * The event name is restricted to the cell-reporting events: graph-level ones
+ * (`change`, `reset`, `sort`) hand the handler the Graph itself, which would
+ * make the `Cell` payload a lie. Widen the union only alongside a check that
+ * the new event reports a cell.
Review Comment:
The union is right; two of the three examples are not. Backbone 1.4.1 fires
`reset` and `sort` as `(collection, options)` (`backbone.js:965`, `:1045`); the
graph re-triggers them verbatim, so a handler gets the cells collection, not
the Graph. And `change` is re-triggered per cell by `_onModelEvent`
(`:1210-1222`), so it usually does deliver a Cell.
The next line tells a maintainer to widen the union only for cell-reporting
events, and `change` is the name they would wrongly rule out. The slip is mine
— it was my wording in the thread that asked for the narrowing.
```suggestion
* The event name is restricted to events that report a cell: `reset` and
`sort`
* hand the handler the cells collection instead, and `change` reports a
cell only
* when a cell changed — setting a graph attribute delivers the Graph. Widen
the
* union only alongside a check that the new event reports a cell.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
