fix(custom): fix elements may not be removed after updates #17333 #17349
Conversation
|
Thanks for your contribution! The pull request is marked to be |
|
I'm not pretty sure but I think it's by design at original. Like the logic of option merging, giving But from my side. I think the change in this PR will bring much more intuitionistic code. So I will give +1 on this change(and a bit more explanation on the doc will be better). The only change I need to request is that we still need to |
src/chart/custom/CustomView.ts
Outdated
| ); | ||
| } | ||
| else { | ||
| removeChildFromGroup(el, oldChild, seriesModel); |
There was a problem hiding this comment.
If "leave transition animation" is disabled, it will still call parent.remove(el) directly, which uses splice internally and brings a weird behavior:
existing children: [elA, elB, elC]
new children: [elM, null, elN]
result: [merge(elA, elM), elC, elN]
I've changed my opinion that
Basically I agree this change, which might make it more intuitive
When setOption, the default strategy is "merge", which is the same as the other series like line, scatter, ...
The merge strategy in custom series contains:
- dataItems are merged by index
- group children are merge by index
In series link line, scatter, graphics are the same between different data items, and it makes "merge" strategy make sense in most cases. But is custom series, graphics may not be the same between different data items, which probably make the "merge" not reasonable.
And consider the case #17349 , with the default "merge" strategy, we can get the transition animation:
But the animation seams not make sense: the data item "a" and the data item "b" has no relationship at all, they are in different month and different date. The animation happens only because they happen to use the same graphic.
This change brings a breaking change but only change the null case in group children but not considered the case like "merge graphic unexpectedly of different data item", "remain properties unexpectedly when merge graphic". I think it is not good enough.
Could we provide a option on custom series like
series: {
type: 'custom',
merge: false,
}When merge: false, there is no merge either between different data items (see https://github.com/apache/echarts/blob/5.3.3/src/chart/custom/CustomView.ts#L222)
or in group children.
And it may fix #17349 thoroughly ?
There was a problem hiding this comment.
I think this mainly depends on how we understand merging. I don't think it's intuitive to use the old element in a group when provided null in new setOption. This is just like when providing series data like [10, 20, null, 40] in a new setOption should not take the null data to use the old value in the previous data.
There was a problem hiding this comment.
I think at least this behavior is not good (by the current modification): 🤔
old children: [elA, elB, elC] (length: 3)
new children: [elM, null, elN] (length: 3)
- if transition animation enabled:
- result:
[merge(elA, elM), merge(elC, elN)](length: 2, which is different from neither the old children nor the input new children)
- result:
- if transition animation disabled:
- result:
[merge(elA, elM), elC, elN](length: 3, but the index ofelCis changed from2to1)
- result:
Both of them seems not expectable and intuitive for users.
(Do I misunderstand with the modified code?)
There was a problem hiding this comment.
You are quite right! I didn't think of the situation. I fixed this and also update the test cases to include with/without animation.
There was a problem hiding this comment.
After adding oldRemovedCount the effect is better but still has some issues:
Consider the case below:
- step 1.
setOptionwith new children:[elA, elB, elC](length: 3) - step 2.
setOptionwith new children:[elA, null, elC](length: 3) (means hideelB) - step 3.
setOptionwith new children:[elA, elB, elC](length: 3) (means showelBagain)
But with current modified code, the result will be:
- After step 1. children will be
[elA, elB, elC] - After step 2. children will be
[elA, elC] - After step 3. children will be
[elA, merge(elC, elB), elC]
which is unexpected.
Consider leave animation
If the setOption is triggered by user's repeat click, the interval between two clicks is not predicable, may be longer or shorter than the leave animation duration. In this case, if the indices of some children is different before and after leave animation, the merge result will be not predicable.
Basically, if we use null/undefined to delete a child, I think this principle should be applied:
The index of any other element should not be changed after this deletion
That is, is we delete a child, that place should became "no element"(probably not supported in current zrender) or "an placeholder element that can not displayed"
src/chart/custom/CustomView.ts
Outdated
| ); | ||
| } | ||
| else { | ||
| removeChildFromGroup(el, oldChild, seriesModel); |
There was a problem hiding this comment.
After adding oldRemovedCount the effect is better but still has some issues:
Consider the case below:
- step 1.
setOptionwith new children:[elA, elB, elC](length: 3) - step 2.
setOptionwith new children:[elA, null, elC](length: 3) (means hideelB) - step 3.
setOptionwith new children:[elA, elB, elC](length: 3) (means showelBagain)
But with current modified code, the result will be:
- After step 1. children will be
[elA, elB, elC] - After step 2. children will be
[elA, elC] - After step 3. children will be
[elA, merge(elC, elB), elC]
which is unexpected.
Consider leave animation
If the setOption is triggered by user's repeat click, the interval between two clicks is not predicable, may be longer or shorter than the leave animation duration. In this case, if the indices of some children is different before and after leave animation, the merge result will be not predicable.
Basically, if we use null/undefined to delete a child, I think this principle should be applied:
The index of any other element should not be changed after this deletion
That is, is we delete a child, that place should became "no element"(probably not supported in current zrender) or "an placeholder element that can not displayed"
| const newChild = newChildren[index]; | ||
| const oldChild = el.childAt(index); | ||
| if (newChild) { | ||
| if (newChild.ignore == null) { |
There was a problem hiding this comment.
Should be:
if (newChild.ignore) {
newChild.ignore = false;
}or
newChild.ignore = null; // or falseThere was a problem hiding this comment.
newChild is the user-provided option. So if newChild.ignore is true, it means the user want to ignore the element so I don't think we should set newChild.ignore = false in this situation.
There was a problem hiding this comment.
What if users set a child as null / undefined at the first time ? (rather than the second time)
| const newChild = newChildren[index]; | ||
| const oldChild = el.childAt(index); | ||
| if (newChild) { | ||
| if (newChild.ignore == null) { |
|
Congratulations! Your PR has been merged. Thanks for your contribution! 👍 |
| @@ -99,6 +99,7 @@ import { | |||
| applyKeyframeAnimation, | |||
| stopPreviousKeyframeAnimationAndRestore | |||
| } from '../../animation/customGraphicKeyframeAnimation'; | |||
| import { SeriesModel } from '../../echarts.all'; | |||
There was a problem hiding this comment.
The import source is incorrect.
doc: update instruction for custom group being null apache/echarts#17349
Brief Information
This pull request is in the type of:
What does this PR do?
When custom series is updated, if the
renderItemreturns a group that contains null for an element that was previously in the group, it will not be removed, causing some of the elements leave behind by mistake.Fixed issues
#17333
Details
Before: What was the problem?
There is an extra element after calling
setOptionfor the second time.After: How does it behave after the fixing?
No extra element after calling
setOptionfor the second time.Document Info
One of the following should be checked.
Misc
ZRender Changes
Related test cases or examples to use the new APIs
custom-update.htmlOthers
Merging options
Other information