fix(custom): fix elements may not be removed after updates #17333 by Ovilia · Pull Request #17349 · apache/echarts

Ovilia · 2022-07-11T09:33:38Z

Brief Information

This pull request is in the type of:

bug fixing
new feature
others

What does this PR do?

When custom series is updated, if the renderItem returns 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 setOption for the second time.

After: How does it behave after the fixing?

No extra element after calling setOption for the second time.

Document Info

One of the following should be checked.

This PR doesn't relate to document changes
The document should be updated later
The document changes have been made in doc: update instruction for custom group being null apache/echarts#17349 echarts-doc#270

Misc

ZRender Changes

This PR depends on ZRender changes (ecomfe/zrender#xxx).

Related test cases or examples to use the new APIs

added a test case custom-update.html
tested and passed the visual tests of current cases with "custom" in their names

Others

Merging options

Please squash the commits into a single one when merging.

Other information

echarts-bot · 2022-07-11T09:34:00Z

Thanks for your contribution!
The community will review it ASAP. In the meanwhile, please checkout the coding standard and Wiki about How to make a pull request.

The pull request is marked to be PR: author is committer because you are a committer of this project.

pissang · 2022-07-11T16:08:48Z

I'm not pretty sure but I think it's by design at original. Like the logic of option merging, giving null means not updating this element in the default merge mode. If we wan't to remove it, we need to change the $mergeChildren to false, which will loose the transition. Perhaps @100pah can give more explanation on this.

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 applyLeaveTransition when removing the child instead of using remove directly

src/chart/custom/CustomView.ts

Ovilia · 2022-07-12T08:47:55Z

@pissang @100pah Thanks for the review. I updated the comment and add a test case where a child of new group returned from the custom series is {}, which should preserve the old element. I will also update the document about this.

100pah · 2022-07-12T11:10:09Z

src/chart/custom/CustomView.ts

+            );
+        }
+        else {
+            removeChildFromGroup(el, oldChild, seriesModel);


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 ?

@Ovilia @pissang what's your opinion?

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.

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)

if transition animation disabled:

result: [merge(elA, elM), elC, elN] (length: 3, but the index of elC is changed from 2 to 1 )

Both of them seems not expectable and intuitive for users.
(Do I misunderstand with the modified code?)

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.

After adding oldRemovedCount the effect is better but still has some issues:

Consider the case below:

step 1. setOption with new children: [elA, elB, elC] (length: 3)

step 2. setOption with new children: [elA, null, elC] (length: 3) (means hide elB)

step 3. setOption with new children: [elA, elB, elC] (length: 3) (means show elB again)

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"

100pah · 2022-07-14T16:45:38Z

src/chart/custom/CustomView.ts

+            );
+        }
+        else {
+            removeChildFromGroup(el, oldChild, seriesModel);


After adding oldRemovedCount the effect is better but still has some issues:

Consider the case below:

step 1. setOption with new children: [elA, elB, elC] (length: 3)

step 2. setOption with new children: [elA, null, elC] (length: 3) (means hide elB)

step 3. setOption with new children: [elA, elB, elC] (length: 3) (means show elB again)

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"

… elements

100pah · 2022-07-28T08:11:42Z

src/chart/custom/CustomView.ts

+        const newChild = newChildren[index];
+        const oldChild = el.childAt(index);
+        if (newChild) {
+            if (newChild.ignore == null) {


Should be:

if (newChild.ignore) { newChild.ignore = false; }

or

newChild.ignore = null; // or false

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.

I got it wrong

What if users set a child as null / undefined at the first time ? (rather than the second time)

100pah · 2022-07-29T03:38:47Z

src/chart/custom/CustomView.ts

+        const newChild = newChildren[index];
+        const oldChild = el.childAt(index);
+        if (newChild) {
+            if (newChild.ignore == null) {


I got it wrong

echarts-bot · 2022-07-29T03:40:13Z

Congratulations! Your PR has been merged. Thanks for your contribution! 👍

plainheart · 2022-07-29T03:47:58Z

src/chart/custom/CustomView.ts

    applyKeyframeAnimation,
    stopPreviousKeyframeAnimationAndRestore
 } from '../../animation/customGraphicKeyframeAnimation';
+import { SeriesModel } from '../../echarts.all';


The import source is incorrect.

doc: update instruction for custom group being null apache/echarts#17349

fix(custom): fix elements may not be removed after updates #17333

bc4ee6c

echarts-bot bot added PR: author is committer PR: awaiting review PR: doc unchanged labels Jul 11, 2022

pull-request-size bot added the size/S label Jul 11, 2022

Ovilia linked an issue Jul 11, 2022 that may be closed by this pull request

[Bug] Custom series graphic elements rendering bug after updating #17333

Closed

Ovilia added this to the 5.4 milestone Jul 11, 2022

100pah requested changes Jul 12, 2022

View reviewed changes

src/chart/custom/CustomView.ts Outdated Show resolved Hide resolved

echarts-bot bot added PR: revision needed and removed PR: awaiting review labels Jul 12, 2022

100pah requested changes Jul 12, 2022

View reviewed changes

src/chart/custom/CustomView.ts Outdated Show resolved Hide resolved

Ovilia added 2 commits July 12, 2022 16:44

fix(custom): apply leave transition and add more comments

22c8218

test(custom): add a test case with {} that should preserve child

ee3b835

echarts-bot bot removed the PR: revision needed label Jul 12, 2022

pull-request-size bot removed the size/S label Jul 12, 2022

echarts-bot bot added the PR: awaiting review label Jul 12, 2022

pull-request-size bot added the size/M label Jul 12, 2022

Ovilia added a commit to apache/echarts-doc that referenced this pull request Jul 12, 2022

doc: update instruction for custom group being null apache/echarts#17349

687b006

Ovilia mentioned this pull request Jul 12, 2022

doc: update instruction for custom group being null apache/echarts#17349 apache/echarts-doc#270

Merged

echarts-bot bot added PR: doc ready and removed PR: doc unchanged labels Jul 12, 2022

100pah requested changes Jul 12, 2022

View reviewed changes

echarts-bot bot added PR: revision needed and removed PR: awaiting review labels Jul 12, 2022

test(custom): add a test case

fc43e0a

echarts-bot bot added PR: awaiting review and removed PR: revision needed labels Jul 13, 2022

fix(custom): fix the case for element after the null child

44e650e

100pah requested changes Jul 14, 2022

View reviewed changes

echarts-bot bot added PR: revision needed and removed PR: awaiting review labels Jul 14, 2022

Ovilia added 3 commits July 25, 2022 15:58

fix(custom): ignore element when renderItem returns a group with null…

8fc48c7

… elements

test(custom): update test case for custom update

09e0c80

Merge branch 'master' into fix-17333

d9d00b8

echarts-bot bot added PR: awaiting review and removed PR: revision needed labels Jul 25, 2022

100pah requested changes Jul 28, 2022

View reviewed changes

echarts-bot bot added PR: revision needed and removed PR: awaiting review labels Jul 28, 2022

100pah approved these changes Jul 29, 2022

View reviewed changes

echarts-bot bot removed the PR: revision needed label Jul 29, 2022

Ovilia merged commit c584377 into master Jul 29, 2022

Ovilia deleted the fix-17333 branch July 29, 2022 03:40

plainheart reviewed Jul 29, 2022

View reviewed changes

Ovilia mentioned this pull request Aug 1, 2022

fix(custom): set ignore only if old child exists #17450

Merged

8 tasks

Ovilia added a commit to apache/echarts-doc that referenced this pull request Aug 1, 2022

Merge pull request #270 from apache/fix-custom-merge

847e47c

doc: update instruction for custom group being null apache/echarts#17349

plainheart mentioned this pull request Aug 1, 2022

chore: fix wrong imports from echarts.all #17452

Merged

3 tasks

Ovilia modified the milestones: 5.4, 5.4.0 Sep 1, 2022

Ovilia mentioned this pull request Sep 16, 2022

doc: coarse pointer apache/echarts-doc#287

Closed

Conversation

Ovilia commented Jul 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Brief Information

What does this PR do?

Fixed issues

Details

Before: What was the problem?

After: How does it behave after the fixing?

Document Info

Misc

ZRender Changes

Related test cases or examples to use the new APIs

Others

Merging options

Other information

Uh oh!

echarts-bot bot commented Jul 11, 2022

Uh oh!

pissang commented Jul 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Ovilia commented Jul 12, 2022

Uh oh!

100pah Jul 12, 2022 • 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!

Ovilia Jul 13, 2022

Choose a reason for hiding this comment

Uh oh!

100pah Jul 13, 2022 • 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!

Ovilia Jul 14, 2022 • 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!

100pah Jul 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Consider the case below:

Consider leave animation

Uh oh!

100pah Jul 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Consider the case below:

Consider leave animation

Uh oh!

100pah Jul 28, 2022

Choose a reason for hiding this comment

Uh oh!

Ovilia Jul 29, 2022

Choose a reason for hiding this comment

Uh oh!

100pah Jul 29, 2022

Choose a reason for hiding this comment

Uh oh!

100pah Jul 29, 2022

Choose a reason for hiding this comment

Uh oh!

100pah Jul 29, 2022

Choose a reason for hiding this comment

Uh oh!

echarts-bot bot commented Jul 29, 2022

Uh oh!

plainheart Jul 29, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

Ovilia commented Jul 11, 2022 •

edited

Loading

pissang commented Jul 11, 2022 •

edited

Loading

100pah Jul 12, 2022 •

edited

Loading

100pah Jul 13, 2022 •

edited

Loading

Ovilia Jul 14, 2022 •

edited

Loading

100pah Jul 14, 2022 •

edited

Loading

100pah Jul 14, 2022 •

edited

Loading