You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@echarts.apache.org by GitBox <gi...@apache.org> on 2022/07/11 09:33:56 UTC
[GitHub] [echarts] Ovilia opened a new pull request, #17349: fix(custom): fix elements may not be removed after updates #17333
Ovilia opened a new pull request, #17349:
URL: https://github.com/apache/echarts/pull/17349
<!-- Please fill in the following information to help us review your PR more efficiently. -->
## Brief Information
This pull request is in the type of:
- [x] bug fixing
- [ ] new feature
- [ ] others
### What does this PR do?
<!-- USE ONE SENTENCE TO DESCRIBE WHAT THIS PR DOES. -->
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?
<!-- DESCRIBE THE BUG OR REQUIREMENT HERE. -->
<!-- ADD SCREENSHOT HERE IF APPLICABLE. -->
There is an extra element after calling `setOption` for the second time.
### After: How does it behave after the fixing?
<!-- THE RESULT AFTER FIXING AND A SIMPLE EXPLANATION ABOUT HOW IT IS FIXED. -->
<!-- ADD SCREENSHOT HERE IF APPLICABLE. -->
No extra element after calling `setOption` for the second time.
## Document Info
One of the following should be checked.
- [x] This PR doesn't relate to document changes
- [ ] The document should be updated later
- [ ] The document changes have been made in apache/echarts-doc#xxx
## 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] 100pah commented on a diff in pull request #17349: fix(custom): fix elements may not be removed after updates #17333
Posted by GitBox <gi...@apache.org>.
100pah commented on code in PR #17349:
URL: https://github.com/apache/echarts/pull/17349#discussion_r931912719
##########
src/chart/custom/CustomView.ts:
##########
@@ -1316,24 +1322,52 @@ function mergeChildren(
// might be better performance.
let index = 0;
for (; index < newLen; index++) {
- newChildren[index] && doCreateOrUpdateEl(
- api,
- el.childAt(index),
- dataIndex,
- newChildren[index] as CustomElementOption,
- seriesModel,
- el
- );
+ const newChild = newChildren[index];
+ const oldChild = el.childAt(index);
+ if (newChild) {
+ if (newChild.ignore == null) {
Review Comment:
Should be:
```ts
if (newChild.ignore) {
newChild.ignore = false;
}
```
or
```ts
newChild.ignore = null; // or false
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] 100pah commented on a diff in pull request #17349: fix(custom): fix elements may not be removed after updates #17333
Posted by GitBox <gi...@apache.org>.
100pah commented on code in PR #17349:
URL: https://github.com/apache/echarts/pull/17349#discussion_r920130459
##########
src/chart/custom/CustomView.ts:
##########
@@ -1316,24 +1322,43 @@ function mergeChildren(
// might be better performance.
let index = 0;
for (; index < newLen; index++) {
- newChildren[index] && doCreateOrUpdateEl(
- api,
- el.childAt(index),
- dataIndex,
- newChildren[index] as CustomElementOption,
- seriesModel,
- el
- );
+ const newChild = newChildren[index];
+ const oldChild = el.childAt(index);
+ if (newChild) {
+ doCreateOrUpdateEl(
+ api,
+ oldChild,
+ dataIndex,
+ newChild as CustomElementOption,
+ seriesModel,
+ el
+ );
+ }
+ else {
+ removeChildFromGroup(el, oldChild, seriesModel);
Review 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)
+ 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?)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] 100pah commented on a diff in pull request #17349: fix(custom): fix elements may not be removed after updates #17333
Posted by GitBox <gi...@apache.org>.
100pah commented on code in PR #17349:
URL: https://github.com/apache/echarts/pull/17349#discussion_r920130459
##########
src/chart/custom/CustomView.ts:
##########
@@ -1316,24 +1322,43 @@ function mergeChildren(
// might be better performance.
let index = 0;
for (; index < newLen; index++) {
- newChildren[index] && doCreateOrUpdateEl(
- api,
- el.childAt(index),
- dataIndex,
- newChildren[index] as CustomElementOption,
- seriesModel,
- el
- );
+ const newChild = newChildren[index];
+ const oldChild = el.childAt(index);
+ if (newChild) {
+ doCreateOrUpdateEl(
+ api,
+ oldChild,
+ dataIndex,
+ newChild as CustomElementOption,
+ seriesModel,
+ el
+ );
+ }
+ else {
+ removeChildFromGroup(el, oldChild, seriesModel);
Review Comment:
I think at least this behavior is not good (by the current modification): 🤔
existing children: `[elA, elB, elC]`
new children: `[elM, null, elN]`
if transition animation enabled:
result: [merge(elA, elM), elC, elN]
if transition animation disabled:
result: [merge(elA, elM), merge(elA, elM)]
Both of them seems not expectable and intuitive for users.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] 100pah commented on a diff in pull request #17349: fix(custom): fix elements may not be removed after updates #17333
Posted by GitBox <gi...@apache.org>.
100pah commented on code in PR #17349:
URL: https://github.com/apache/echarts/pull/17349#discussion_r921362146
##########
src/chart/custom/CustomView.ts:
##########
@@ -1316,24 +1322,43 @@ function mergeChildren(
// might be better performance.
let index = 0;
for (; index < newLen; index++) {
- newChildren[index] && doCreateOrUpdateEl(
- api,
- el.childAt(index),
- dataIndex,
- newChildren[index] as CustomElementOption,
- seriesModel,
- el
- );
+ const newChild = newChildren[index];
+ const oldChild = el.childAt(index);
+ if (newChild) {
+ doCreateOrUpdateEl(
+ api,
+ oldChild,
+ dataIndex,
+ newChild as CustomElementOption,
+ seriesModel,
+ el
+ );
+ }
+ else {
+ removeChildFromGroup(el, oldChild, seriesModel);
Review Comment:
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 elements should not be changed after this deletion.
That is, is we delete a child, that place may be kept "no element" or "an placeholder element that can not displayed"
##########
src/chart/custom/CustomView.ts:
##########
@@ -1316,24 +1322,43 @@ function mergeChildren(
// might be better performance.
let index = 0;
for (; index < newLen; index++) {
- newChildren[index] && doCreateOrUpdateEl(
- api,
- el.childAt(index),
- dataIndex,
- newChildren[index] as CustomElementOption,
- seriesModel,
- el
- );
+ const newChild = newChildren[index];
+ const oldChild = el.childAt(index);
+ if (newChild) {
+ doCreateOrUpdateEl(
+ api,
+ oldChild,
+ dataIndex,
+ newChild as CustomElementOption,
+ seriesModel,
+ el
+ );
+ }
+ else {
+ removeChildFromGroup(el, oldChild, seriesModel);
Review Comment:
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 may be kept "no element" or "an placeholder element that can not displayed"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] 100pah commented on a diff in pull request #17349: fix(custom): fix elements may not be removed after updates #17333
Posted by GitBox <gi...@apache.org>.
100pah commented on code in PR #17349:
URL: https://github.com/apache/echarts/pull/17349#discussion_r920130459
##########
src/chart/custom/CustomView.ts:
##########
@@ -1316,24 +1322,43 @@ function mergeChildren(
// might be better performance.
let index = 0;
for (; index < newLen; index++) {
- newChildren[index] && doCreateOrUpdateEl(
- api,
- el.childAt(index),
- dataIndex,
- newChildren[index] as CustomElementOption,
- seriesModel,
- el
- );
+ const newChild = newChildren[index];
+ const oldChild = el.childAt(index);
+ if (newChild) {
+ doCreateOrUpdateEl(
+ api,
+ oldChild,
+ dataIndex,
+ newChild as CustomElementOption,
+ seriesModel,
+ el
+ );
+ }
+ else {
+ removeChildFromGroup(el, oldChild, seriesModel);
Review 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 both of the input old and new children)
+ if transition animation disabled:
+ result: `[merge(elA, elM), elC, elN]` (length: 3, but the index of `elC` change from `2` to `1` )
Both of them seems not expectable and intuitive for users.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] 100pah commented on a diff in pull request #17349: fix(custom): fix elements may not be removed after updates #17333
Posted by GitBox <gi...@apache.org>.
100pah commented on code in PR #17349:
URL: https://github.com/apache/echarts/pull/17349#discussion_r920130459
##########
src/chart/custom/CustomView.ts:
##########
@@ -1316,24 +1322,43 @@ function mergeChildren(
// might be better performance.
let index = 0;
for (; index < newLen; index++) {
- newChildren[index] && doCreateOrUpdateEl(
- api,
- el.childAt(index),
- dataIndex,
- newChildren[index] as CustomElementOption,
- seriesModel,
- el
- );
+ const newChild = newChildren[index];
+ const oldChild = el.childAt(index);
+ if (newChild) {
+ doCreateOrUpdateEl(
+ api,
+ oldChild,
+ dataIndex,
+ newChild as CustomElementOption,
+ seriesModel,
+ el
+ );
+ }
+ else {
+ removeChildFromGroup(el, oldChild, seriesModel);
Review 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 both of the old children and 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?)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] Ovilia commented on a diff in pull request #17349: fix(custom): fix elements may not be removed after updates #17333
Posted by GitBox <gi...@apache.org>.
Ovilia commented on code in PR #17349:
URL: https://github.com/apache/echarts/pull/17349#discussion_r920833222
##########
src/chart/custom/CustomView.ts:
##########
@@ -1316,24 +1322,43 @@ function mergeChildren(
// might be better performance.
let index = 0;
for (; index < newLen; index++) {
- newChildren[index] && doCreateOrUpdateEl(
- api,
- el.childAt(index),
- dataIndex,
- newChildren[index] as CustomElementOption,
- seriesModel,
- el
- );
+ const newChild = newChildren[index];
+ const oldChild = el.childAt(index);
+ if (newChild) {
+ doCreateOrUpdateEl(
+ api,
+ oldChild,
+ dataIndex,
+ newChild as CustomElementOption,
+ seriesModel,
+ el
+ );
+ }
+ else {
+ removeChildFromGroup(el, oldChild, seriesModel);
Review Comment:
You are quite right! I didn't think of the situation. I fixed this and also update the test case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] 100pah commented on a diff in pull request #17349: fix(custom): fix elements may not be removed after updates #17333
Posted by GitBox <gi...@apache.org>.
100pah commented on code in PR #17349:
URL: https://github.com/apache/echarts/pull/17349#discussion_r932839998
##########
src/chart/custom/CustomView.ts:
##########
@@ -1316,24 +1322,52 @@ function mergeChildren(
// might be better performance.
let index = 0;
for (; index < newLen; index++) {
- newChildren[index] && doCreateOrUpdateEl(
- api,
- el.childAt(index),
- dataIndex,
- newChildren[index] as CustomElementOption,
- seriesModel,
- el
- );
+ const newChild = newChildren[index];
+ const oldChild = el.childAt(index);
+ if (newChild) {
+ if (newChild.ignore == null) {
Review Comment:
I got it wrong
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] 100pah commented on a diff in pull request #17349: fix(custom): fix elements may not be removed after updates #17333
Posted by GitBox <gi...@apache.org>.
100pah commented on code in PR #17349:
URL: https://github.com/apache/echarts/pull/17349#discussion_r918602222
##########
src/chart/custom/CustomView.ts:
##########
@@ -1316,14 +1316,21 @@ function mergeChildren(
// might be better performance.
let index = 0;
for (; index < newLen; index++) {
- newChildren[index] && doCreateOrUpdateEl(
- api,
- el.childAt(index),
- dataIndex,
- newChildren[index] as CustomElementOption,
- seriesModel,
- el
- );
+ const newChild = newChildren[index];
+ if (newChild) {
+ doCreateOrUpdateEl(
+ api,
+ el.childAt(index),
+ dataIndex,
+ newChild as CustomElementOption,
+ seriesModel,
+ el
+ );
+ }
+ else {
+ // The element is being null after updating, remove the old element
+ el.remove(el.childAt(index));
Review Comment:
```js
el.remove(el.childAt(index));
```
This is not correct. `el.remove` will change the entire indices of the subsequent elements
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] 100pah commented on a diff in pull request #17349: fix(custom): fix elements may not be removed after updates #17333
Posted by GitBox <gi...@apache.org>.
100pah commented on code in PR #17349:
URL: https://github.com/apache/echarts/pull/17349#discussion_r920130459
##########
src/chart/custom/CustomView.ts:
##########
@@ -1316,24 +1322,43 @@ function mergeChildren(
// might be better performance.
let index = 0;
for (; index < newLen; index++) {
- newChildren[index] && doCreateOrUpdateEl(
- api,
- el.childAt(index),
- dataIndex,
- newChildren[index] as CustomElementOption,
- seriesModel,
- el
- );
+ const newChild = newChildren[index];
+ const oldChild = el.childAt(index);
+ if (newChild) {
+ doCreateOrUpdateEl(
+ api,
+ oldChild,
+ dataIndex,
+ newChild as CustomElementOption,
+ seriesModel,
+ el
+ );
+ }
+ else {
+ removeChildFromGroup(el, oldChild, seriesModel);
Review 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)
+ if transition animation disabled:
+ result: `[merge(elA, elM), elC, elN]` (length: 3, but the index of `elC` change from `2` to `1` )
Both of them seems not expectable and intuitive for users.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] 100pah commented on a diff in pull request #17349: fix(custom): fix elements may not be removed after updates #17333
Posted by GitBox <gi...@apache.org>.
100pah commented on code in PR #17349:
URL: https://github.com/apache/echarts/pull/17349#discussion_r921362146
##########
src/chart/custom/CustomView.ts:
##########
@@ -1316,24 +1322,43 @@ function mergeChildren(
// might be better performance.
let index = 0;
for (; index < newLen; index++) {
- newChildren[index] && doCreateOrUpdateEl(
- api,
- el.childAt(index),
- dataIndex,
- newChildren[index] as CustomElementOption,
- seriesModel,
- el
- );
+ const newChild = newChildren[index];
+ const oldChild = el.childAt(index);
+ if (newChild) {
+ doCreateOrUpdateEl(
+ api,
+ oldChild,
+ dataIndex,
+ newChild as CustomElementOption,
+ seriesModel,
+ el
+ );
+ }
+ else {
+ removeChildFromGroup(el, oldChild, seriesModel);
Review Comment:
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"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] 100pah commented on a diff in pull request #17349: fix(custom): fix elements may not be removed after updates #17333
Posted by GitBox <gi...@apache.org>.
100pah commented on code in PR #17349:
URL: https://github.com/apache/echarts/pull/17349#discussion_r921362146
##########
src/chart/custom/CustomView.ts:
##########
@@ -1316,24 +1322,43 @@ function mergeChildren(
// might be better performance.
let index = 0;
for (; index < newLen; index++) {
- newChildren[index] && doCreateOrUpdateEl(
- api,
- el.childAt(index),
- dataIndex,
- newChildren[index] as CustomElementOption,
- seriesModel,
- el
- );
+ const newChild = newChildren[index];
+ const oldChild = el.childAt(index);
+ if (newChild) {
+ doCreateOrUpdateEl(
+ api,
+ oldChild,
+ dataIndex,
+ newChild as CustomElementOption,
+ seriesModel,
+ el
+ );
+ }
+ else {
+ removeChildFromGroup(el, oldChild, seriesModel);
Review Comment:
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 become "no element" or "an placeholder element that can not displayed"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] 100pah commented on a diff in pull request #17349: fix(custom): fix elements may not be removed after updates #17333
Posted by GitBox <gi...@apache.org>.
100pah commented on code in PR #17349:
URL: https://github.com/apache/echarts/pull/17349#discussion_r921362146
##########
src/chart/custom/CustomView.ts:
##########
@@ -1316,24 +1322,43 @@ function mergeChildren(
// might be better performance.
let index = 0;
for (; index < newLen; index++) {
- newChildren[index] && doCreateOrUpdateEl(
- api,
- el.childAt(index),
- dataIndex,
- newChildren[index] as CustomElementOption,
- seriesModel,
- el
- );
+ const newChild = newChildren[index];
+ const oldChild = el.childAt(index);
+ if (newChild) {
+ doCreateOrUpdateEl(
+ api,
+ oldChild,
+ dataIndex,
+ newChild as CustomElementOption,
+ seriesModel,
+ el
+ );
+ }
+ else {
+ removeChildFromGroup(el, oldChild, seriesModel);
Review Comment:
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 apply:
The index of any other elements should not be changed after this deletion.
That is, is we delete a child, that place may be kept "no element" or "an placeholder element that can not displayed"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] 100pah commented on a diff in pull request #17349: fix(custom): fix elements may not be removed after updates #17333
Posted by GitBox <gi...@apache.org>.
100pah commented on code in PR #17349:
URL: https://github.com/apache/echarts/pull/17349#discussion_r920130459
##########
src/chart/custom/CustomView.ts:
##########
@@ -1316,24 +1322,43 @@ function mergeChildren(
// might be better performance.
let index = 0;
for (; index < newLen; index++) {
- newChildren[index] && doCreateOrUpdateEl(
- api,
- el.childAt(index),
- dataIndex,
- newChildren[index] as CustomElementOption,
- seriesModel,
- el
- );
+ const newChild = newChildren[index];
+ const oldChild = el.childAt(index);
+ if (newChild) {
+ doCreateOrUpdateEl(
+ api,
+ oldChild,
+ dataIndex,
+ newChild as CustomElementOption,
+ seriesModel,
+ el
+ );
+ }
+ else {
+ removeChildFromGroup(el, oldChild, seriesModel);
Review Comment:
I think at least this behavior is not good (by the current modification): 🤔
old children: `[elA, elB, elC]`
new children: `[elM, null, elN]`
+ if transition animation enabled:
+ result: `[merge(elA, elM), merge(elC, elN)]`
+ if transition animation disabled:
+ result: `[merge(elA, elM), elC, elN]`
Both of them seems not expectable and intuitive for users.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] Ovilia commented on a diff in pull request #17349: fix(custom): fix elements may not be removed after updates #17333
Posted by GitBox <gi...@apache.org>.
Ovilia commented on code in PR #17349:
URL: https://github.com/apache/echarts/pull/17349#discussion_r919631231
##########
src/chart/custom/CustomView.ts:
##########
@@ -1316,24 +1322,43 @@ function mergeChildren(
// might be better performance.
let index = 0;
for (; index < newLen; index++) {
- newChildren[index] && doCreateOrUpdateEl(
- api,
- el.childAt(index),
- dataIndex,
- newChildren[index] as CustomElementOption,
- seriesModel,
- el
- );
+ const newChild = newChildren[index];
+ const oldChild = el.childAt(index);
+ if (newChild) {
+ doCreateOrUpdateEl(
+ api,
+ oldChild,
+ dataIndex,
+ newChild as CustomElementOption,
+ seriesModel,
+ el
+ );
+ }
+ else {
+ removeChildFromGroup(el, oldChild, seriesModel);
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] 100pah commented on a diff in pull request #17349: fix(custom): fix elements may not be removed after updates #17333
Posted by GitBox <gi...@apache.org>.
100pah commented on code in PR #17349:
URL: https://github.com/apache/echarts/pull/17349#discussion_r918842734
##########
src/chart/custom/CustomView.ts:
##########
@@ -1316,24 +1322,43 @@ function mergeChildren(
// might be better performance.
let index = 0;
for (; index < newLen; index++) {
- newChildren[index] && doCreateOrUpdateEl(
- api,
- el.childAt(index),
- dataIndex,
- newChildren[index] as CustomElementOption,
- seriesModel,
- el
- );
+ const newChild = newChildren[index];
+ const oldChild = el.childAt(index);
+ if (newChild) {
+ doCreateOrUpdateEl(
+ api,
+ oldChild,
+ dataIndex,
+ newChild as CustomElementOption,
+ seriesModel,
+ el
+ );
+ }
+ else {
+ removeChildFromGroup(el, oldChild, seriesModel);
Review Comment:
If leave 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.
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
```js
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] 100pah commented on a diff in pull request #17349: fix(custom): fix elements may not be removed after updates #17333
Posted by GitBox <gi...@apache.org>.
100pah commented on code in PR #17349:
URL: https://github.com/apache/echarts/pull/17349#discussion_r918842734
##########
src/chart/custom/CustomView.ts:
##########
@@ -1316,24 +1322,43 @@ function mergeChildren(
// might be better performance.
let index = 0;
for (; index < newLen; index++) {
- newChildren[index] && doCreateOrUpdateEl(
- api,
- el.childAt(index),
- dataIndex,
- newChildren[index] as CustomElementOption,
- seriesModel,
- el
- );
+ const newChild = newChildren[index];
+ const oldChild = el.childAt(index);
+ if (newChild) {
+ doCreateOrUpdateEl(
+ api,
+ oldChild,
+ dataIndex,
+ newChild as CustomElementOption,
+ seriesModel,
+ el
+ );
+ }
+ else {
+ removeChildFromGroup(el, oldChild, seriesModel);
Review 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.
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
```js
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] 100pah commented on a diff in pull request #17349: fix(custom): fix elements may not be removed after updates #17333
Posted by GitBox <gi...@apache.org>.
100pah commented on code in PR #17349:
URL: https://github.com/apache/echarts/pull/17349#discussion_r918842734
##########
src/chart/custom/CustomView.ts:
##########
@@ -1316,24 +1322,43 @@ function mergeChildren(
// might be better performance.
let index = 0;
for (; index < newLen; index++) {
- newChildren[index] && doCreateOrUpdateEl(
- api,
- el.childAt(index),
- dataIndex,
- newChildren[index] as CustomElementOption,
- seriesModel,
- el
- );
+ const newChild = newChildren[index];
+ const oldChild = el.childAt(index);
+ if (newChild) {
+ doCreateOrUpdateEl(
+ api,
+ oldChild,
+ dataIndex,
+ newChild as CustomElementOption,
+ seriesModel,
+ el
+ );
+ }
+ else {
+ removeChildFromGroup(el, oldChild, seriesModel);
Review 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
```js
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] 100pah commented on a diff in pull request #17349: fix(custom): fix elements may not be removed after updates #17333
Posted by GitBox <gi...@apache.org>.
100pah commented on code in PR #17349:
URL: https://github.com/apache/echarts/pull/17349#discussion_r920130459
##########
src/chart/custom/CustomView.ts:
##########
@@ -1316,24 +1322,43 @@ function mergeChildren(
// might be better performance.
let index = 0;
for (; index < newLen; index++) {
- newChildren[index] && doCreateOrUpdateEl(
- api,
- el.childAt(index),
- dataIndex,
- newChildren[index] as CustomElementOption,
- seriesModel,
- el
- );
+ const newChild = newChildren[index];
+ const oldChild = el.childAt(index);
+ if (newChild) {
+ doCreateOrUpdateEl(
+ api,
+ oldChild,
+ dataIndex,
+ newChild as CustomElementOption,
+ seriesModel,
+ el
+ );
+ }
+ else {
+ removeChildFromGroup(el, oldChild, seriesModel);
Review 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 both of the old children and 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] 100pah commented on a diff in pull request #17349: fix(custom): fix elements may not be removed after updates #17333
Posted by GitBox <gi...@apache.org>.
100pah commented on code in PR #17349:
URL: https://github.com/apache/echarts/pull/17349#discussion_r921362146
##########
src/chart/custom/CustomView.ts:
##########
@@ -1316,24 +1322,43 @@ function mergeChildren(
// might be better performance.
let index = 0;
for (; index < newLen; index++) {
- newChildren[index] && doCreateOrUpdateEl(
- api,
- el.childAt(index),
- dataIndex,
- newChildren[index] as CustomElementOption,
- seriesModel,
- el
- );
+ const newChild = newChildren[index];
+ const oldChild = el.childAt(index);
+ if (newChild) {
+ doCreateOrUpdateEl(
+ api,
+ oldChild,
+ dataIndex,
+ newChild as CustomElementOption,
+ seriesModel,
+ el
+ );
+ }
+ else {
+ removeChildFromGroup(el, oldChild, seriesModel);
Review Comment:
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 may be kept "no element" or "an placeholder element that can not displayed"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] Ovilia merged pull request #17349: fix(custom): fix elements may not be removed after updates #17333
Posted by GitBox <gi...@apache.org>.
Ovilia merged PR #17349:
URL: https://github.com/apache/echarts/pull/17349
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] 100pah commented on a diff in pull request #17349: fix(custom): fix elements may not be removed after updates #17333
Posted by GitBox <gi...@apache.org>.
100pah commented on code in PR #17349:
URL: https://github.com/apache/echarts/pull/17349#discussion_r920130459
##########
src/chart/custom/CustomView.ts:
##########
@@ -1316,24 +1322,43 @@ function mergeChildren(
// might be better performance.
let index = 0;
for (; index < newLen; index++) {
- newChildren[index] && doCreateOrUpdateEl(
- api,
- el.childAt(index),
- dataIndex,
- newChildren[index] as CustomElementOption,
- seriesModel,
- el
- );
+ const newChild = newChildren[index];
+ const oldChild = el.childAt(index);
+ if (newChild) {
+ doCreateOrUpdateEl(
+ api,
+ oldChild,
+ dataIndex,
+ newChild as CustomElementOption,
+ seriesModel,
+ el
+ );
+ }
+ else {
+ removeChildFromGroup(el, oldChild, seriesModel);
Review 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 both of the old children and the input new children)
+ if transition animation disabled:
+ result: `[merge(elA, elM), elC, elN]` (length: 3, but the index of `elC` change from `2` to `1` )
Both of them seems not expectable and intuitive for users.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] 100pah commented on a diff in pull request #17349: fix(custom): fix elements may not be removed after updates #17333
Posted by GitBox <gi...@apache.org>.
100pah commented on code in PR #17349:
URL: https://github.com/apache/echarts/pull/17349#discussion_r920130459
##########
src/chart/custom/CustomView.ts:
##########
@@ -1316,24 +1322,43 @@ function mergeChildren(
// might be better performance.
let index = 0;
for (; index < newLen; index++) {
- newChildren[index] && doCreateOrUpdateEl(
- api,
- el.childAt(index),
- dataIndex,
- newChildren[index] as CustomElementOption,
- seriesModel,
- el
- );
+ const newChild = newChildren[index];
+ const oldChild = el.childAt(index);
+ if (newChild) {
+ doCreateOrUpdateEl(
+ api,
+ oldChild,
+ dataIndex,
+ newChild as CustomElementOption,
+ seriesModel,
+ el
+ );
+ }
+ else {
+ removeChildFromGroup(el, oldChild, seriesModel);
Review Comment:
I think at least this behavior is not good (by the current modification): 🤔
old children: `[elA, elB, elC]`
new children: `[elM, null, elN]`
+ if transition animation enabled:
+ result: [merge(elA, elM), merge(elC, elN)]
+ if transition animation disabled:
+ result: [merge(elA, elM), elC, elN]
Both of them seems not expectable and intuitive for users.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] 100pah commented on a diff in pull request #17349: fix(custom): fix elements may not be removed after updates #17333
Posted by GitBox <gi...@apache.org>.
100pah commented on code in PR #17349:
URL: https://github.com/apache/echarts/pull/17349#discussion_r920130459
##########
src/chart/custom/CustomView.ts:
##########
@@ -1316,24 +1322,43 @@ function mergeChildren(
// might be better performance.
let index = 0;
for (; index < newLen; index++) {
- newChildren[index] && doCreateOrUpdateEl(
- api,
- el.childAt(index),
- dataIndex,
- newChildren[index] as CustomElementOption,
- seriesModel,
- el
- );
+ const newChild = newChildren[index];
+ const oldChild = el.childAt(index);
+ if (newChild) {
+ doCreateOrUpdateEl(
+ api,
+ oldChild,
+ dataIndex,
+ newChild as CustomElementOption,
+ seriesModel,
+ el
+ );
+ }
+ else {
+ removeChildFromGroup(el, oldChild, seriesModel);
Review Comment:
At least this behavior is not good (by the current modification):
existing children: `[elA, elB, elC]`
new children: `[elM, null, elN]`
if transition animation enabled:
result: [merge(elA, elM), elC, elN]
if transition animation disabled:
result: [merge(elA, elM), merge(elA, elM)]
Both of them seems not expectable and intuitive for users.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] 100pah commented on a diff in pull request #17349: fix(custom): fix elements may not be removed after updates #17333
Posted by GitBox <gi...@apache.org>.
100pah commented on code in PR #17349:
URL: https://github.com/apache/echarts/pull/17349#discussion_r932843672
##########
src/chart/custom/CustomView.ts:
##########
@@ -1316,24 +1322,52 @@ function mergeChildren(
// might be better performance.
let index = 0;
for (; index < newLen; index++) {
- newChildren[index] && doCreateOrUpdateEl(
- api,
- el.childAt(index),
- dataIndex,
- newChildren[index] as CustomElementOption,
- seriesModel,
- el
- );
+ const newChild = newChildren[index];
+ const oldChild = el.childAt(index);
+ if (newChild) {
+ if (newChild.ignore == null) {
Review Comment:
What if users set a child as `null` / `undefined` at the first time ? (rather than the second time)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] 100pah commented on a diff in pull request #17349: fix(custom): fix elements may not be removed after updates #17333
Posted by GitBox <gi...@apache.org>.
100pah commented on code in PR #17349:
URL: https://github.com/apache/echarts/pull/17349#discussion_r918582647
##########
src/chart/custom/CustomView.ts:
##########
@@ -1316,14 +1316,21 @@ function mergeChildren(
// might be better performance.
let index = 0;
for (; index < newLen; index++) {
- newChildren[index] && doCreateOrUpdateEl(
- api,
- el.childAt(index),
- dataIndex,
- newChildren[index] as CustomElementOption,
- seriesModel,
- el
- );
+ const newChild = newChildren[index];
+ if (newChild) {
+ doCreateOrUpdateEl(
+ api,
+ el.childAt(index),
+ dataIndex,
+ newChild as CustomElementOption,
+ seriesModel,
+ el
+ );
+ }
+ else {
+ // The element is being null after updating, remove the old element
+ el.remove(el.childAt(index));
Review Comment:
Basically I agree this change, which might make it more intuitive. BTW, using "merge" by default rather than "replace" because the performance seams to be better.
But this is a breaking change, we need to note it as "breaking change" in release note.
And if modifying this behavior, the comment of this function are also needed to be updated.
> // (1) By default, `elOption.$mergeChildren` is `'byIndex'`, which indicates that
// the existing children will not be removed, and enables the feature that
// update some of the props of some of the children simply by construct
// the returned children of `renderItem` like:
// `var children = group.children = []; children[3] = {opacity: 0.5};`
Probably need to explain the usage like: `null` means delete and empty object `{}` means keep nothing changed.
Do we need to add some doc to explain that in <https://echarts.apache.org/en/option.html#series-custom.renderItem.return_group>?
This doc of `$mergeChildren` is missing because I'm not sure whether to provide this option and whether to name it as `$mergeChildren` yet.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] Ovilia commented on pull request #17349: fix(custom): fix elements may not be removed after updates #17333
Posted by GitBox <gi...@apache.org>.
Ovilia commented on PR #17349:
URL: https://github.com/apache/echarts/pull/17349#issuecomment-1181493906
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] 100pah commented on a diff in pull request #17349: fix(custom): fix elements may not be removed after updates #17333
Posted by GitBox <gi...@apache.org>.
100pah commented on code in PR #17349:
URL: https://github.com/apache/echarts/pull/17349#discussion_r918842734
##########
src/chart/custom/CustomView.ts:
##########
@@ -1316,24 +1322,43 @@ function mergeChildren(
// might be better performance.
let index = 0;
for (; index < newLen; index++) {
- newChildren[index] && doCreateOrUpdateEl(
- api,
- el.childAt(index),
- dataIndex,
- newChildren[index] as CustomElementOption,
- seriesModel,
- el
- );
+ const newChild = newChildren[index];
+ const oldChild = el.childAt(index);
+ if (newChild) {
+ doCreateOrUpdateEl(
+ api,
+ oldChild,
+ dataIndex,
+ newChild as CustomElementOption,
+ seriesModel,
+ el
+ );
+ }
+ else {
+ removeChildFromGroup(el, oldChild, seriesModel);
Review 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, the animation occur 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
```js
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] echarts-bot[bot] commented on pull request #17349: fix(custom): fix elements may not be removed after updates #17333
Posted by GitBox <gi...@apache.org>.
echarts-bot[bot] commented on PR #17349:
URL: https://github.com/apache/echarts/pull/17349#issuecomment-1180175631
Thanks for your contribution!
The community will review it ASAP. In the meanwhile, please checkout [the coding standard](https://echarts.apache.org/en/coding-standard.html) and Wiki about [How to make a pull request](https://github.com/apache/echarts/wiki/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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on pull request #17349: fix(custom): fix elements may not be removed after updates #17333
Posted by GitBox <gi...@apache.org>.
pissang commented on PR #17349:
URL: https://github.com/apache/echarts/pull/17349#issuecomment-1180598511
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 let code much more intuitionistic. 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] echarts-bot[bot] commented on pull request #17349: fix(custom): fix elements may not be removed after updates #17333
Posted by GitBox <gi...@apache.org>.
echarts-bot[bot] commented on PR #17349:
URL: https://github.com/apache/echarts/pull/17349#issuecomment-1198847191
Congratulations! Your PR has been merged. Thanks for your contribution! 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] Ovilia commented on a diff in pull request #17349: fix(custom): fix elements may not be removed after updates #17333
Posted by GitBox <gi...@apache.org>.
Ovilia commented on code in PR #17349:
URL: https://github.com/apache/echarts/pull/17349#discussion_r932828514
##########
src/chart/custom/CustomView.ts:
##########
@@ -1316,24 +1322,52 @@ function mergeChildren(
// might be better performance.
let index = 0;
for (; index < newLen; index++) {
- newChildren[index] && doCreateOrUpdateEl(
- api,
- el.childAt(index),
- dataIndex,
- newChildren[index] as CustomElementOption,
- seriesModel,
- el
- );
+ const newChild = newChildren[index];
+ const oldChild = el.childAt(index);
+ if (newChild) {
+ if (newChild.ignore == null) {
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] plainheart commented on a diff in pull request #17349: fix(custom): fix elements may not be removed after updates #17333
Posted by GitBox <gi...@apache.org>.
plainheart commented on code in PR #17349:
URL: https://github.com/apache/echarts/pull/17349#discussion_r932843099
##########
src/chart/custom/CustomView.ts:
##########
@@ -99,6 +99,7 @@ import {
applyKeyframeAnimation,
stopPreviousKeyframeAnimationAndRestore
} from '../../animation/customGraphicKeyframeAnimation';
+import { SeriesModel } from '../../echarts.all';
Review Comment:
The import source is incorrect.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] Ovilia commented on a diff in pull request #17349: fix(custom): fix elements may not be removed after updates #17333
Posted by GitBox <gi...@apache.org>.
Ovilia commented on code in PR #17349:
URL: https://github.com/apache/echarts/pull/17349#discussion_r920833222
##########
src/chart/custom/CustomView.ts:
##########
@@ -1316,24 +1322,43 @@ function mergeChildren(
// might be better performance.
let index = 0;
for (; index < newLen; index++) {
- newChildren[index] && doCreateOrUpdateEl(
- api,
- el.childAt(index),
- dataIndex,
- newChildren[index] as CustomElementOption,
- seriesModel,
- el
- );
+ const newChild = newChildren[index];
+ const oldChild = el.childAt(index);
+ if (newChild) {
+ doCreateOrUpdateEl(
+ api,
+ oldChild,
+ dataIndex,
+ newChild as CustomElementOption,
+ seriesModel,
+ el
+ );
+ }
+ else {
+ removeChildFromGroup(el, oldChild, seriesModel);
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] 100pah commented on a diff in pull request #17349: fix(custom): fix elements may not be removed after updates #17333
Posted by GitBox <gi...@apache.org>.
100pah commented on code in PR #17349:
URL: https://github.com/apache/echarts/pull/17349#discussion_r918582647
##########
src/chart/custom/CustomView.ts:
##########
@@ -1316,14 +1316,21 @@ function mergeChildren(
// might be better performance.
let index = 0;
for (; index < newLen; index++) {
- newChildren[index] && doCreateOrUpdateEl(
- api,
- el.childAt(index),
- dataIndex,
- newChildren[index] as CustomElementOption,
- seriesModel,
- el
- );
+ const newChild = newChildren[index];
+ if (newChild) {
+ doCreateOrUpdateEl(
+ api,
+ el.childAt(index),
+ dataIndex,
+ newChild as CustomElementOption,
+ seriesModel,
+ el
+ );
+ }
+ else {
+ // The element is being null after updating, remove the old element
+ el.remove(el.childAt(index));
Review Comment:
Basically I agree this change, which might make it more intuitive. BTW, using "merge" by default rather than replace because the performance seams to be better.
But this is a breaking change, we need to note it as "breaking change" in release note.
And if modifying this behavior, the comment of this function are also needed to be updated.
> // (1) By default, `elOption.$mergeChildren` is `'byIndex'`, which indicates that
// the existing children will not be removed, and enables the feature that
// update some of the props of some of the children simply by construct
// the returned children of `renderItem` like:
// `var children = group.children = []; children[3] = {opacity: 0.5};`
Probably need to explain the usage like: `null` means delete and empty object `{}` means keep nothing change.
Do we need to add some doc to explain that in <https://echarts.apache.org/en/option.html#series-custom.renderItem.return_group>?
This doc of `$mergeChildren` is missing because I'm not sure whether to provide this option and whether to name it as `$mergeChildren` yet.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] 100pah commented on a diff in pull request #17349: fix(custom): fix elements may not be removed after updates #17333
Posted by GitBox <gi...@apache.org>.
100pah commented on code in PR #17349:
URL: https://github.com/apache/echarts/pull/17349#discussion_r920130459
##########
src/chart/custom/CustomView.ts:
##########
@@ -1316,24 +1322,43 @@ function mergeChildren(
// might be better performance.
let index = 0;
for (; index < newLen; index++) {
- newChildren[index] && doCreateOrUpdateEl(
- api,
- el.childAt(index),
- dataIndex,
- newChildren[index] as CustomElementOption,
- seriesModel,
- el
- );
+ const newChild = newChildren[index];
+ const oldChild = el.childAt(index);
+ if (newChild) {
+ doCreateOrUpdateEl(
+ api,
+ oldChild,
+ dataIndex,
+ newChild as CustomElementOption,
+ seriesModel,
+ el
+ );
+ }
+ else {
+ removeChildFromGroup(el, oldChild, seriesModel);
Review Comment:
I think at least this behavior is not good (by the current modification): 🤔
old children: `[elA, elB, elC]`
new children: `[elM, null, elN]`
if transition animation enabled:
result: [merge(elA, elM), elC, elN]
if transition animation disabled:
result: [merge(elA, elM), merge(elA, elM)]
Both of them seems not expectable and intuitive for users.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org