-
Notifications
You must be signed in to change notification settings - Fork 19.7k
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
Fix/axis pointer multi #11648
Fix/axis pointer multi #11648
Conversation
(1) Fix that tooltip shows multiple values around both sides of the pointer. Fix #9829. (2) Fix a probably typo (diff < maxDistance) introduced by 4ef66e1 (3) Remove the previous logic to resolve #2869. Because currently if there are multiple values on the same axis tick, both of them will be found and displayed in tooltip by default. The related historical commits: 60d341f (fix #2869) 3426bd0 (support find multiple values on the same axis tick).
if (dist <= maxDistance) { | ||
// When the `value` is at the middle of `this.get(dim, i)` and `this.get(dim, i+1)`, | ||
// we'd better not push both of them to `nearestIndices`, otherwise it is easy to | ||
// get more than one item in `nearestIndices` (more specifically, in `tooltip`). |
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.
Does this mean we SHOULD NOT get more than one item
in this method?
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.
No. If there are more than one items on a axis ticks, we get more than one items.
If itemN --- pointer ---- itemN+1 we got only itemN, but not both itemN and itemN+1.
But the comment need to be updated to make it more understandable.
nearestIndicesLen = 0; | ||
} | ||
if (diff === minDiff) { | ||
nearestIndices[nearestIndicesLen++] = i; |
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.
Perhaps it's more clear to use push
instead of a self-increase index here.
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.
Just a little optimize to avoid always do arr.length = 0; arr.push(i)
.
I think it's OK since many self managed length is used in ArrayBuffer in this file ?
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.
Understand, didn't see the nearestIndicesLen
will be reset to 0 in line https://github.com/apache/incubator-echarts/blob/2eac3913216f4f9a462897a0da839a878d438dea/src/data/List.js#L1178
// get more than one item in `nearestIndices` (more specifically, in `tooltip`). | ||
// So we chose the one that `diff >= 0` in this csae. | ||
if (dist < minDist | ||
|| (dist === minDist && diff >= 0 && minDiff < 0) |
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.
Is this same to (dist === minDist && diff !== minDiff)
because diff
is dist
with sign
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.
Only when the current minDiff < 0
and diff >= 0
we clear the previous records.
That means that we choose diff >= 0
with higher priority than diff <= 0
.
nearestIndices.length = 0; | ||
nearestIndicesLen = 0; | ||
} | ||
if (diff === minDiff) { |
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'm confused about this condition. diff
should be always same with minDiff
if https://github.com/apache/incubator-echarts/blob/2eac3913216f4f9a462897a0da839a878d438dea/src/data/List.js#L1177 is executed.
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 previous condition
if (dist < minDist
|| (dist === minDist && diff >= 0 && minDiff < 0) { ... }
is not always entered, as the previous comment described.
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.
Can we get the following rule from these codes:
If the condition
dist < minDist || (dist === minDist && diff >= 0 && minDiff < 0)
is failed.
Then
dist < minDist
=>Math.abs(diff) !== Math.abs(minDiff)
=>diff !== minDiff
- Or
diff >= 0 && minDiff < 0
=>diff !== minDiff
Which means the diff === minDiff
condition will always be failed.
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.
minDiff
and minDist
will be modified if dist < minDist || (dist === minDist && diff >= 0 && minDiff < 0)
is true.
Take this test case for example:
function core({diff, minDiff}) {
console.log('---- minDiff:', minDiff, 'diff:', diff);
var dist = Math.abs(diff);
var minDist = Math.abs(minDiff);
if (dist < minDist
|| (dist === minDist && diff >= 0 && minDiff < 0)
) {
minDist = dist;
minDiff = diff;
nearestIndicesLen = 0;
console.log('Clear previous');
}
if (diff === minDiff) {
console.log('Add index');
}
else {
console.log('Not add index');
}
}
core({minDiff: 5, diff: -5})
core({minDiff: 5, diff: 5})
core({minDiff: -5, diff: 5})
core({minDiff: -5, diff: -5})
core({minDiff: 5, diff: 4})
core({minDiff: -5, diff: -4})
core({minDiff: 5, diff: 6})
core({minDiff: -5, diff: -6})
The result is:
---- minDiff: 5 diff: -5
Not add index
---- minDiff: 5 diff: 5
Add index
---- minDiff: -5 diff: 5
Clear previous
Add index
---- minDiff: -5 diff: -5
Add index
---- minDiff: 5 diff: 4
Clear previous
Add index
---- minDiff: -5 diff: -4
Clear previous
Add index
---- minDiff: 5 diff: 6
Not add index
---- minDiff: -5 diff: -6
Not add index
fix:
(1) Fix that tooltip shows multiple values around both sides of the pointer. Fix #9829.
(2) Fix a probably typo (diff < maxDistance) introduced by 4ef66e1
(3) Remove the previous logic that resolved #2869. Because currently if there are multiple values on the same axis tick, both of them will be found and displayed in tooltip by default. The related historical commits:
60d341f (fix #2869)
3426bd0 (support find multiple values on the same axis tick).