fix: Improve autosave and unsaved changes detection#104
Conversation
Previously, host apps determined whether content changes were present by comparing the content received from GutenbergKit to the latest server-provided post content. This is problematic as the server makes slight modifications to content markup, creating perpetually changed content. To address this, we need GutenbergKit to communicate changed content. We accomplish by sending a new `changed` parameter alongside the title and content. Rather than updating the post and content refs on every content change, we now do so at the time at which the host app requests the latest content. This ensures the host and GutenbergKit synchronize their latest perception of changes. We also now rely upon `AutosaveMonitor` to throttle how often we request the host app persist the latest changes, rather than a generic store subscription.
dcalhoun
force-pushed
the
fix/improve-autosave-and-unsaved-changes-detection
branch
from
a014374 to
131c5fd
Compare
We must register blocks prior to attempts to serialize the raw post content within the editor. This change was not tested. It should be tested once we re-engage work on the remote editor.
| receiveEntityRecords( 'postType', post.type, post ); | ||
|
|
||
| setupEditor( post, {} ); | ||
| registerCoreBlocks(); |
There was a problem hiding this comment.
This was relocated to the entry file as the blocks must be registered before we parse the raw post content provided by the host app.
| if ( postContentRef.current === null ) { | ||
| postContentRef.current = serialize( parse( post.content.raw || '' ) ); | ||
| } |
There was a problem hiding this comment.
Explicitly selectively instantiated to avoid unnecessary performance costs.
| if ( changed ) { | ||
| postTitleRef.current = title; | ||
| postContentRef.current = content; | ||
| } |
There was a problem hiding this comment.
Selectively updating these refs allows us to accurately communicate a "content has changed" status to the host app, which is important for WordPress-iOS' implementation.
| useEffect( () => { | ||
| return subscribe( () => { | ||
| const { title, content } = window.editor.getTitleAndContent(); | ||
| if ( | ||
| title !== postTitleRef.current || | ||
| content !== postContentRef.current | ||
| ) { | ||
| onEditorContentChanged(); | ||
| postTitleRef.current = title; | ||
| postContentRef.current = content; | ||
| } | ||
| } ); | ||
| }, [] ); |
There was a problem hiding this comment.
This generic subscription is replaced by the AutosaveMonitor, which provides a more robust implementation, including intervals and dirty checks.
There was a problem hiding this comment.
Approved as it's an improvement. I would, however, considering implementing it in a slightly different way:
- The app initialize the editor with the initial title and content
- The editor keeps track of whether the post has changes (title/content) and communicates it via a new property
EditorState/hasChanges. - If the app receives an update with
hasChangesset totrue, it only then triggers the autosave logic to put data on disk. Autosave happens infrequently and only as a crash-protection measure. This is the only time the app needs to get the latest content from the editor. - The app never changes the title or content on its side. If it needs to, it uses one of the editor bridge APIs to do so (e.g. when restoring from a previous revision)
This setup will minimize the need to pass serialized content via the bridge and establish the source of truth for the content – the editor. I might be missing something, but it seems sound.
android/Gutenberg/src/main/java/org/wordpress/gutenberg/GutenbergView.kt
Show resolved
Hide resolved
My perception is that our approach accomplishes these two aspect currently. Please correct me if I'm wrong.
Given the As
I agree avoiding unnecessary bridge communication is a worthwhile target. However, it's unclear to me whether the proposed changes would reduce communication. That said, I am very open to refactors to reduce complexity. I'd just like to understand the proposal better—if you are willing to open pull requests expressing the idea, even better. |
One of the reasons would be to ensure that the "Publish/Save" button updates (nearly) instantly after you make a change, otherwise it feels a bit slow. It's, of course, not the end of the world. In terms of the performance, my assumption is that serializing and sending the entire post content on every keystone is not feasible, especially with larger posts. Perhaps, there is an acceptable setting that balances performance and responsiveness. Unfortunately, I'm not sure how to measure it, but I'm suggesting a potential design where no serialization is needed*, so it never becomes a bottleneck. The logic that responds to changes and determines if the content is different from the initial content will live entirely in the JS land. *it will only be needed once every ~10 seconds for autosave |
I agree, this would be nice. I'll note that Gutenberg Mobile (GBM) does not perform this way at the moment.
I understood the intent; I fail to understand how your proposal's execution differs from what I shared, specifically regarding performance. My perception now is that the critical piece you suggest is immediately updating UI based on the I believe accomplishing immediate updates for the UI involves a larger refactor of code untouched in the proposed changes. Namely, the host manages the enablement of the Publish/Update button via changes to the post |
Yeah, the way it uses
Yeah, maybe it's OK. In terms of UX, I'd say it feels fine. Performance – not sure how to measure. I think you convinced me that your version is ultimately better. |
dcalhoun
deleted the
fix/improve-autosave-and-unsaved-changes-detection
branch
2 participants
Related:
What?
Improve autosave and change detection logic, aligning more closely with the
approach of Gutenberg Mobile.
Why?
Previously, host apps determined whether content changes were present by
comparing the content received from GutenbergKit to the latest server-provided
post content. This is problematic as the server makes slight modifications to
content markup, creating perpetually changed content.
How?
To address this, we need GutenbergKit to communicate changed content. We
accomplish by sending a new
changedparameter alongside the title and content.Rather than updating the post and content refs on every content change, we now
do so at the time at which the host app requests the latest content. This
ensures the host and GutenbergKit synchronize their latest perception of
changes.
We also now rely upon
AutosaveMonitorto throttle how often we request thehost app persist the latest changes, rather than a generic store subscription.
Testing Instructions
See sibling PRs for platform-specific testing instructions:
Accessibility Testing Instructions
N/A, no user-facing changes.
Screenshots or screencast
N/A, no user-facing changes.