Skip to content
This repository was archived by the owner on Jul 29, 2019. It is now read-only.

Graph3d setOptions throws exception setData does not exist #3251

Closed
DaveWare opened this issue Jul 12, 2017 · 7 comments
Closed

Graph3d setOptions throws exception setData does not exist #3251

DaveWare opened this issue Jul 12, 2017 · 7 comments
Assignees

Comments

@DaveWare
Copy link

DaveWare commented Jul 12, 2017

I just started trying to use Graph3d yesterday and used the available example code 10_styling.html to reproduce the excpetion. Btw; nice work! I changed the example to call setBarWidth() when x or y width is changed.

I cloned the latest repository and am using the distribution:

I wanted to allow the users to dynamically set the value range for coloring. When I pass updated options to graph3d.setOptions() an exception is throw from DataGroup.reload line 13850 that says DataGroup does not have a setData method. I changed reload to take a graph3d as an argument and called graph3d.setData(this.dataTable) rather than this.setData(dataTable). This seems to have the correct behavior. The graph updates and has the new value range. Or in the case of the example code below the correct bar width.

Not sure if this would be the proper fix as I have just started looking at your component.

Object doesn't support property or method 'set

  /**
   * Reload the data
   */
  DataGroup.prototype.reload = function () {
    if (this.dataTable) {
      this.setData(this.dataTable);
    }
  };

10_styling.html
10_styling.txt

@wimrijnders
Copy link
Contributor

wimrijnders commented Jul 12, 2017

OK, great, a Graph3D issue. I would be happy to help, but would you mind cleaning up your comment a bit? Seems like some text is missing. I cleaned it up as far as I could.

@wimrijnders
Copy link
Contributor

I think you're on to something here. I must have missed this in the refactoring to DataSet. I'll look at it in depth tomorrow. Thanks for reporting!

@wimrijnders
Copy link
Contributor

wimrijnders commented Jul 13, 2017

Confirmed. This is a silly oversight. I owe you a beer for finding this.

Your fix works, but I find it a very roundabout way of setting the data. I propose the following, which will do exactly the same thing:

/**
 * Update the options. Options will be merged with current options
 *
 * @param {Object} options
 */
Graph3d.prototype.setOptions = function (options) {
...
  // re-load the data
  //this.dataGroup.reload();
  var dataTable = this.dataGroup.getDataTable();
  if (dataTable) {
    this.setData(dataTable);
  }
...
};

This gets rid of the dependency to the Graph3D instance within DataGroup. In fact, DataGroup.reload() can just plain die now.

I'll make a PR for this. Again, thanks for reporting.

@wimrijnders wimrijnders self-assigned this Jul 13, 2017
@DaveWare
Copy link
Author

DaveWare commented Jul 13, 2017 via email

@wimrijnders
Copy link
Contributor

My pleasure, but for completeness I will state that what I offered you was a workaround; the fix is forthcoming and will be in the next release.

The circular dependency mine introduced was not good.

Indeed, but that was not your fault, the circular thing was already there. Best to kill it right away, before my eyes start to bleed.

wimrijnders added a commit to wimrijnders/vis that referenced this issue Jul 14, 2017
Fix for almende#3251

A reference was missed in DataGroup.reload() during refactoring.
Rather than fix this method, it has been removed and the logic moved to `Graph3d`.
This makes for somewhat cleaner code.
@DaveWare
Copy link
Author

DaveWare commented Jul 14, 2017 via email

@wimrijnders
Copy link
Contributor

wimrijnders commented Jul 15, 2017

Thank you for your attention; I see the problem, it's in DataGroup.initializeData() where _onChange() is stopped before the guard checks for input run. I'll take this along in the pending PR I'll make a new PR for this.

I would say that your solution is appropriate here. The reason that incremental updates take longer is that the graph is redrawn after every change. There are several ways around this, but your solution is by far the simplest.

wimrijnders added a commit to wimrijnders/vis that referenced this issue Jul 15, 2017
This adds a unit test for PR almende#3255 which fixes almende#3251.
The unit test will fail without the PR merged.

**NOTE:** This also adds module `canvas`, required for the unit test. This module
proved to be quite fickly to install properly. During reviewing, please pay special
attention to the proper installation of this modul. I.e. do following to test:

```
> npm install                      # If no errors, continue
> npm test /tests/Graph3D.test.js  # Run unit test isolated
```
yotamberk pushed a commit that referenced this issue Jul 16, 2017
* Fix missing reference to Graph3D instance in DataGroup

Fix for #3251

A reference was missed in DataGroup.reload() during refactoring.
Rather than fix this method, it has been removed and the logic moved to `Graph3d`.
This makes for somewhat cleaner code.

* Fixes due to review
wimrijnders added a commit to wimrijnders/vis that referenced this issue Jul 19, 2017
**Note:** This is a small fix and should be easy to review.

Second fix for almende#3251.

In method `DataGroup.initializeData()`, if the passed `rawData` is bad for some reason,
it was possible to lose the subscriptions due to early return from the method.

This fix changes the order in the method so that the guard clauses execute *before* the
subscription is changed.
yotamberk pushed a commit that referenced this issue Jul 20, 2017
**Note:** This is a small fix and should be easy to review.

Second fix for #3251.

In method `DataGroup.initializeData()`, if the passed `rawData` is bad for some reason,
it was possible to lose the subscriptions due to early return from the method.

This fix changes the order in the method so that the guard clauses execute *before* the
subscription is changed.
yotamberk pushed a commit that referenced this issue Jul 20, 2017
* Add unit tests for Graph3D issue

This adds a unit test for PR #3255 which fixes #3251.
The unit test will fail without the PR merged.

**NOTE:** This also adds module `canvas`, required for the unit test. This module
proved to be quite fickly to install properly. During reviewing, please pay special
attention to the proper installation of this modul. I.e. do following to test:

```
> npm install                      # If no errors, continue
> npm test /tests/Graph3D.test.js  # Run unit test isolated
```

* Fix for travis-cl

* Add giflib to travis test definition

* Add libgif to travis test definition - take 2

* Proper setup and teardown for jsdom-global

* Minor fixes and cleanup
agnesnanxin pushed a commit to agnesnanxin/vis that referenced this issue Aug 30, 2017
* Fix missing reference to Graph3D instance in DataGroup

Fix for almende#3251

A reference was missed in DataGroup.reload() during refactoring.
Rather than fix this method, it has been removed and the logic moved to `Graph3d`.
This makes for somewhat cleaner code.

* Fixes due to review
primozs pushed a commit to primozs/vis that referenced this issue Jan 3, 2019
* Fix missing reference to Graph3D instance in DataGroup

Fix for almende#3251

A reference was missed in DataGroup.reload() during refactoring.
Rather than fix this method, it has been removed and the logic moved to `Graph3d`.
This makes for somewhat cleaner code.

* Fixes due to review
primozs pushed a commit to primozs/vis that referenced this issue Jan 3, 2019
**Note:** This is a small fix and should be easy to review.

Second fix for almende#3251.

In method `DataGroup.initializeData()`, if the passed `rawData` is bad for some reason,
it was possible to lose the subscriptions due to early return from the method.

This fix changes the order in the method so that the guard clauses execute *before* the
subscription is changed.
primozs pushed a commit to primozs/vis that referenced this issue Jan 3, 2019
* Add unit tests for Graph3D issue

This adds a unit test for PR almende#3255 which fixes almende#3251.
The unit test will fail without the PR merged.

**NOTE:** This also adds module `canvas`, required for the unit test. This module
proved to be quite fickly to install properly. During reviewing, please pay special
attention to the proper installation of this modul. I.e. do following to test:

```
> npm install                      # If no errors, continue
> npm test /tests/Graph3D.test.js  # Run unit test isolated
```

* Fix for travis-cl

* Add giflib to travis test definition

* Add libgif to travis test definition - take 2

* Proper setup and teardown for jsdom-global

* Minor fixes and cleanup
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants