Skip to content

Commit

Permalink
Perf step 0: Do not allow getting connected model (microsoft#2615)
Browse files Browse the repository at this point in the history
* Perf: Do not allow getting connected model

* fix build

* fix build
  • Loading branch information
JiuqingSong authored May 7, 2024
1 parent 9b86a06 commit ea719e1
Show file tree
Hide file tree
Showing 12 changed files with 91 additions and 230 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -141,10 +141,13 @@ export default class InsertEntityPane extends React.Component<ApiPaneProps, Inse
};

private onGetEntities = () => {
const model = this.props.getEditor().getContentModelCopy('connected');
const allEntities: ContentModelEntity[] = [];

findAllEntities(model, allEntities);
this.props.getEditor().formatContentModel(model => {
findAllEntities(model, allEntities);

return false;
});

this.setState({
entities: allEntities.filter(e => !!e),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import { ContentModelDocument } from 'roosterjs-content-model-types';
import { ContentModelDocumentView } from './components/model/ContentModelDocumentView';
import { exportButton } from './buttons/exportButton';
import { importModelButton } from './buttons/importModelButton';
import { refreshButton } from './buttons/refreshButton';
import { Ribbon, RibbonButton, RibbonPlugin } from '../../roosterjsReact/ribbon';
import { SidePaneElementProps } from '../SidePaneElement';

Expand All @@ -15,6 +14,7 @@ export interface ContentModelPaneState {

export interface ContentModelPaneProps extends ContentModelPaneState, SidePaneElementProps {
ribbonPlugin: RibbonPlugin;
refreshButton: RibbonButton<string>;
}

export class ContentModelPane extends React.Component<
Expand All @@ -26,7 +26,7 @@ export class ContentModelPane extends React.Component<
constructor(props: ContentModelPaneProps) {
super(props);

this.contentModelButtons = [refreshButton, exportButton, importModelButton];
this.contentModelButtons = [this.props.refreshButton, exportButton, importModelButton];

this.state = {
model: null,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { ContentModelPane, ContentModelPaneProps } from './ContentModelPane';
import { createRibbonPlugin, RibbonPlugin } from '../../roosterjsReact/ribbon';
import { createRibbonPlugin, RibbonButton, RibbonPlugin } from '../../roosterjsReact/ribbon';
import { getRefreshButton } from './buttons/refreshButton';
import { IEditor, PluginEvent } from 'roosterjs-content-model-types';
import { setCurrentContentModel } from './currentModel';
import { SidePaneElementProps } from '../SidePaneElement';
Expand All @@ -10,10 +11,12 @@ export class ContentModelPanePlugin extends SidePanePluginImpl<
ContentModelPaneProps
> {
private contentModelRibbon: RibbonPlugin;
private refreshButton: RibbonButton<string>;

constructor() {
super(ContentModelPane, 'contentModel', 'Content Model');
this.contentModelRibbon = createRibbonPlugin();
this.refreshButton = getRefreshButton(this);
}

initialize(editor: IEditor): void {
Expand All @@ -33,13 +36,7 @@ export class ContentModelPanePlugin extends SidePanePluginImpl<
}

onPluginEvent(e: PluginEvent) {
if (e.eventType == 'contentChanged' && e.source == 'RefreshModel') {
this.getComponent(component => {
const model = this.editor.getContentModelCopy('connected');
component.setContentModel(model);
setCurrentContentModel(model);
});
} else if (
if (
e.eventType == 'input' ||
e.eventType == 'selectionChanged' ||
e.eventType == 'editorReady'
Expand All @@ -59,6 +56,7 @@ export class ContentModelPanePlugin extends SidePanePluginImpl<
...baseProps,
model: null,
ribbonPlugin: this.contentModelRibbon,
refreshButton: this.refreshButton,
};
}

Expand All @@ -68,11 +66,20 @@ export class ContentModelPanePlugin extends SidePanePluginImpl<
}
};

private onModelChange = () => {
onModelChange = (force?: boolean) => {
this.getComponent(component => {
const model = this.editor.getContentModelCopy('connected');
component.setContentModel(model);
setCurrentContentModel(model);
this.editor.formatContentModel(
model => {
component.setContentModel(model);
setCurrentContentModel(model);

return false;
},
undefined,
{
tryGetFromCache: !force,
}
);
});
};
}
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
import { ContentModelPanePlugin } from '../ContentModelPanePlugin';
import { RibbonButton } from '../../../roosterjsReact/ribbon';

export const refreshButton: RibbonButton<'buttonNameRefresh'> = {
key: 'buttonNameRefresh',
unlocalizedText: 'Refresh',
iconName: 'Refresh',
onClick: editor => {
editor.triggerEvent('contentChanged', {
source: 'RefreshModel',
});
},
};
export function getRefreshButton(
plugin: ContentModelPanePlugin
): RibbonButton<'buttonNameRefresh'> {
return {
key: 'buttonNameRefresh',
unlocalizedText: 'Refresh',
iconName: 'Refresh',
onClick: () => {
plugin.onModelChange(true /*force*/);
},
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,11 @@ export function paste(
const trustedHTMLHandler = editor.getTrustedHTMLHandler();

if (!clipboardData.modelBeforePaste) {
clipboardData.modelBeforePaste = cloneModelForPaste(
editor.getContentModelCopy('connected')
);
editor.formatContentModel(model => {
clipboardData.modelBeforePaste = cloneModelForPaste(model);

return false;
});
}

// 1. Prepare variables
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,39 +18,38 @@ export const createContentModel: CreateContentModel = (core, option, selectionOv
// Flush all mutations if any, so that we can get an up-to-date Content Model
core.cache.textMutationObserver?.flushMutations();

let cachedModel =
selectionOverride || (option && !option.tryGetFromCache) ? null : core.cache.cachedModel;

if (cachedModel && core.lifecycle.shadowEditFragment) {
// When in shadow edit, use a cloned model so we won't pollute the cached one
cachedModel = cloneModel(cachedModel, { includeCachedElement: true });
}

if (cachedModel) {
return cachedModel;
} else {
const selection =
selectionOverride == 'none'
? undefined
: selectionOverride || core.api.getDOMSelection(core) || undefined;
const saveIndex = !option && !selectionOverride;
const editorContext = core.api.createEditorContext(core, saveIndex);
const settings = core.environment.domToModelSettings;
const domToModelContext = option
? createDomToModelContext(editorContext, settings.builtIn, settings.customized, option)
: createDomToModelContextWithConfig(settings.calculated, editorContext);

if (selection) {
domToModelContext.selection = selection;
if (!selectionOverride && (!option || option.tryGetFromCache)) {
const cachedModel = core.cache.cachedModel;

if (cachedModel) {
// When in shadow edit, use a cloned model so we won't pollute the cached one
return core.lifecycle.shadowEditFragment
? cloneModel(cachedModel, { includeCachedElement: true })
: cachedModel;
}
}

const model = domToContentModel(core.logicalRoot, domToModelContext);
const selection =
selectionOverride == 'none'
? undefined
: selectionOverride || core.api.getDOMSelection(core) || undefined;
const saveIndex = !option && !selectionOverride;
const editorContext = core.api.createEditorContext(core, saveIndex);
const settings = core.environment.domToModelSettings;
const domToModelContext = option
? createDomToModelContext(editorContext, settings.builtIn, settings.customized, option)
: createDomToModelContextWithConfig(settings.calculated, editorContext);

if (selection) {
domToModelContext.selection = selection;
}

if (saveIndex) {
core.cache.cachedModel = model;
updateCachedSelection(core.cache, selection);
}
const model = domToContentModel(core.logicalRoot, domToModelContext);

return model;
if (saveIndex) {
core.cache.cachedModel = model;
updateCachedSelection(core.cache, selection);
}

return model;
};
9 changes: 1 addition & 8 deletions packages/roosterjs-content-model-core/lib/editor/Editor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,6 @@ export class Editor implements IEditor {
/**
* Create Content Model from DOM tree in this editor
* @param mode What kind of Content Model we want. Currently we support the following values:
* - connected: Returns a connect Content Model object. "Connected" means if there is any entity inside editor, the returned Content Model will
* contain the same wrapper element for entity. This option should only be used in some special cases. In most cases we should use "disconnected"
* to get a fully disconnected Content Model so that any change to the model will not impact editor content.
* - disconnected: Returns a disconnected clone of Content Model from editor which you can do any change on it and it won't impact the editor content.
* If there is any entity in editor, the returned object will contain cloned copy of entity wrapper element.
* If editor is in dark mode, the cloned entity will be converted back to light mode.
Expand All @@ -100,11 +97,7 @@ export class Editor implements IEditor {
const core = this.getCore();

switch (mode) {
case 'connected':
return core.api.createContentModel(core, {
tryGetFromCache: true, // Pass an option here to force disable save index
});

case 'connected': // Get a connected model is deprecated. Now we will always return disconnected model
case 'disconnected':
return cloneModel(
core.api.createContentModel(core, {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ describe('createContentModel with selection', () => {
/*
| Scenarios | can use cache | can write cache | comment |
|-----------------------------------|---------------|-----------------|----------------------------------------------------------------------------------------------------------------|
| getContentModelCopy: connected | true | false | Mostly used by demo site, we can use existing model but this should not impact cache |
| getContentModelCopy: connected | false | false | This is now deprecated |
| getContentModelCopy: disconnected | false | false | Used by plugins and test code to read current model. We will return a cloned model, and do not impact cache |
| getContentModelCopy: clean | false | false | Used by export HTML, do not use cache to make sure the model is up to date |
| formatInsertPointWithContentModel | false | false | Used by insertEntity (recent change), do not use cache since we need to add shadow insert point |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,13 +135,8 @@ describe('Editor', () => {

const editor = new Editor(div);

const model1 = editor.getContentModelCopy('connected');

expect(model1).toBe(mockedModel);
expect(createContentModelSpy).toHaveBeenCalledWith(mockedCore, { tryGetFromCache: true });

editor.dispose();
expect(() => editor.getContentModelCopy('connected')).toThrow();
expect(() => editor.getContentModelCopy('disconnected')).toThrow();
expect(resetSpy).toHaveBeenCalledWith();
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ describe(ID, () => {

paste(editor, clipboardData, 'asImage');

const model = editor.getContentModelCopy('connected');
const model = editor.getContentModelCopy('disconnected');
const width = editor.getDOMHelper().getClientWidth();

expect(model).toEqual({
Expand All @@ -60,6 +60,10 @@ describe(ID, () => {
maxWidth: `${width}px`,
},
dataset: {},
alt: undefined,
title: undefined,
isSelectedAsImageSelection: undefined,
isSelected: undefined,
},
{
segmentType: 'SelectionMarker',
Expand All @@ -80,6 +84,9 @@ describe(ID, () => {
},
],
format: {},
cachedElement: undefined,
isImplicit: undefined,
segmentFormat: undefined,
},
],
format: {},
Expand Down
10 changes: 6 additions & 4 deletions packages/roosterjs-content-model-types/lib/editor/IEditor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,19 +23,21 @@ import type { EntityState } from '../parameter/FormatContentModelContext';
* An interface of Editor, built on top of Content Model
*/
export interface IEditor {
/**
* @deprecated Use formatContentModel() instead
*/
getContentModelCopy(mode: 'connected'): ContentModelDocument;

/**
* Create Content Model from DOM tree in this editor
* @param mode What kind of Content Model we want. Currently we support the following values:
* - connected: Returns a connect Content Model object. "Connected" means if there is any entity inside editor, the returned Content Model will
* contain the same wrapper element for entity. This option should only be used in some special cases. In most cases we should use "disconnected"
* to get a fully disconnected Content Model so that any change to the model will not impact editor content.
* - disconnected: Returns a disconnected clone of Content Model from editor which you can do any change on it and it won't impact the editor content.
* If there is any entity in editor, the returned object will contain cloned copy of entity wrapper element.
* If editor is in dark mode, the cloned entity will be converted back to light mode.
* - clean: Similar with disconnected, this will return a disconnected model, the difference is "clean" mode will not include any selection info.
* This is usually used for exporting content
*/
getContentModelCopy(mode: 'connected' | 'disconnected' | 'clean'): ContentModelDocument;
getContentModelCopy(mode: 'disconnected' | 'clean'): ContentModelDocument;

/**
* Get current running environment, such as if editor is running on Mac
Expand Down
Loading

0 comments on commit ea719e1

Please sign in to comment.