Network: use separate refresh indicator in NodeBase, instead of width… #2885

wimrijnders · 2017-03-21T06:08:28Z

This PR is a resolution of #1847 and #2436. Please examine these for the problem description.

Discussion

It should be noted that it is not necessary to call getNodeAt() within an event listener to determine the handled node; the node id is passed to the event listener in the supplied parameter:

network.on('click', function(params){ 
  var nodeId = params.nodes[0];
  ...
});

However, there are situations imaginable where getNodeAt() is called outside of an event listener,
so it still makes sense to ensure that this method is working correctly.

Analysis

The basic problem is that within Node, the dimension this.shape.width has a double function
as an indicator that a shape should be recalculated. This happens in method refresh(), which
is called by select() (among others):

  /**
   * Reset the calculated size of the node, forces it to recalculate its size
   * @private
   */
  _reset() {
    this.shape.width = undefined;
    this.shape.height = undefined;
  }

The undefined value of the width gets picked up during redraw in the shape instances, where it is
used in a lazy fashion to recalculate the shape. Almost all shapes do it in the following way:

  _resizeShape(...) {
    if ((this.width === undefined) || (this.labelModule.differentState(selected, hover))) {
      // Recalculate here
    }
  }

The exceptions are shapes CircularImage and Image, which defer the calculation until the contained image is loaded.

In practice, when a Node is clicked on, the click event will fire before Network.redraw() has been called. So, during the click event, the shape's width member is undefined.
The width is also used within getNodeAt() to determine the selected node, ultimately within method Node.isOverlappingWith(). Because the width is undefined, isOverlappingWith() returns undefined.

Solution

Member Shape.width should not be used as an indicator for resize, it should have a single task as a dimension. It is better that a new, different member variable is used for refresh indication.

Added member variable doRefresh in NodeBase, which is set in Node._reset()
Refactored test on resize required to NodeBase.needRefresh() for all relevant shapes
Note that the exceptional shapes Image and CircularImage do not have this adjustment

This can be tested in example examples/network/events/interactionEvents.html, where a line of code
is added to show the output of getNodeAt() during a click event.
All shape types have been tested using this page; however, this test code has been removed.

… parameter

wimrijnders · 2017-03-21T06:21:50Z

After staring long and hard at the code of the image shapes, I figured out how to best use doRefresh for these.

So now the refresh of shapes image and circularImage works similar to the rest of the shapes.

mojoaxel · 2017-03-21T18:43:44Z

examples/network/events/interactionEvents.html

@@ -53,6 +54,7 @@
    network.on("click", function (params) {
        params.event = "[original event]";
        document.getElementById('eventSpan').innerHTML = '<h2>Click event:</h2>' + JSON.stringify(params, null, 4);
+        console.log('click event, getNodeAt returns: ' + this.getNodeAt(params.pointer.DOM));


Thanks for updating the example. Perfect!

mojoaxel · 2017-03-21T18:45:10Z

lib/network/modules/components/Node.js

@@ -380,8 +380,7 @@ class Node {
   * @private
   */
  _reset() {


Can we get rid of the _reset function and replace it with needRefresh?

Yup, can do.

mojoaxel · 2017-03-21T18:47:32Z

lib/network/modules/components/nodes/util/NodeBase.js

+   *
+   * @protected
+   */
+  needRefresh(selected, hover) {


I think this is a "private" function. it should start with a underscore: _needRefresh.
Also i would rename it to _needsRefresh but that is just personal taste.

That was my initial thought too. However, it's not strictly private but protected, in the sense that it is called from derived classes. So I wasn't 100% sure to make it look like a private method.

Are there any guidelines for this?

There are no guidelines. I don't know the code good enough to know if there is a schema for this. Have a look at the surrounding code - you decide.

I still think you should add the "s": needsRefresh :-)

There are no guidelines

Actually, I think there should be. In due time, I will make a proposal; there are a couple of style issues that bother me.

needsRefresh

Yes, that's better. Silly me.

mojoaxel · 2017-03-21T18:58:29Z

lib/network/modules/components/Node.js

@@ -380,8 +380,7 @@ class Node {
   * @private
   */
  _reset() {
-    this.shape.width = undefined;
-    this.shape.height = undefined;
+    this.shape.doRefresh = true;


For me doRefresh sounds like a function. What about this.shape._refreshNeeded?

OK, but without the '_' prefix, because it is not private.

ok, I guess

mojoaxel · 2017-03-21T19:29:35Z

lib/network/modules/components/nodes/util/NodeBase.js

+   * @protected
+   */
+  needRefresh(selected, hover) {
+    if (this.doRefresh) {


It is a little dangerous to do a if (this.doRefresh) because this is valid e.g. also if doRefresh is a valid String or any object. That's why I would recommend to invert the logic:

if (!this.doRefresh) { return (this.width === undefined) || (this.labelModule.differentState(selected, hover)); } this.doRefresh = false; return true;

mojoaxel · 2017-03-22T14:57:33Z

lib/network/modules/components/nodes/util/NodeBase.js

+   *
+   * @protected
+   */
+  needRefresh(selected, hover) {


I still think you should add the "s": needsRefresh :-)

mojoaxel · 2017-03-22T21:51:34Z

lib/network/modules/components/nodes/util/CircleImageBase.js

@@ -84,6 +89,7 @@ class CircleImageBase extends NodeBase {
      this.width = width;
      this.height = height;
      this.radius = 0.5 * this.width;
+      this.refreshNeeded = false;


@wimrijnders Are you sure we don't need the refresh here because of the hover/selected state? I'm not sure I understand it.

The purpose of this line is to turn off the indicator variable after a triggered resize calculation.
As far as I can tell, all the required resizing has been done in this block before this line. The value is set to false to prevent the resize calculation from triggering continuously.

In summary, when disregarding the rest of the logic, what happens here is:

if (refreshNeeded === true) { // Do the resize calculation refreshNeeded = false; }

I hope this explains the intention. I believe this code is correct.

Ah, you mean: why is not hover and selected checked for triggering the resize calculation, just like with the rest of the shapes.

The direct answer is: I don't know, this is how the code worked before I added the changes and I did not want to change the logic too much.
I'll look at the code to see if I can understand this myself.

OK, I understand what is happening; needsRefresh() is/should be called in the child classes. So the changes I made to CircleImageBase are misplaced.

needsRefresh() is already called in Circle.resize(). For completeness, the call should also be used in CircularImage.resize().

I will make an update to the PR which I hope will clarify the point. Well spotted, @mojoaxel.

wimrijnders · 2017-03-24T06:59:35Z

OK, figured it out. I moved the test for refresh from CircleImageBase to its child classes.

I also did some cleanup and refactoring in the process, to better understand the code. But this was becoming a never-ending story: the more I look, the more I see things that need looking at. So, I left some TODO's in the code and quickly closed off before I found more.

So, please glance at the TODO's to see if they're OK.

britonk1 · 2018-04-13T10:42:48Z

@wimrijnders

Regarding getNodeAt() not being required within event listeners - I don't believe this to be the case for "oncontext" events? The on context event does not explicitly select the node that has been right clicked and therefore the node ID does not necessarily get passed in params.

I am still having the problem with getNodeAt using the latest version of VIS.

wimrijnders added 2 commits March 21, 2017 06:59

Network: use separate refresh indicator in NodeBase, instead of width…

a0f4f35

… parameter

Added usage of iBaseNode.doRefresh to image shapes.

dd1d04c

mojoaxel added Network Confirmed Bug labels Mar 21, 2017

mojoaxel added this to the Minor Release v4.20 milestone Mar 21, 2017

Rearranged code in NodeBase.needRefresh()

f23ee86

mojoaxel suggested changes Mar 21, 2017

View reviewed changes

yotamberk and others added 5 commits March 21, 2017 22:16

Merge branch 'develop' into PR31

d2d7166

Fixes for review

e840d82

Merge branch 'develop' into PR31

f602dca

Merge branch 'PR31' of github.com:wimrijnders/vis into PR31

fb06b54

Removed strange link

3ec5e81

mojoaxel suggested changes Mar 22, 2017

View reviewed changes

Added 's' in needsRefresh(); typo fix

9490fe6

mojoaxel reviewed Mar 22, 2017

View reviewed changes

Fixes for review, code cleanup

c3d75cf

wimrijnders added 2 commits March 24, 2017 08:41

Modified ratio calc to original working.

847dfda

Made code in CircularImage.resize() more sensible.

df7ad23

mojoaxel approved these changes Mar 28, 2017

View reviewed changes

mojoaxel and others added 5 commits March 28, 2017 20:21

Merge branch 'develop' into PR31

0480112

Merge branch 'develop' into PR31

bc7bb53

Merge branch 'develop' into PR31

f4aa408

Merge branch 'develop' into PR31

c0fa21d

Merge branch 'develop' into PR31

a3fd3b4

yotamberk merged commit 1ae1e15 into almende:develop Apr 14, 2017

wimrijnders deleted the PR31 branch April 14, 2017 10:56

bradh mentioned this pull request Apr 14, 2017

Adding width and height check #2436

Closed

wimrijnders mentioned this pull request Apr 15, 2017

[Network] Method "getNodeAt" is not working at click on selected node #1847

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Network: use separate refresh indicator in NodeBase, instead of width… #2885

Network: use separate refresh indicator in NodeBase, instead of width… #2885

wimrijnders commented Mar 21, 2017 •

edited by mojoaxel

wimrijnders commented Mar 21, 2017

mojoaxel Mar 21, 2017

mojoaxel Mar 21, 2017

wimrijnders Mar 22, 2017

mojoaxel Mar 21, 2017 •

edited

wimrijnders Mar 22, 2017

mojoaxel Mar 22, 2017

mojoaxel Mar 22, 2017

wimrijnders Mar 22, 2017

mojoaxel Mar 21, 2017

wimrijnders Mar 22, 2017

mojoaxel Mar 22, 2017

mojoaxel Mar 21, 2017

mojoaxel Mar 22, 2017

mojoaxel Mar 22, 2017 •

edited

wimrijnders Mar 23, 2017

wimrijnders Mar 23, 2017

wimrijnders Mar 23, 2017

wimrijnders commented Mar 24, 2017 •

edited

britonk1 commented Apr 13, 2018 •

edited

Network: use separate refresh indicator in NodeBase, instead of width… #2885

Network: use separate refresh indicator in NodeBase, instead of width… #2885

Conversation

wimrijnders commented Mar 21, 2017 • edited by mojoaxel

Discussion

Analysis

Solution

wimrijnders commented Mar 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mojoaxel Mar 21, 2017 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mojoaxel Mar 22, 2017 • edited

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

wimrijnders commented Mar 24, 2017 • edited

britonk1 commented Apr 13, 2018 • edited

wimrijnders commented Mar 21, 2017 •

edited by mojoaxel

mojoaxel Mar 21, 2017 •

edited

mojoaxel Mar 22, 2017 •

edited

wimrijnders commented Mar 24, 2017 •

edited

britonk1 commented Apr 13, 2018 •

edited