fix: theme-switch-keep-data. close apache#21200

[mustcanbedo]

Brief Information

This pull request is in the type of:

• bug fixing
• new feature
• others

What does this PR do?

Fixed issues

#21200

Details

Before: What was the problem?

After: How does it behave after the fixing?

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

N.A.

Others

Merging options

• Please squash the commits into a single one when merging.

Other information

[echarts-bot]

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.

⚠️ MISSING DOCUMENT INFO: Please make sure one of the document options are checked in this PR's description. Search "Document Info" in the description of this PR. This should be done either by the author or the reviewers of the PR.

[mustcanbedo]

@Ovilia mustcanbedo@8575bb1#r164131989 Hello, I responded to your last comments, but it seems you didn't see them. I'm replying to you again here.

[Ovilia]

Hi, I believe I've reviewed a similar PR probably from you. Is it deleted?

As I said, it's not correct to make a copy of old options like this. You need to dive into the code to see why option is not kepted after calling setTheme.

[mustcanbedo]

@Ovilia Hello, I have made changes according to your latest suggestions.

[mustcanbedo]

@Ovilia

[mustcanbedo]

@Ovilia Hello, I have revised the version according to your ideas again, please have a look.

[mustcanbedo]

@Ovilia

[mustcanbedo]

@Ovilia Hello, looking forward to your feedback.

[Ovilia]

Sorry for the late reply. @100pah will help review the PR soon.

[100pah]

The format of the theme object is different from the option, and currently all of the theme are applied by Component#mergeDefaultAndTheme, therefore, theme object should not be the input of this._mergeOption.

From my understanding, under the current definition, the implementation of this feature (keep the previous changes when setTheme) is unfeasible.

The current definition is: theme acts as "default values", and option can override properties specified in a theme, but not vice versa.

Suppose we simply merge a new theme to the current model, some bad cases probably arise, for example:

1. apply theme1 by setTheme
2. apply option1 by setOption
3. apply theme2 by setTheme
4. apply option2 by setOption

Some properties specified by option1 may be overridden by theme2, which is unexpected.
Once we introduce that bad cases, they are unable to be fixed and cause the behavior unpredictable and error-prone.

If implement this feature by saving all of the inputs to the API (setOption, dispatchAction, etc.), and restore them when calling setTheme, that might be logically correct, but may significantly degrade performance, and cause memory leak in scenarios that setOption is called repeatedly over time.

I'm not quite sure the real-world scenarios that requires "setTheme while keeping data". Is there any alternative way to achieve that?

[mustcanbedo]

1. Whether the demand you proposed is necessary is actually reasonable; for example, in the case raised by the bug reporter, when he just needed to change the theme, all the data was lost.
2. There are indeed scenario issues with the current modification method.
3. The scenario issues can actually be resolved by modifying the mergeTheme; the changes have now been submitted for another code review.

[mustcanbedo]

@100pah Looking forward to your review again.

[100pah]

1. themeItem isn't supposed to be modified here (calling merge in this way will modify it).
2. The method mergeTheme only handles the top-level non-component properties in option (like color, animation ... ), regardless of any components theme.
3. I'm afraid the priority issue can not be resolved simply by changing the merging order. Many props existing in model.option are default values rather than from user option. And some props in a option do not exits independently; their default values are determined in combination (e.g., box layout props). varying merging order makes the determination inconsistent and hard to understand.

Theoretically "changing theme and keep the currently state" requires a big refactor: introduce a extra underlying model into the model cascade to accommodate theme option, which can be replaced in setTheme. And some approaches of fetching values from model, such as options.xxx or model.get('xxx', true) need to be modified to consider the new added theme model. But I'm not sure whether we need to make that change yet, since it's quite large.

[mustcanbedo]

So it seems that if we need to solve this problem now, there are two options: either do it the way I did in my first fix commit: 8575bb1, that is, keep the configuration first, then apply it after changing the theme, or we would really have to carry out a significant amount of refactoring work. Currently, my proposed solutions are:

1. As you suggested, add a layer of models to handle it (private _themeModel: Model; private _userModel: Model; private _defaultModel: Model;).
2. Reference the flags from the option source, but in terms of memory usage this could potentially cause considerable performance overhead.
   If a short-term fix for this issue is needed, or a refactor for a complete solution, could there be an opportunity to participate together if needed?

[100pah]

Currently, I think the quick fix proposed should not be applied, since it brings more bugs that cannot to be fixed.

So it seems that if we need to solve this problem now

Could you explain the original scenario where you need to use setTheme while keeping data?
I’d like to determine whether this usage is appropriate, and whether a major refactor is worth it.

[mustcanbedo]

The most basic requirement comes from this issue(#21200 )

[100pah]

I'm afraid issue(#21200 ) is not a basic requirement. It does not mention why we need use the API like that in a real-world scenario.

[mustcanbedo]

Upon seeing this topic, my thoughts are about how the system's theme switching could be used in this way. However, it also seems reasonable to expect that after switching the theme, the previous settings should remain consistent.

[mustcanbedo]

@100pah Looking forward to your reply once again.

[100pah]

Before this issue is thoroughly resolved, a workaround can be used:

// --- Bad (May be unexpected) ---
const chart1 = echarts.init(mainDOMElement);
chart1.setOption(option1);
chart1.setOption(option2); // call `setOption` in "merge mode"
chart1.setOption(option3);
chart1.setTheme('dark');
// After calling `setTheme`, the previous options (option1 and option2) are discarded. Only the last option (option3) is retained.

// --- Solution ---
const chart1 = echarts.init(mainDOMElement);
// Make sure every option contains all the information and using `notMerge mode` in `setOption`.
chart1.setOption(option1, {notMerge: true});
chart1.setOption(option2, {notMerge: true});
chart1.setOption(option3, {notMerge: true});
chart1.setTheme('dark');
// The previous options (option1 and option2) are also discarded. But the last option (option3) contains all the information and retained, so the finall effect is correct.