100pah commented on code in PR #21201:
URL: https://github.com/apache/echarts/pull/21201#discussion_r2383960074
##########
src/model/Global.ts:
##########
@@ -1039,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
Review Comment:
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.
--
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]
