Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

addPoint() does not return point #10413

Closed
LaniMoo opened this issue Mar 25, 2019 · 9 comments · Fixed by #10476
Closed

addPoint() does not return point #10413

LaniMoo opened this issue Mar 25, 2019 · 9 comments · Fixed by #10476
Labels
Status: Stale This issue hasn't had any activity for a while, and will be auto-closed if no further updates occur. Type: Enhancement

Comments

@LaniMoo
Copy link

LaniMoo commented Mar 25, 2019

The one-liner return point; is missing from addPoint() so it returns nothing. Please hear out my use-case:

Since there is no way to toggle visibility of categories, I must remove the points manually to "hide" them. If storing the added points were possible:

Expected behaviour

// Adding data
let points = [];

for (let value of category1.data) {
   // Store the added point!!
   let point = series.addPoint({
      name: 'Category 1',
      y: value
   });

   points.push(point);
}

Then I would be able to do this:

// Hiding a category
for (let point of points) {
   point.remove();
}

Actual behaviour

But instead I have to iterate through the entire series to find the points to remove for one category:

for (let points of series.points) {
   if (point.name === 'Category1') {
      point.remove();
   }
}

Live demo with steps to reproduce

https://jsfiddle.net/lanimelrok/1b6z8vqk/

Looking at the source code, it looks like just a one-liner to return the point. Could we please return the added point so that I won't have to iterate through possibly hundreds of points in a series just to delete a few? Thank you so much for your understanding.

addPoint: function (options, redraw, shift, animation) {
var series = this,
seriesOptions = series.options,
data = series.data,
chart = series.chart,
xAxis = series.xAxis,
names = xAxis && xAxis.hasNames && xAxis.names,
dataOptions = seriesOptions.data,
point,
isInTheMiddle,
xData = series.xData,
i,
x;
// Optional redraw, defaults to true
redraw = pick(redraw, true);
// Get options and push the point to xData, yData and series.options. In
// series.generatePoints the Point instance will be created on demand
// and pushed to the series.data array.
point = { series: series };
series.pointClass.prototype.applyOptions.apply(point, [options]);
x = point.x;
// Get the insertion point
i = xData.length;
if (series.requireSorting && x < xData[i - 1]) {
isInTheMiddle = true;
while (i && xData[i - 1] > x) {
i--;
}
}
// Insert undefined item
series.updateParallelArrays(point, 'splice', i, 0, 0);
// Update it
series.updateParallelArrays(point, i);
if (names && point.name) {
names[x] = point.name;
}
dataOptions.splice(i, 0, options);
if (isInTheMiddle) {
series.data.splice(i, 0, null);
series.processData();
}
// Generate points to be added to the legend (#1329)
if (seriesOptions.legendType === 'point') {
series.generatePoints();
}
// Shift the first point off the parallel arrays
if (shift) {
if (data[0] && data[0].remove) {
data[0].remove(false);
} else {
data.shift();
series.updateParallelArrays(point, 'shift');
dataOptions.shift();
}
}
// redraw
series.isDirty = true;
series.isDirtyData = true;
if (redraw) {
chart.redraw(animation); // Animation is set anyway on redraw, #5665
}
},

@pawelfus
Copy link
Contributor

Hi @LaniMoo

Thanks for sharing the idea! I agree with you, it would be useful to get that point. Just like we do with addSeries and addAxis.

@LaniMoo
Copy link
Author

LaniMoo commented Mar 26, 2019

Wonderful!! Thanks for your understanding, @pawelfus!!!!

@LaniMoo
Copy link
Author

LaniMoo commented Jun 4, 2019

Bummer, I see it got reverted. Any chance return point; will be added back any time soon? Thanks!

@pawelfus
Copy link
Contributor

pawelfus commented Jun 5, 2019

No ETA @LaniMoo - we will try to add this in the next release.

@TorsteinHonsi
Copy link
Collaborator

The reason we're not returning the point is that there are cases where a Point object is actually not generated. For example, in a Highstock chart with data grouping, when running addPoint adds the point configuration to a list, but Point instances actually reflect groups of data points. So there is no guarantee that when you run addPoint it will result in a Point instance.

Another case is if we have a chart that is zoomed to, say X values 0-500. If you add a point at x=1000, we don't generate a Point instance until the user zooms out to see it.

Bottom line, as a CPU/memory optimization, Point instances are only created when needed.

I see two viable solutions:

  1. Return the index of the point. This lets you look up the point by series.points[index], or run series.removePoint(index) directly. A caveat is that indexes may change as other points are added or removed. See this demo, where the indices are mapped to Points immediately after generation.
  2. Supply a callback to run when the point is actually generated. This is perhaps a bit more complicated to implement, but avoids the problem of keeping track of changing indices.

@pawelfus

@TorsteinHonsi
Copy link
Collaborator

Or a third solution could be to either add a series event on point generated, or refactor the existing addPoint to not run until there's an actual Point instance to pass over.

@pawelfus
Copy link
Contributor

pawelfus commented Jun 5, 2019

Thanks for the details! Yup, that makes sense now. How about simply returning undefined when actual point was not created? I think potential issues with index are not worth implementation.

This suppose to be a simple fix, but given the complexity of the issue, we may delay this. Honestly, this sounds more like a promise use case. We can try to implement something similar (like your other proposed solutions), but I'm not sure if it's worth it.

@TorsteinHonsi
Copy link
Collaborator

I agree returning a promise would be the most natural solution. Returning the index would help, but would need to come with a warning. Maybe it's best to leave it for now, as the points can already be found using id.

@stale
Copy link

stale bot commented Dec 4, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions!

@stale stale bot added the Status: Stale This issue hasn't had any activity for a while, and will be auto-closed if no further updates occur. label Dec 4, 2020
@highsoft-bot highsoft-bot added this to To do in Development-Flow via automation Dec 4, 2020
@stale stale bot closed this as completed Dec 12, 2020
Development-Flow automation moved this from To do to Done Dec 12, 2020
@Izothep Izothep removed this from Done in Development-Flow Jul 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Stale This issue hasn't had any activity for a while, and will be auto-closed if no further updates occur. Type: Enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants