ValidateConnection method fixed to prevent multiple links into single input port. #6

daniilsavchuk · 2017-04-20T11:49:10Z

General idea

Currently Pipeline Builder does not take into account the links to the global workflow ports during new connection validation.This allows to bind multiple links into single input port which results in code generation errors.
This pull request introduces improved ValidateConnection method:
Check the global port bindings (to prevent adding link if it is already bound to workflow port) and making Pipeline Builder able to show the available ports to bind while dragging the new link.

According to this new checking method it becomes untrivial to understand which port is available to be connected, by this way pull reques also introduce the GUI improvements as follow:

Empty Ports
Available to connect ports (during link dragging)

Changes

Visualizer.js - override the Paper validateConnection method to take into account the workflow binding
pipeline.scss - added styles for empty and available ports
Paper.js - switched on the markAvailable JointJS feature and updated the port validation condition
VisualStep.js - changed update method to support port style switching in case of situation

…nk creation detection)

coveralls · 2017-04-20T11:52:08Z

Coverage remained the same at 94.703% when pulling 3b68ddd on multiple-link-fix into fa591e4 on dev.

voidest

Everithing seems OK for me; nonetheless, there I a copule of concerns I would like to clarify.

voidest · 2017-04-20T17:26:27Z

src/visual/Visualizer.js

+    this.paper.options.validateConnection = (cellViewS, magnetS, cellViewT, magnetT, end, linkView) => {
+      const args = [cellViewS, magnetS, cellViewT, magnetT, end, linkView];
+
+      if (this.paperDefaultValidateConnectionMethod.call(this.paper, ...args)) {


I am a little bit concerned about the usage of the spread syntax here since there is an apply method which does not require dots.

This is very strong notice and I followed this to make code seems more cleanly. Thank you very much.

Done in 3b68ddd commit

voidest · 2017-04-20T17:27:17Z

src/visual/Visualizer.js

@@ -86,6 +86,20 @@ export default class Visualizer {
    this._timer = null;
    this._step = null;
    this.clear();
+
+    this.paperDefaultValidateConnectionMethod = this.paper.options.validateConnection;


Is it really necessary to hold this value as a member?

Thank you so much for the review, it is very valuable!

I have followed this recommendation and it has been done in b29a2e2 commit

voidest · 2017-04-20T17:42:34Z

src/visual/VisualStep.js

+      if (port.group === 'out') {
+        isEmpty = isEmpty && (_.size(step.o[port.id].inputs) === 0 && _.size(step.o[port.id].outputs) === 0);
+      }
+


Could you please explain me the reason for putting isEmpty && in expressions above. As far as I understand, both if-conditions are mutually exclusive.

So usefull advise thank you very much!
Done in dc9b29b commit

coveralls · 2017-04-21T10:00:45Z

Coverage remained the same at 94.703% when pulling 3571b0a on multiple-link-fix into fa591e4 on dev.

paulsmirnov

I'm afraid there's an opportunity for the bug to appear that's need to be addressed before merging the PR.

paulsmirnov · 2017-04-21T10:18:39Z

src/visual/Paper.js

-        if (!magnetT || magnetT.getAttribute('class') !== 'port-body' ||
+        if (!magnetT || (magnetT.getAttribute('class') !== 'port-body' &&
+          magnetT.getAttribute('class') !== 'port-body-empty' &&
+          magnetT.getAttribute('class') !== 'port-body-empty available-magnet') ||


Is it possible to write the condition in a more compact and error-proof way? I'm a bit disappointed by multiple getAttribute() calls and strict comparisons. What if there are two spaces between classes? different class order? one more (irrelevant) class that we decide to add later? The condition will be broken in this case and we'll waste our time on debugging.

Thank you so for this comment. I thought about it when wrote a code upper, but it was moddified old variant (I did not change this condition paradigm). Anyway thank you very much. This method has been refactored, according to your advice and was pushed with the d9b5b6b commit!

paulsmirnov · 2017-04-21T10:41:12Z

src/visual/Visualizer.js

@@ -86,6 +86,20 @@ export default class Visualizer {
    this._timer = null;
    this._step = null;
    this.clear();
+
+    const paperDefaultValidateConnectionMethod = this.paper.options.validateConnection;


Won't oldValidateConnection or just validateConnection be shorter and more clear?

Thank you it is a nice note. The second proposed variant was selected.
Done in 3571b0a commit

coveralls · 2017-04-21T16:05:28Z

Coverage remained the same at 94.703% when pulling dc6f291 on multiple-link-fix into fa591e4 on dev.

voidest

OK.

paulsmirnov

Ok, thank you.

paulsmirnov · 2017-04-24T11:20:10Z

src/visual/Paper.js

+
+          _.forEach(portClasses, (portClass) => {
+            result = result && (availableClasses.indexOf(portClass) >= 0);
+          });


Lodash has a great shortcut for such iterations:

result = _.some(portClasses, pc => availableClasses.indexOf(pc) >= 0);

Probably, it would be even better to swap arrays (shorter - first). Also, _.intersection() is worth mentioning :)

Thank you so much for this advise. I promise I will use it in future in suitable situations not only at this code snippet (I hope it will be refactored by me in the next PR)

daniilsavchuk added 2 commits April 18, 2017 20:14

Improved validateConnection method (enabled workflow side multiple li…

e0c9320

…nk creation detection)

Improved visual for empty, available to connect and connected ports

3b68ddd

daniilsavchuk requested review from voidest, paulsmirnov and sidoruka April 20, 2017 11:49

voidest suggested changes Apr 20, 2017

View reviewed changes

daniilsavchuk added 3 commits April 21, 2017 12:40

Changed call(...args) to apply(args) to beautify code

b29a2e2

Refactored function storage variable to make it not a member

dc9b29b

Refactored port style selector function

3571b0a

paulsmirnov requested changes Apr 21, 2017

View reviewed changes

daniilsavchuk added 2 commits April 21, 2017 18:39

Refactored overritten function name

d9b5b6b

Refactored confition that check i/o ports grouping

dc6f291

voidest reviewed Apr 21, 2017

View reviewed changes

voidest approved these changes Apr 21, 2017

View reviewed changes

paulsmirnov approved these changes Apr 24, 2017

View reviewed changes

sidoruka approved these changes Apr 25, 2017

View reviewed changes

sidoruka merged commit 9d7b41f into dev Apr 25, 2017

sidoruka deleted the multiple-link-fix branch April 25, 2017 10:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ValidateConnection method fixed to prevent multiple links into single input port. #6

ValidateConnection method fixed to prevent multiple links into single input port. #6

daniilsavchuk commented Apr 20, 2017

coveralls commented Apr 20, 2017

voidest left a comment

voidest Apr 20, 2017

daniilsavchuk Apr 21, 2017

voidest Apr 20, 2017

daniilsavchuk Apr 21, 2017

voidest Apr 20, 2017

daniilsavchuk Apr 21, 2017

coveralls commented Apr 21, 2017

paulsmirnov left a comment

paulsmirnov Apr 21, 2017

daniilsavchuk Apr 21, 2017 •

edited

Loading

paulsmirnov Apr 21, 2017

daniilsavchuk Apr 21, 2017

coveralls commented Apr 21, 2017

voidest left a comment

paulsmirnov left a comment

paulsmirnov Apr 24, 2017

daniilsavchuk Apr 24, 2017 •

edited

Loading

ValidateConnection method fixed to prevent multiple links into single input port. #6

ValidateConnection method fixed to prevent multiple links into single input port. #6

Conversation

daniilsavchuk commented Apr 20, 2017

General idea

Changes

coveralls commented Apr 20, 2017

voidest left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 21, 2017

paulsmirnov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniilsavchuk Apr 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 21, 2017

voidest left a comment

Choose a reason for hiding this comment

paulsmirnov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniilsavchuk Apr 24, 2017 • edited Loading

Choose a reason for hiding this comment

daniilsavchuk Apr 21, 2017 •

edited

Loading

daniilsavchuk Apr 24, 2017 •

edited

Loading