Refactor autoskip functionality into a separate method #4614

benmccann · 2017-08-04T01:21:57Z

This is the first step towards improving the auto-skip functionality. I haven't attempted to make any functionality changes in this PR. My only goal here is to refactor the functionality into a self-contained method as a starting point towards further improvements.

I also haven't moved the calculation to happen outside of draw. I'm not confident I know this section of the code well enough yet to do that safely and would prefer to save that for later when I have a better understanding and am sure the change is safe.

simonbrunel · 2017-08-04T19:50:22Z

src/core/core.scale.js

+		 * Returns a subset of ticks to be plotted.
+		 * This functionality makes it so that we don't have overlapping labels.
+		 */
+		autoSkip: function() {


I would call it _disseminate and make it explicitly @private. I would also add the ticks as parameter

I had debated making ticks a parameter, so I've changed that since you've suggested it as well

I had also though about making it private, but I think that it's a method that users are potentially likely to want to override with their own implementation

I'm not a big fan of disseminate as a name. We're not really spreading the ticks, but rather are skipping some

I'm not a big fan of autoSkip, but actually was thinking of _decimate (not _disseminate). We should make it private until there is a need/request for it to be public, we don't want to maintain this kind of backward compatibility.

simonbrunel · 2017-08-04T20:02:53Z

src/core/core.scale.js

+				// if they defined a max number of optionTicks,
+				// increase skipRatio until that number is met
+				if (maxTicks && tickCount > maxTicks) {
+					while (!skipRatio || tickCount / (skipRatio || 1) > maxTicks) {


Do we really need a loop here? Maybe something like (not tested):

skipRatio = Math.max(skipRatio, ~~(tickCount/maxTicks));

I didn't want to change the logic too much in this PR. I figure it's probably all going to be mostly rewritten, so not worth investing in. And also I want to minimize changes before adding some more tests to ensure the logic is right. This PR helps work towards that by structuring the code so that it will be easier to test

We don't know how long it will take to get it fully rewritten, neither if it will land in 2.7.0. Let's take time to cleanup the code while refactoring it. Else there is no need for this kind of PR and I would prefer to merge the final one with a full overview of the new implementation.

Of course, that only applies if my suggested formula correctly works :)

done. yeah, your formula is correct

simonbrunel · 2017-08-04T20:03:42Z

src/core/core.scale.js

+				}
+			}
+
+			helpers.each(me._ticks, function(tick, index) {


I would convert this each into a regular for() loop since there might be potentially lot of ticks.

simonbrunel · 2017-08-04T22:08:11Z

src/core/core.scale.js

+			var cosRotation = Math.cos(labelRotationRadians);
+			var longestRotatedLabel = me.longestLabelWidth * cosRotation;
+			var result = [];
+			var ilen = ticks.length;


Let's choose between ilen or tickCount :)

done. good catch

etimberg · 2017-08-04T22:23:44Z

For context, the branch where I played around with tick alignment is https://github.com/chartjs/Chart.js/blob/vertical-tick-alignment/src/core/core.scale.js. There are some changes in calculateTickRotation. There are also some changes to try and work on the auto-skipper. It's definitely not production ready, but could be a useful data point

etimberg

I think this is a good intermediate step towards cleaning all this up

simonbrunel · 2017-08-07T07:17:42Z

src/core/core.scale.js

+				// Since we always show the last tick,we need may need to hide the last shown one before
+				var shouldSkip = (skipRatio > 1 && i % skipRatio > 0) || (i % skipRatio === 0 && i + skipRatio >= tickCount);
+				if (shouldSkip && !isLastTick) {
+					return;


continue?

thanks. missed that one when converting from forEach

simonbrunel · 2017-08-07T07:18:23Z

src/core/core.scale.js

+
+				// Since we always show the last tick,we need may need to hide the last shown one before
+				var shouldSkip = (skipRatio > 1 && i % skipRatio > 0) || (i % skipRatio === 0 && i + skipRatio >= tickCount);
+				if (shouldSkip && !isLastTick) {


Why || helpers.isNullOrUndef(label) is not needed here?

hmm. I can't remember anymore. I've added it back. I really don't care about improving this code as I want to rewrite it all

I really care about everything that is merged in master :)

simonbrunel · 2017-08-07T07:20:26Z

src/core/core.scale.js

+
+				// Since we always show the last tick,we need may need to hide the last shown one before
+				var shouldSkip = (skipRatio > 1 && i % skipRatio > 0) || (i % skipRatio === 0 && i + skipRatio >= tickCount);
+				if (shouldSkip && !isLastTick) {


Can we replace isLastTick by i < tickCount - 1 and remove the isLastTick variable?

simonbrunel · 2017-08-07T07:21:00Z

src/core/core.scale.js

+				if (shouldSkip && !isLastTick) {
+					return;
+				}
+				result.push(tick);


Can we replace tick by ticks[i] and remove the tick variable? (except if the test on label is needed)

I added back the test on label. It actually does look like it should be there

simonbrunel · 2017-08-07T07:22:10Z

src/core/core.scale.js

+			}
+
+			for (i = 0; i < tickCount; i++) {
+				var tick = ticks[i];


Can we move all variable declarations outside the loop?

simonbrunel · 2017-08-07T10:19:30Z

src/core/core.scale.js

+
+				// Since we always show the last tick,we need may need to hide the last shown one before
+				shouldSkip = (skipRatio > 1 && i % skipRatio > 0) || (i % skipRatio === 0 && i + skipRatio >= tickCount);
+				if (shouldSkip && i !== tickCount - 1 || helpers.isNullOrUndef(label)) {


You don't need this intermediary label variable, you can use tick.label directly here.

simonbrunel

A minor cleanup (label) then good to be merged.

benmccann force-pushed the autoskip branch 2 times, most recently from abb0d05 to edfaaf7 Compare August 4, 2017 01:55

simonbrunel requested changes Aug 4, 2017

View reviewed changes

simonbrunel requested a review from etimberg August 4, 2017 20:04

benmccann force-pushed the autoskip branch from edfaaf7 to 4a1b0fb Compare August 4, 2017 21:18

simonbrunel reviewed Aug 4, 2017

View reviewed changes

benmccann force-pushed the autoskip branch 4 times, most recently from ebd0857 to 8dd3dfc Compare August 5, 2017 15:47

etimberg approved these changes Aug 7, 2017

View reviewed changes

simonbrunel requested changes Aug 7, 2017

View reviewed changes

benmccann force-pushed the autoskip branch from 8dd3dfc to aa5dc03 Compare August 7, 2017 07:56

simonbrunel reviewed Aug 7, 2017

View reviewed changes

simonbrunel approved these changes Aug 7, 2017

View reviewed changes

Refactor autoskip functionality into a separate method

a551002

benmccann force-pushed the autoskip branch from aa5dc03 to a551002 Compare August 7, 2017 16:50

simonbrunel added this to the Version 2.7 milestone Aug 7, 2017

simonbrunel added the type: enhancement label Aug 7, 2017

simonbrunel merged commit 1eeffa3 into chartjs:master Aug 7, 2017

yofreke pushed a commit to yofreke/Chart.js that referenced this pull request Dec 30, 2017

Refactor autoskip functionality into a separate method (chartjs#4614)

22d76d3

benmccann deleted the autoskip branch February 10, 2019 05:54

exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021

Refactor autoskip functionality into a separate method (chartjs#4614)

eb93d25

Refactor autoskip functionality into a separate method #4614

Refactor autoskip functionality into a separate method #4614

Uh oh!

Conversation

benmccann commented Aug 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonbrunel Aug 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benmccann Aug 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etimberg commented Aug 4, 2017

Uh oh!

etimberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonbrunel left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

benmccann commented Aug 4, 2017 •

edited

Loading

simonbrunel Aug 4, 2017 •

edited

Loading

benmccann Aug 4, 2017 •

edited

Loading