refactor: Separate Gutenberg editor implementations #21493

dcalhoun · 2024-11-25T21:02:33Z

Closes https://github.com/Automattic/dotcom-forge/issues/9831.

Separate the Gutenberg Mobile and GutenbergKit fragments to reduce regression risk when modifying one of the two editors. While the editors do share commonalities, there are also significant differences. We also do not iterate on the editors simultaneously or at the same pace.

Testing Instructions

Test both the Gutenberg Mobile and GutenbergKit editors, attempting to find regressions with foundational features—text editing, media uploads, etc. Toggle the "experimental block editor" feature flags to switch between the two editors.

Regression Notes

Potential unintended areas of impact
Regressions in either of the block editors.
What I did to test those areas of impact (or what existing automated tests I relied on)
Manually tested both editors.
What automated tests I added (or what prevented me from doing so)
None, deemed unnecessary for the experimental editor.

PR Submission Checklist:

I have completed the Regression Notes.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Testing Checklist (strike-out the not-applying and unnecessary ones):

dangermattic · 2024-11-25T21:05:23Z

	3 Warnings
⚠️	This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
⚠️	Class `GutenbergKitEditorFragment` is missing tests, but `unit-tests-exemption` label was set to ignore this.
⚠️	PR is not assigned to a milestone.

Generated by 🚫 Danger

wpmobilebot · 2024-11-25T21:20:03Z

📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.

	App Name	Jetpack
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr21493-e862f40
	Commit	`e862f40`
	Direct Download	`jetpack-prototype-build-pr21493-e862f40.apk`

Note: Google Login is not supported on these builds.

wpmobilebot · 2024-11-25T21:20:34Z

📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.

	App Name	WordPress
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr21493-e862f40
	Commit	`e862f40`
	Direct Download	`wordpress-prototype-build-pr21493-e862f40.apk`

Note: Google Login is not supported on these builds.

github-advanced-security

Android Lint found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

Enable simple checking for both GBM and GBK.

This reverts commit 6bb6165.

There are now separate fragments for "new" and "old" Gutenberg, negating the need for branching logic within the fragments themselves.

The majority--if not all--of this code is unused by GutenbergKit. No-op methods were retained to satisfy the requirements of the class shared between Gutenberg Mobile and GutenbergKit.

The use of "new" to uniquely describe something is problematic, as it provides little context and could become incorrect at some future time.

sonarcloud · 2024-11-26T14:51:24Z

Please retry analysis of this Pull-Request directly on SonarQube Cloud

codecov · 2024-11-26T15:11:08Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 39.44%. Comparing base (0ceef78) to head (e862f40).
Report is 17 commits behind head on trunk.

Additional details and impacted files

@@            Coverage Diff             @@
##            trunk   #21493      +/-   ##
==========================================
- Coverage   39.44%   39.44%   -0.01%     
==========================================
  Files        2121     2119       -2     
  Lines       99564    99556       -8     
  Branches    15313    15313              
==========================================
- Hits        39277    39269       -8     
  Misses      56806    56806              
  Partials     3481     3481

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dcalhoun

I consider the failed SonarQube Cloud result irrelevant as previous failed runs originated from crossing a code duplication threshold. However, the point of these changes is to duplicate code in order to reduce unnecessary risk associated with modifying code shared between two very different implementations.

dcalhoun · 2024-11-26T14:50:06Z

WordPress/build.gradle

@@ -180,8 +180,8 @@ android {
        buildConfigField "boolean", "ENABLE_SITE_MONITORING", "false"
        buildConfigField "boolean", "SYNC_PUBLISHING", "false"
        buildConfigField "boolean", "ENABLE_IN_APP_UPDATES", "false"
-        buildConfigField "boolean", "ENABLE_NEW_GUTENBERG", "false"
-        buildConfigField "boolean", "ENABLE_NEW_GUTENBERG_THEME_STYLES", "false"
+        buildConfigField "boolean", "ENABLE_GUTENBERG_KIT", "false"


The term "new" is problematic, as it provides little context and can become outdated/incorrect in the future. I replaced it with a more meaningful name.

dcalhoun · 2024-11-26T14:51:33Z

WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostActivity.kt

+            when {
+                htmlModeMenuStateOn -> PostEditorAnalyticsSession.Editor.HTML
+                isGutenberg -> PostEditorAnalyticsSession.Editor.GUTENBERG
+                isGutenbergKit -> PostEditorAnalyticsSession.Editor.GUTENBERG_KIT


Introduced a new GutenbergKit-specific analytic indicator to denote the different Gutenberg editors.

dcalhoun · 2024-11-26T14:56:03Z

WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostActivity.kt

+                    if (isGutenbergKitEditor && showGutenbergEditor) {
+                        createGutenbergKitEditorFragment()


This—along with the related fragment files—is the predominant change proposed. The separation is proposed to reduce risk associated with modifying shared logic between the Gutenberg editors, which are diverging and not progressing forward at the same rate.

dcalhoun · 2024-11-26T14:59:55Z

libs/editor/src/main/java/org/wordpress/android/editor/gutenberg/GutenbergEditorFragment.java

These changes should, in theory, restore this file to its state prior to 93c47b1, where it was heavily modified for the addition of GutenbergKit. This theory can be validated in the form of copying and pasting the implementation from the parent commit.

I.e., checking out this branch and pasting the previous implementation into this file in your editor should result in Git showing little-to-no modifications¹ via git diff.

Footnotes

There are a few stylistic changes—spacing, ordering, etc. ↩

checking out this branch and pasting the previous implementation into this file in your editor should result in Git showing little-to-no modifications1

Just an FYI that I had to implement onEditorContentChanged and onOpenMediaLibrary for the previous code to work.

Thanks. That is expected given the current version of the shared interface requires those methods.

dcalhoun · 2024-11-26T15:01:16Z

.../editor/src/main/java/org/wordpress/android/editor/gutenberg/GutenbergKitEditorFragment.java

This new GutenbergKit implementation is devoid of all Gutenberg Mobile-specific logic. Hence, the smaller line numbers and several no-ops satisfying the shared interface. Long term, we might consider severing the shared interface.

dcalhoun · 2024-11-26T15:07:11Z

WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostActivity.kt

Long term, we might also consider how this implementation could be simplified, creating clear and useful separation between Aztec, Gutenberg Mobile, and GutenbergKit.

nbradbury · 2024-11-26T18:42:25Z

@dcalhoun This is unrelated to this PR but wanted to mention it. While testing I noticed the tool buttons don't always appear correctly. Here I've enabled both bold and italic, but the italic button is solid black.

dcalhoun · 2024-11-26T19:36:38Z

While testing I noticed the tool buttons don't always appear correctly. Here I've enabled both bold and italic, but the italic button is solid black.

Thanks for the note. This relates to the way mobile web browsers apply hover styles and will be addressed within https://github.com/Automattic/dotcom-forge/issues/8678.

nbradbury

Changes look good and I'm glad for the separation

sentry-io · 2024-11-29T01:13:59Z

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

‼️ CursorWindowAllocationException: Could not allocate CursorWindow '' of size 20971520 due to error -12. org.wordpress.android.fluxc.persistence.EditorT... View Issue

_{Did you find this useful? React with a 👍 or 👎}

dcalhoun added [Type] Task Gutenberg Editing and display of Gutenberg blocks. labels Nov 25, 2024

dcalhoun added the unit-tests-exemption label Nov 25, 2024

github-advanced-security bot found potential problems Nov 25, 2024

View reviewed changes

dcalhoun added 8 commits November 26, 2024 09:46

refactor: Duplicate Gutenberg editor fragment

2b6a2d4

refactor: Abstract Gutenberg editor fragment detection logic

96b0e1e

Enable simple checking for both GBM and GBK.

refactor: Instantiate GutenbergKitEditorFragment

b843c3c

Revert "refactor: Abstract Gutenberg editor fragment detection logic"

b9278c7

This reverts commit 6bb6165.

refactor: Remove unnecessary "new Gutenberg" branches

4b67a37

There are now separate fragments for "new" and "old" Gutenberg, negating the need for branching logic within the fragments themselves.

refactor: Remove unnecessary code

43a17b5

The majority--if not all--of this code is unused by GutenbergKit. No-op methods were retained to satisfy the requirements of the class shared between Gutenberg Mobile and GutenbergKit.

refactor: Rename "NewGutenberg" to "GutenbergKit"

a8a6937

The use of "new" to uniquely describe something is problematic, as it provides little context and could become incorrect at some future time.

docs: Note no-op reasons

e862f40

dcalhoun force-pushed the refactor/separate-editor-implementations branch from c100ab2 to e862f40 Compare November 26, 2024 14:46

dcalhoun commented Nov 26, 2024

View reviewed changes

dcalhoun marked this pull request as ready for review November 26, 2024 16:43

dcalhoun requested a review from nbradbury November 26, 2024 16:43

nbradbury self-assigned this Nov 26, 2024

nbradbury approved these changes Nov 26, 2024

View reviewed changes

dcalhoun changed the title ~~refactor: Separate editor implementations~~ refactor: Separate Gutenberg editor implementations Nov 26, 2024

dcalhoun merged commit 817841f into trunk Nov 26, 2024
27 of 29 checks passed

dcalhoun deleted the refactor/separate-editor-implementations branch November 26, 2024 19:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Separate Gutenberg editor implementations #21493

refactor: Separate Gutenberg editor implementations #21493

dcalhoun commented Nov 25, 2024 •

edited

Loading

dangermattic commented Nov 25, 2024 •

edited

Loading

wpmobilebot commented Nov 25, 2024 •

edited

Loading

wpmobilebot commented Nov 25, 2024 •

edited

Loading

github-advanced-security bot left a comment

sonarcloud bot commented Nov 26, 2024

codecov bot commented Nov 26, 2024 •

edited

Loading

dcalhoun left a comment

dcalhoun Nov 26, 2024

dcalhoun Nov 26, 2024

dcalhoun Nov 26, 2024

dcalhoun Nov 26, 2024

nbradbury Nov 26, 2024

dcalhoun Nov 26, 2024

dcalhoun Nov 26, 2024

dcalhoun Nov 26, 2024

nbradbury commented Nov 26, 2024

dcalhoun commented Nov 26, 2024

nbradbury left a comment

sentry-io bot commented Nov 29, 2024 •

edited

Loading

		if (isGutenbergKitEditor && showGutenbergEditor) {
		createGutenbergKitEditorFragment()

refactor: Separate Gutenberg editor implementations #21493

refactor: Separate Gutenberg editor implementations #21493

Conversation

dcalhoun commented Nov 25, 2024 • edited Loading

Testing Instructions

Regression Notes

PR Submission Checklist:

Testing Checklist (strike-out the not-applying and unnecessary ones):

dangermattic commented Nov 25, 2024 • edited Loading

wpmobilebot commented Nov 25, 2024 • edited Loading

wpmobilebot commented Nov 25, 2024 • edited Loading

github-advanced-security bot left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Nov 26, 2024

codecov bot commented Nov 26, 2024 • edited Loading

Codecov Report

dcalhoun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Footnotes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbradbury commented Nov 26, 2024

dcalhoun commented Nov 26, 2024

nbradbury left a comment

Choose a reason for hiding this comment

sentry-io bot commented Nov 29, 2024 • edited Loading

Suspect Issues

dcalhoun commented Nov 25, 2024 •

edited

Loading

dangermattic commented Nov 25, 2024 •

edited

Loading

wpmobilebot commented Nov 25, 2024 •

edited

Loading

wpmobilebot commented Nov 25, 2024 •

edited

Loading

codecov bot commented Nov 26, 2024 •

edited

Loading

sentry-io bot commented Nov 29, 2024 •

edited

Loading