WIP: Disconnect incompatible types when port definition changes #120

kairichard · 2019-10-18T18:35:36Z

The commit ce6fc74 introduced logic that would try to construct
a new generic type from all previously connected ports.

For doing so it would not use the actual type but try to reconstruct it
from the current port e.g. this. See the method getConnectedType.

This method, however, is not able to do that correctly and would often fail
thus resulting in the error types not unifiable.

The questions I was asking myself:

why would we want to try to reconstruct a type and unify it?
what was the original intention behind the implementation? - sadly that
is not clear from the code, the commit or the comments :-/
wouldn't it be easier to just leave everything as it is and not
surprise the user?
how can we avoid throwing the error and not freezing the editor?

In order to lower the surprise factor, I chose to remove most of the
original logic and simply disconnect two ports if their types are not
compatible.

The optimal solution, in my opinion, would be to check if the type change
can be propagated through all connected ports - both ways - and only
then apply the change w/o disconnecting anything. If at any point along
the connections/edges we encounter an incompatible type, cut the connection
between the root and the rest of the nodes.

But you know what - that is so far out of scope right now!

-- closes #119

The commit ce6fc74 introduced logic that would try to construct a new generic type from all previously connected ports. For doing so it would not use the actual type but try to reconstruct it from the current port e.g. `this`. See the method `getConnectedType`. This method however is not able to do that correctly and would often fail thus resulting in the error `types not unifiable`. The questions I was asking myself: - why would we want to try to reconstruct a type and unify it? - what was the original intention behind the implementation? - sadly that is not clear from the code, the commit or the comments :-/ - wouldn't it be easier to just leave everything as it is and not surprise the user? - how can we avoid throwing the error and not freezing the editor? In order to lower the surprise factor I chose to remove most of the original logic and simply disconnect two ports if their types are not compatible. The optimal solution in my opinion would be to check if the type change can be propagated through all connected ports - both ways - and only than apply the change w/o disconnecting anything. If at any point along the connections/edges we encounter an incompatible type, cut the connection between the root and the rest of the nodes. But you know what - that is so far out of scope right now!

kairichard · 2019-10-18T20:15:14Z

@td5r there are a few test failing but I am not 100% sure what they actually test and whether that makes sense? Advice needed.

td5r · 2019-10-25T11:40:41Z

@kairichard this is not done yet, is it? Incompatibales types won't be disconnet

kairichard · 2019-10-28T11:04:25Z

Streams inside a map

now disconnect as well
Another example:

kairichard · 2019-10-29T09:16:58Z

@td5r I think I discovered another reason why the original code exists - it allows connecting a trigger port to any other port w/o disconnecting it.

kairichard · 2019-10-29T09:32:56Z

I addressed this partly in 47232ed

kairichard · 2019-10-29T09:36:20Z

@td5r I am at the point where I would really like to rewrite the UI. With the underlying data structure being a directed acyclic graph and borrowing the type system from some other codebase that gets it right.

(ノಠ益ಠ)ノ彡┻━┻

td5r · 2019-11-06T22:04:58Z

src/slang/core/abstract/utils/connection-check.ts

 	// Triggers can always be destinations, even for specifications, maps and streams
 	if (destinationType.getTypeIdentifier() === TypeIdentifier.Trigger) {
 		return true;
 	}

+	if (sourceType.getTypeIdentifier() === TypeIdentifier.Trigger) {


@kairichard you may not connect a trigger source port to any other port but the other way around is ok.

@td5r But that is the case when I start dragging a connection from an operator inport to another outport. What you see in the last gif wouldn't work w/o this change.

td5r · 2019-11-06T22:06:22Z

src/styles/index.ts

-// @ts-ignore
-import styling from "./index.scss";
-export const STYLING = styling.toString();
+export const STYLING: string = `@import url(https://fonts.googleapis.com/css?family=Roboto+Slab:300%7CRoboto);.joint-viewport{-webkit-user-select:none;-moz-user-select:none;-ms-user-select:none;user-select:none}.joint-paper-background,.joint-paper-grid,.joint-paper>svg{position:absolute;top:0;left:0;right:0;bottom:0}[magnet=true]:not(.joint-element){cursor:crosshair}[magnet=true]:not(.joint-element):hover{opacity:.7}.joint-element{cursor:move}.joint-element *{user-drag:none}.joint-element .scalable *{vector-effect:non-scaling-stroke}.marker-source,.marker-target{vector-effect:non-scaling-stroke}.joint-paper{position:relative}.joint-highlight-opacity{opacity:.3}.joint-link .connection,.joint-link .connection-wrap{fill:none}.marker-vertices{opacity:0;cursor:move}.marker-arrowheads{opacity:0;cursor:move;cursor:-webkit-grab;cursor:-moz-grab}.link-tools{opacity:0;cursor:pointer}.link-tools .tool-options{display:none}.joint-link:hover .link-tools,.joint-link:hover .marker-arrowheads,.joint-link:hover .marker-vertices{opacity:1}.marker-vertex-remove{cursor:pointer;opacity:.1}.marker-vertex-group:hover .marker-vertex-remove{opacity:1}.marker-vertex-remove-area{opacity:.1;cursor:pointer}.marker-vertex-group:hover .marker-vertex-remove-area{opacity:1}.joint-element .fobj{overflow:hidden}.joint-element .fobj body{background-color:transparent;margin:0;position:static}.joint-element .fobj div{text-align:center;vertical-align:middle;display:table-cell;padding:0 5px 0 5px}.sl-port{-webkit-transition-duration:.2s;transition-duration:.2s}.sl-view{width:100%;height:100%}.sl-blackbox .sl-rectangle,.sl-blackbox-ghost .sl-rectangle{fill:#fefefe;stroke:#212124;stroke-width:1.5px;paint-order:stroke}.sl-blackbox .sl-label,.sl-blackbox-ghost .sl-label{fill:#212124}.sl-blackbox-ghost .sl-rectangle{fill-opacity:.1}.sl-outer .sl-rectangle{stroke:#212124;fill:#fefefe;stroke-width:1.5px}.sl-outer.sl-blupr-elem .sl-rectangle{cursor:not-allowed;fill:rgba(33,33,36,.1)}.sl-blueprint-port .sl-label{fill:#212124}.sl-port{stroke-width:0;stroke-opacity:.6;-webkit-transition-property:stroke-width;transition-property:stroke-width}.sl-port.sl-stripe{stroke:#fff;fill:#fff}.sl-port.sl-type-number{stroke:#2e49b3;fill:#2e49b3}.sl-port.sl-type-string{stroke:#952e2e;fill:#952e2e}.sl-port.sl-type-boolean{stroke:#ff764d;fill:#ff764d}.sl-port.sl-type-binary{stroke:#83a91d;fill:#83a91d}.sl-port.sl-type-primitive{stroke:#209cee;fill:#209cee}.sl-port.sl-type-trigger{stroke:rgba(152,152,151,.5);fill:rgba(152,152,151,.5)}.sl-port.sl-type-generic{stroke:#b86bff;fill:#b86bff}.sl-port.sl-type-ghost{stroke:rgba(184,107,255,.5);fill:rgba(184,107,255,.5)}.sl-connection-wrap{fill:none;stroke:#989897;stroke-width:10px;stroke-opacity:0;stroke-linecap:round}.sl-connection-wrap:hover{stroke-opacity:.3}.sl-connection-wrap.sl-is-selected{stroke:#212124;stroke-opacity:.2}.sl-connection-wrap.sl-is-selected~.link-tools{display:initial}.sl-connection{vector-effect:none;fill:none;stroke-opacity:1}.sl-connection.sl-type-number{stroke:#2e49b3}.sl-connection.sl-type-string{stroke:#952e2e}.sl-connection.sl-type-boolean{stroke:#ff764d}.sl-connection.sl-type-binary{stroke:#83a91d}.sl-connection.sl-type-primitive{stroke:#209cee}.sl-connection.sl-type-trigger{stroke:rgba(152,152,151,.5)}.sl-connection.sl-type-generic{stroke:#b86bff}.sl-connection.sl-type-ghost{stroke:rgba(184,107,255,.5);stroke-opacity:.3}@-webkit-keyframes port-pulse{from{stroke-width:0}to{stroke-width:8px}}@keyframes port-pulse{from{stroke-width:0}to{stroke-width:8px}}.joint-paper{font-family:Roboto,sans-serif;font-size:16px;font-size:1rem}.joint-highlight-stroke{stroke:none}.available-magnet path{-webkit-animation-name:port-pulse;animation-name:port-pulse;-webkit-animation-duration:.5s;animation-duration:.5s;-webkit-animation-iteration-count:infinite;animation-iteration-count:infinite;-webkit-animation-direction:alternate;animation-direction:alternate;-webkit-animation-timing-function:ease-in;animation-timing-function:ease-in}.tool-remove circle{fill:#f74125}.tool-remove path{fill:#fefefe}.sl-port-info{background:#f5f5f5;padding:1px;margin:0;border-radius:8px;font-size:12px;font-size:.75rem}.sl-port-info .sl-port-name{text-decoration:underline;padding:0 6px}.sl-port-info .sl-port-type{color:#fafafa;padding:0 6px;border-radius:15px}.sl-port-info .sl-port-type.sl-type-number{background-color:#2e49b3}.sl-port-info .sl-port-type.sl-type-string{background-color:#952e2e}.sl-port-info .sl-port-type.sl-type-boolean{background-color:#ff764d}.sl-port-info .sl-port-type.sl-type-binary{background-color:#83a91d}.sl-port-info .sl-port-type.sl-type-primitive{background-color:#209cee}.sl-port-info .sl-port-type.sl-type-trigger{background-color:rgba(152,152,151,.5)}.sl-port-info .sl-port-type.sl-type-generic{background-color:#b86bff}.sl-port-info .sl-port-type.sl-type-ghost{background-color:rgba(184,107,255,.5)}`;


@td5r must be reverted ⚠️

td5r · 2019-11-06T22:07:27Z

src/slang/core/abstract/port.ts

@@ -293,52 +305,6 @@ export abstract class GenericPortModel<O extends PortOwner> extends SlangNode {
 		return type;
 	}

-	public anySubStreamConnected(): boolean {


@kairichard did you remove them because they were not used anywhere?

@td5r indeed.

kairichard added Bug Something isn't working Editor labels Oct 18, 2019

kairichard assigned td5r Oct 18, 2019

kairichard changed the title ~~Try not be smart about types.~~ Disconnect incompatible types when port definition changes Oct 24, 2019

td5r changed the title ~~Disconnect incompatible types when port definition changes~~ WIP: Disconnect incompatible types when port definition changes Oct 25, 2019

Merge master and walk all connections and disconnect the imcompat ones.

969eeec

Get all connections to the port and walk them

f4bbf1b

Source and Destinations can be triggers

47232ed

Taleh Didover added 2 commits November 5, 2019 23:04

Make tests more readable

0470b73

Make tests more readable part 2

2e8a8a0

td5r reviewed Nov 6, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Disconnect incompatible types when port definition changes #120

WIP: Disconnect incompatible types when port definition changes #120

kairichard commented Oct 18, 2019

kairichard commented Oct 18, 2019

td5r commented Oct 25, 2019

kairichard commented Oct 28, 2019 •

edited

Loading

kairichard commented Oct 29, 2019 •

edited

Loading

kairichard commented Oct 29, 2019

kairichard commented Oct 29, 2019

td5r Nov 6, 2019

kairichard Nov 7, 2019

td5r Nov 6, 2019

td5r Nov 6, 2019

kairichard Nov 7, 2019

WIP: Disconnect incompatible types when port definition changes #120

Are you sure you want to change the base?

WIP: Disconnect incompatible types when port definition changes #120

Conversation

kairichard commented Oct 18, 2019

kairichard commented Oct 18, 2019

td5r commented Oct 25, 2019

kairichard commented Oct 28, 2019 • edited Loading

Streams inside a map

kairichard commented Oct 29, 2019 • edited Loading

kairichard commented Oct 29, 2019

kairichard commented Oct 29, 2019

td5r Nov 6, 2019

Choose a reason for hiding this comment

kairichard Nov 7, 2019

Choose a reason for hiding this comment

td5r Nov 6, 2019

Choose a reason for hiding this comment

td5r Nov 6, 2019

Choose a reason for hiding this comment

kairichard Nov 7, 2019

Choose a reason for hiding this comment

kairichard commented Oct 28, 2019 •

edited

Loading

kairichard commented Oct 29, 2019 •

edited

Loading