100pah commented on code in PR #21201:
URL: https://github.com/apache/echarts/pull/21201#discussion_r2382345940
##########
src/model/Global.ts:
##########
@@ -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);
+ }
+ }
Review Comment:
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?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
