-
Notifications
You must be signed in to change notification settings - Fork 1.4k
refactor: Fetch and cache GutenbergKit editor settings #21791
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
Conversation
Generated by 🚫 Danger |
a2d6293 to
26d42ef
Compare
Project dependencies changeslist! Upgraded Dependencies
org.wordpress.gutenbergkit:android:trunk-fa72e630203e7472d55f4abedfd5c462d2333584, (changed from trunk-a03e0dae10a404c88c215bfcee3176df951302f5)tree +--- project :libs:editor
-| \--- org.wordpress.gutenbergkit:android:trunk-a03e0dae10a404c88c215bfcee3176df951302f5
+| \--- org.wordpress.gutenbergkit:android:trunk-fa72e630203e7472d55f4abedfd5c462d2333584
-\--- org.wordpress.gutenbergkit:android:trunk-a03e0dae10a404c88c215bfcee3176df951302f5 (*)
+\--- org.wordpress.gutenbergkit:android:trunk-fa72e630203e7472d55f4abedfd5c462d2333584 (*) |
|
| App Name | WordPress |
|
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr21791-d99dc81 | |
| Commit | d99dc81 | |
| Direct Download | wordpress-prototype-build-pr21791-d99dc81.apk |
|
| App Name | Jetpack |
|
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr21791-d99dc81 | |
| Commit | d99dc81 | |
| Direct Download | jetpack-prototype-build-pr21791-d99dc81.apk |
.../editor/src/main/java/org/wordpress/android/editor/gutenberg/GutenbergKitEditorFragment.java
Fixed
Show fixed
Hide fixed
.../editor/src/main/java/org/wordpress/android/editor/gutenberg/GutenbergKitEditorFragment.java
Fixed
Show fixed
Hide fixed
.../editor/src/main/java/org/wordpress/android/editor/gutenberg/GutenbergKitEditorFragment.java
Fixed
Show fixed
Hide fixed
| if (editorFragment !is GutenbergKitEditorFragment) { | ||
| refreshEditorContent() | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The timing of this content refresh is currently incompatible with/unnecessary for GutenbergKit.
| annotation class AddOn | ||
|
|
||
| override fun getDbVersion(): Int { | ||
| return 204 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noting that a 204 migration did not exist. I do not believe this is a problem, but noting it just in case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This store was built based on the existing EditorThemeStore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dcalhoun I'm curious why we have this duplication with EditorThemeStore? Do we need two separate stores for editor settings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nbradbury it's an attempt to mitigate regressions.
When the GutenbergKit implementation first began, we colocated much of the new logic with existing editor logic; this included introducing a good amount of branching logic to exiting code. In retrospect, we deemed this misguided, as every change we made in the advancement of GutenbergKit meant the possibility of a regression in the Gutenberg Mobile and Aztec editors. We ultimately separated a good amount of the logic in #21493.
Duplicating EditorThemeStore to EditorSettings felt appropriate for continuing that separation practice, it also allowed for a simpler implementation for GutenbergKit.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #21791 +/- ##
=======================================
Coverage 39.32% 39.32%
=======================================
Files 2125 2125
Lines 99871 99871
Branches 15385 15385
=======================================
Hits 39277 39277
Misses 57114 57114
Partials 3480 3480 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@dcalhoun This isn't something that needs to be addressed here, especially since Semi-related, we may want to make use of the |
@nbradbury thank you for the feedback. This is a great point. I believe the WP-Android app suffers from similar exceptions in certain contexts.
Leveraging the Is there an alternative persistence layer to SQLite that makes sense for this specific feature? Or is SQLite the best choice? For context, the data is site-specific and should likely persist at least a few days. |
I recommend sticking with SQLite, especially for this feature since the data isn't unreasonably large. I've heard other devs recommend storing the data in a separate file and write the filename to the database, but I've never felt the need to do that. |
nbradbury
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! ![]()
|
@nbradbury thank you for the review. Did you have time to also review the sibling PR: wordpress-mobile/GutenbergKit#114? |
bf0f0ca to
234d290
Compare
Mirror theme change subscription.
Avoid typing complex JSON structures.
Communicate active background work to the user.
Improve offline support and performance.
234d290 to
6064d99
Compare
Match caching strategy for other FluxC stores.
This is the expected ID value for the Gutenberg editor.
This reverts commit a715424.
This was added erroneously.
Now that we await the editor settings, we should postpone starting the editor. This means we only start the editor once.
Ensure the editor continues launching if retrieving site-specific editor settings fails.
Apply the cached settings rather than the default settings, if available.
6064d99 to
d99dc81
Compare
|






Fetch and cache editor settings before providing them to the editor, as it
allows improving offline support. The changes also communicate the
background activity with an activity indicator.
Ref CMM-200.
Related:
To Test:
Important
Ensure the following experimental features are enabled:
1: Initial editor fallbacks to default settings
2: Editor fetches site-specific settings
3: Editor fallback to cached settings
Regression Notes
Regressions in Gutenberg Mobile or Aztec editors.
Manually smoke tested the editors.
Deemed unnecessary for the experimental editor.
PR Submission Checklist:
RELEASE-NOTES.txtif necessary.Testing Checklist (strike-out the not-applying and unnecessary ones):