Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix cache issue #2547

Merged
merged 7 commits into from
Apr 1, 2024
Merged

Fix cache issue #2547

merged 7 commits into from
Apr 1, 2024

Conversation

JiuqingSong
Copy link
Contributor

@JiuqingSong JiuqingSong commented Mar 29, 2024

To repro:

  1. Open demo site, turn on side pane
  2. Type "abc"
  3. Type ENTER
  4. Type BACKSPACE, keep doing it until everything is gone
  5. Keep typing BACKSPACE

Expect: nothing happens
Actual, "ab" reappears

The root cause is demo site will always update content model by calling getContentModelCopy() which will update model cache before our formatContentModel() API write cache back, it is triggered when backspace. So the result is cached model in DOM tree is updated by demo site, but cached model in editor is updated by formatContentModel(), so the two models don't match. Later when we keep editing, it will edit on a staled cache.

To fix it, we should not update cache when create model from demo site. Furthermore, we should not update cache at all from API getContentModelCopy. It should be a "readonly" operation, to create model only, but not touch cache at all.

In this change I explicitly specified if a API call should reuse cache, and as long as any option is passed in, we should not update cache. For more detailed summary:

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: 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
getFormatState true false We can reuse cache if we have, but when there is no cache, we will create reduced model so do not impact cache
other formatContentModel cases true true Normal case, we can reuse cache, and should update cache

I added test cases to ensure the result of this table.

Also fix another recent regression in Editor.getContentModelCopy() with parameter "clean", we need to clone the content model to avoid removing entity DOM element when write back to DOM. Related test cases added.

@JiuqingSong JiuqingSong merged commit 7130d12 into master Apr 1, 2024
7 checks passed
@JiuqingSong JiuqingSong deleted the u/jisong/cacheissue branch April 1, 2024 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants