Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
+105 −5
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8575bb1
fix: theme-switch-keep-data. close apache##21200
mustcanbedo Aug 15, 2025
6c4ba79
fix: getOption does not exist
mustcanbedo Aug 18, 2025
2565079
fix: Change the thought process
mustcanbedo Aug 27, 2025
ebd26ff
Merge branch 'fix-theme-switch-keep-data' of https://github.com/mustc…
mustcanbedo Aug 27, 2025
f72bb3f
fix: Restore the original code
mustcanbedo Aug 27, 2025
03e13c2
fix: Scene issue
mustcanbedo Sep 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions src/model/Global.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ class GlobalModel extends Model<ECUnitOption> {
}

private _resetOption(
type: 'recreate' | 'timeline' | 'media',
type: 'recreate' | 'timeline' | 'media' | 'theme',
opt: InnerSetOptionOpts
): boolean {
let optionChanged = false;
Expand Down Expand Up @@ -283,6 +283,13 @@ class GlobalModel extends Model<ECUnitOption> {
// If we really need to modify a props in each `MediaUnit['option']`, use the full version
// (`{baseOption, media}`) in `setOption`.
// For `timeline`, the case is the same.
if (type === 'theme') {
const themeOption = this._theme.option;
if (themeOption) {
mergeTheme(this.option, themeOption, true);
optionChanged = true;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

  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.


if (!type || type === 'recreate' || type === 'timeline') {
const timelineOption = optionManager.getTimelineOption(this);
Expand Down Expand Up @@ -321,7 +328,6 @@ class GlobalModel extends Model<ECUnitOption> {
const replaceMergeMainTypeMap = opt && opt.replaceMergeMainTypeMap;

resetSourceDefaulter(this);

// If no component class, merge directly.
// For example: color, animaiton options, etc.
each(newOption, function (componentOption, mainType: ComponentMainType) {
Expand Down Expand Up @@ -547,7 +553,7 @@ echarts.use([${seriesImportName}]);`);

setTheme(theme: object) {
this._theme = new Model(theme);
this._resetOption('recreate', null);
this._resetOption('theme', null);
}

getTheme(): Model {
Expand Down Expand Up @@ -1011,7 +1017,7 @@ function isNotTargetSeries(seriesModel: SeriesModel, payload: Payload): boolean
}
}

function mergeTheme(option: ECUnitOption, theme: ThemeOption): void {
function mergeTheme(option: ECUnitOption, theme: ThemeOption, preserveUserOptions?: boolean): void {
// PENDING
// NOT use `colorLayer` in theme if option has `color`
const notMergeColorLayer = option.color && !option.colorLayer;
Expand All @@ -1029,7 +1035,9 @@ function mergeTheme(option: ECUnitOption, theme: ThemeOption): void {
if (typeof themeItem === 'object') {
option[name] = !option[name]
? clone(themeItem)
: merge(option[name], themeItem, false);
: preserveUserOptions
? merge(themeItem, option[name], false) // User options have higher priority
Copy link
Member

@100pah 100pah Sep 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

  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.

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

@100pah 100pah Sep 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

: merge(option[name], themeItem, false); // Theme has higher priority
}
else {
if (option[name] == null) {
Expand Down
92 changes: 92 additions & 0 deletions test/theme-switch-keep-data.html
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.