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

Fix/axis pointer multi #11648

Merged
merged 3 commits into from Nov 15, 2019
Merged

Fix/axis pointer multi #11648

merged 3 commits into from Nov 15, 2019

Conversation

100pah
Copy link
Member

@100pah 100pah commented Nov 14, 2019

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).

100pah added 2 commits Nov 14, 2019
(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`).
Copy link
Contributor

@pissang pissang Nov 15, 2019

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?

Copy link
Member Author

@100pah 100pah Nov 15, 2019

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;
Copy link
Contributor

@pissang pissang Nov 15, 2019

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.

Copy link
Member Author

@100pah 100pah Nov 15, 2019

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 ?

Copy link
Contributor

@pissang pissang Nov 15, 2019

Choose a reason for hiding this comment

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

// 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)
Copy link
Contributor

@pissang pissang Nov 15, 2019

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

Copy link
Member Author

@100pah 100pah Nov 15, 2019

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) {
Copy link
Contributor

@pissang pissang Nov 15, 2019

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.

Copy link
Member Author

@100pah 100pah Nov 15, 2019

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.

Copy link
Contributor

@pissang pissang Nov 15, 2019

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

  1. dist < minDist
    => Math.abs(diff) !== Math.abs(minDiff)
    => diff !== minDiff
  2. Or diff >= 0 && minDiff < 0
    => diff !== minDiff

Which means the diff === minDiff condition will always be failed.

Copy link
Member Author

@100pah 100pah Nov 15, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants