100pah commented on code in PR #20705:
URL: https://github.com/apache/echarts/pull/20705#discussion_r1925440520
##########
src/core/echarts.ts:
##########
@@ -693,10 +685,30 @@ class ECharts extends Eventful<ECEventDefinition> {
}
/**
- * @deprecated
+ * Update theme with name or theme option and repaint the chart.
+ * @param theme Theme name or theme option.
*/
- private setTheme(): void {
- deprecateLog('ECharts#setTheme() is DEPRECATED in ECharts 3.0');
+ setTheme(theme: string | ThemeOption): void {
+ this._updateTheme(theme);
+ if (this._model) {
+ this._model.setTheme(this._theme);
+ // Force refresh to apply theme changes
+ updateMethods.prepareAndUpdate.call(this, {
+ type: 'themeChanged'
+ });
+ }
Review Comment:
After reviewing the implementation of `ECharts#resize`, I updated by
understanding.
+ Whether to trigger a event when an API called? The current approach is
that a parameter `silent` is provided, which is supported by all of the
`ECharts#setOption`, `ECharts#resize` and `ECharts#dispatchAction`. So in
`setTheme` we'd better follow the convention.
+ Whether to call `zr.flush()` to perform zr render before the API return?
+ The current impl is not consistent:
+ `ECharts#setOption`: if not ssr, call `zr.flush()` immediately
+ `ECharts#dispatchAction` provide an parameter `flush` to enable
customization it.
+ `ECharts#resize` does not call `zr.flush()`.
+ If we do not intend to figure this inconsistency this time, I think
that it's OK to follow what `ECharts#resize` does.
+ Whether to use a nested-call-guard: a flag `IN_MAIN_PROCESS_KEY` is used
to prevent from nested calling in both `ECharts#setOption`, `ECharts#resize`
and `ECharts#dispatchAction`. So I think we can follow that if we do not want
to renew the impl.
So IMO the impl of setTheme can be (modified based on the impl of
`ECharts#resize`):
```js
export interface SetThemeOpts {
silent?: boolean // by default false.
};
setTheme(theme: string | ThemeOption, opts?: SetThemeOpts): void {
if (this[IN_MAIN_PROCESS_KEY]) {
if (__DEV__) {
error('`setTheme` should not be called during the main
process.');
}
return;
}
if (this._disposed) {
disposedWarning(this.id);
return;
}
const ecModel = this._model;
if (!ecModel) {
return;
}
let silent = opts && opts.silent;
let updateParams = null as UpdateLifecycleParams;
// There is some real cases that:
// chart.setOption(option, { lazyUpdate: true });
// chart.setTheme('some_theme');
if (this[PENDING_UPDATE]) {
if (silent == null) {
silent = (this[PENDING_UPDATE] as
ECharts[PENDING_UPDATE]).silent;
}
updateParams = (this[PENDING_UPDATE] as
ECharts[PENDING_UPDATE]).updateParams;
this[PENDING_UPDATE] = null;
}
this[IN_MAIN_PROCESS_KEY] = true;
try {
this._updateTheme(theme);
ecModel.setTheme(this._theme);
prepare(this);
updateMethods.update.call(this, {type: 'setTheme'},
updateParams);
}
catch (e) {
this[IN_MAIN_PROCESS_KEY] = false;
throw e;
}
this[IN_MAIN_PROCESS_KEY] = false;
flushPendingActions.call(this, silent);
triggerUpdatedEvent.call(this, silent);
}
```
BTW, it seems that there is some defects in the impl of `ECharts#resize`:
`this[PENDING_UPDATE].updateParams` is omitted in `lazyUpdate` scenario. I've
no idea whether it does that deliberately or to be a mistake.
--
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]
