[REF] Master simplify model startup vsc #4160
Conversation
|
robodoo rebase-ff |
|
Merge method set to rebase and fast-forward. |
| @@ -27,7 +27,7 @@ export interface UIPluginConfig { | |||
| readonly custom: ModelConfig["custom"]; | |||
| readonly session: Session; | |||
| readonly defaultCurrencyFormat?: Format; | |||
| readonly customColors: Color[]; | |||
| // readonly customColors: Color[]; | |||
| for (let Plugin of coreViewsPluginRegistry.getAll()) { | ||
| const plugin = this.setupUiPlugin(Plugin); | ||
| this.coreViewsPlugins.push(plugin); | ||
| const plugin = this.setupCoreUiPlugin(Plugin); |
There was a problem hiding this comment.
I didn't think coreViewPlugin was a very good name, but IMO CoreUIPlugin is even more confusing.
Anyway the registry should also be renamed if we want to change the nomenclature
| for (let name of Plugin.getters) { | ||
| if (!(name in plugin)) { | ||
| throw new Error(`Invalid getter name: ${name} for plugin ${plugin.constructor}`); | ||
| } | ||
| if (name in this.getters) { | ||
| throw new Error(`Getter "${name}" is already defined.`); | ||
| } | ||
| this.getters[name] = plugin[name].bind(plugin); | ||
| } |
There was a problem hiding this comment.
Any way to extract this loop in a method ? It's now copy/pasted 3 times in this file
There was a problem hiding this comment.
the core_ui_plugins where of type UIPlugin, which is not correct
could you elaborate in the commit message why it's not correct?
It's actually not clear to me why it's not correct. Core UI plugins still handle UI commands, can use all getters. Except what's available in the plugin config, nothing changed much.
They should probably not be able to handle UI commands, but then this commit is not enough (there's currently a few exceptions: START, EVALUATE_CELLS, REFRESH_PIVOT, others?).
Also note that moving the evaluation to a worker will be very hard, even impossible. It depends on a loooot of things, especially for odoo pivots (rpc, web pivot model, and more)
| @@ -587,13 +598,13 @@ export class Model extends EventBus<any> implements CommandDispatcher { | |||
| private dispatchToHandlers(handlers: CommandHandler<Command>[], command: Command) { | |||
| const isCommandCore = isCoreCommand(command); | |||
| for (const handler of handlers) { | |||
| if (!isCommandCore && handler instanceof CorePlugin) { | |||
| if (!isCommandCore && this.corePlugins.includes(handler as any)) { | |||
There was a problem hiding this comment.
Looks like this is not improving performance :/
I tried autofilling 10k rows twice.
before: Replayed 19998 commands in 1266 ms
after: Replayed 19998 commands in 1412 ms => ~11% more
(average over 6 runs)
Maybe we can find another way
There was a problem hiding this comment.
yes, I'm looking into it.
| @@ -21,7 +21,7 @@ interface CellWithSize { | |||
| size: Pixel; | |||
| } | |||
|
|
|||
| export class HeaderSizeUIPlugin extends UIPlugin<HeaderSizeState> implements HeaderSizeState { | |||
There was a problem hiding this comment.
note that this plugin depends on DOM api to create a canvas element. We won't be able to move it to a Worker as it is now, we'll have to refactor a bit more here.
private ctx = document.createElement("canvas").getContext("2d")
There was a problem hiding this comment.
it's only used to compute text size, it can be done in an offscreen canvas in a worker I think :)
There was a problem hiding this comment.
I didn't know OffscreenCanvas were a thing
Simplify the creation and binding on the history object. This is preliminary work for separating Model.ts into multiple independant parts. Task: 3637435
Before this commit, the core_ui_plugins where of type UIPlugin, which is not correct After this commit, they have their own type: CoreUiPlugin, which is basically a core plugin that has access to all getters Task: 3637435
Before this commit, the Custom Colors plugins was using its history for a transient value. After this commit, it uses its local property without history.
…e for basically each command, nothing works anymore.
…ry change for basically each command," This reverts commit 62177fa.
This reverts commit 87ef2f6.
VincentSchippefilt
force-pushed
the
master-simplify-model-startup-vsc
branch
from
4e511f5
to
eb4af98
Compare
Description:
preliminary work to split the model into core_model and all_the_rest_model
Task: : 3637435 (https://www.odoo.com/web#id=3637435&action=333&active_id=2328&model=project.task&view_type=form&cids=1&menu_id=4720)
review checklist