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 5 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
16 changes: 13 additions & 3 deletions src/model/Global.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,14 +242,14 @@ class GlobalModel extends Model<ECUnitOption> {
* @return Whether option changed.
*/
resetOption(
type: 'recreate' | 'timeline' | 'media',
type: 'recreate' | 'timeline' | 'media'| 'theme',
opt?: Pick<GlobalModelSetOptionOpts, 'replaceMerge'>
): boolean {
return this._resetOption(type, normalizeSetOptionInput(opt));
}

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) {
optionChanged = true;
this._mergeOption({ theme: themeOption }, opt);
}
}
Copy link
Member

@100pah 100pah Sep 26, 2025

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

@mustcanbedo mustcanbedo Sep 26, 2025

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 @@ -322,6 +329,9 @@ class GlobalModel extends Model<ECUnitOption> {

resetSourceDefaulter(this);

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

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

getTheme(): Model {
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.