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

Infinite loop when width or height < padding. #5048

Closed
wants to merge 5 commits into from

Conversation

fanthos
Copy link
Contributor

@fanthos fanthos commented Dec 15, 2017

Simple fix #5047

@@ -172,6 +172,12 @@ module.exports = function(Chart) {
// Step 1
var chartWidth = width - leftPadding - rightPadding;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A simpler way to do this more inline with existing conventions would be:

var chartWidth = Math.min(width - leftPadding - rightPadding, 0);

@fanthos
Copy link
Contributor Author

fanthos commented Dec 27, 2017

Not sure this fixed by #5041 or not.

@jcopperfield
Copy link
Contributor

@benmccann This particular case has already been addressed. That is not to say that the layoutservice is perfect. It should probably sanitize all box and areas widths/heights to be 0 or greater. However that would require more than only checking the chartarea.

@benmccann
Copy link
Contributor

@fanthos this PR has a merge conflict and will need to be rebased

@jcopperfield could you clarify where this case is being addressed?

@jcopperfield
Copy link
Contributor

@benmccann The PR #5041 fixes the infinite loop in the time scale, by having getLabelCapacity always returning a value greater or equal to zero.
Like I said before, IMO the layout service should better check all sizes to be correct, i.e. all boxes, not just the chart width/height like proposed in this PR, so that's why I think this PR is not the correct way to solve this.

@benmccann
Copy link
Contributor

@fanthos Thanks for this effort. I'm going to close this PR since it cannot be merged due to the merge conflict and it seems the issue has already been addressed. Let us know if you still have the issue after 2.7.2 is released and we still need to fix it

@benmccann benmccann closed this Jan 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG]Infinite loop when canvas width = 0 and padding > 0
3 participants