-
Notifications
You must be signed in to change notification settings - Fork 397
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
AxisTickCalculator_Date: Make sure we exit if we tried all possible timespans #717
Conversation
@timmolter Any chance to get this merged and released? Would be great to be able to use the new version but unfortunately this is a blocker for us. |
@mccartney Since you apparently recently merged some PRs, could you please have a look at this one aswell and merge it? |
if(index >= timeSpans.size() - 1) | ||
break; // We reached the end, even though we might not yet have the result we want, | ||
// continuing will lead to an Exception! | ||
} while (skip | ||
|| !areAllTickLabelsUnique(tickLabels) | ||
|| !willLabelsFitInTickSpaceHint(tickLabels, gridStepInChartSpace)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A tiny remark is that a break at the end of the loop can likely be replaced by changing the condition in while
. Can you try that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition will get a lot more complicated and the explanation comment why this is necessary probably needs to be removed. Are you sure that's a good idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure, but in general the readability of that loop is not perfect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can just merge as is, and then clean-up the code afterwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have an idea when there will be a new release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't. AFAIK @timmolter is the only one who can issue a new release.
@mccartney Would now be a good time for a release in your opinion? |
Yes. If possible, please do the needful. |
done. 3.8.4 is out. |
Unfortunately I don't know how to add a proper test for this but in #601 there is test data that can be used for that.
Fixes #601