Introduce the gutenberg mobile editor #8701
Conversation
But encounter the `Can't find variable: Symbol` error even though the standalone RN app works fine.
This reverts commit d5e8e0b73d7e72265a6a196f37b808d05c90b319.
Needed by react-native-svg
Needed by react-native-recyclerview-list.
Needed by react-native-gutenberg-bridge.
This reverts commit 08899b6.
…lag-for-gutenberg-source Binary deps and a flag for gutenberg sources
We are reverting to this method to avoid the brittle situation with having the cyclic dependency between the automated builds (where Travis depends on jitpack for the bridge dep which in turn depends Travis for the JS artifact publishing).
There will be additional work to properly have a UI switch in a different PR.
Besides, it's downloaded automatically during build.
…oid-directly Gb/js bundle in wpandroid directly
Generated by 🚫 Danger |
|
Google Play will not accept updates without 64-bit binaries since August 2019. I know August 2019 seems far away, but just leaving a comment here re: this point:
|
|
Yeap, that's indeed known @mzorz but good idea mentioning it here too. 👍 I'd expect Facebook and the React Native community to deliver 64bits before it becomes a problem 🤞. |
There was a problem hiding this comment.
@hypest I am done with my pass of the PR. I don't think any of my comments are blockers, so I am approving the PR and I'll let you decide which ones, if any, you want to improve in this PR. I am a little concerned about the new fragment, because it'll become very messy to deal with soon. I left a separate comment for that to make it easier to discuss in a thread.
As we talked about it on Slack, I will be afk tomorrow and since this should be merged before Monday, I'll let someone else take over the review if there are any changes. Since none of it is a blocker, it shouldn't be a problem. I'll however follow up with the comments the first chance I get.
Hope the review helped and sorry about being nitpicky and leaving a few too many comments!
P.S: I am holding on to some improvement ideas I had, because I don't think they'd be very helpful in this PR and hopefully if we make a switch to Kotlin, it will much easier to implement them.
| @@ -353,6 +359,8 @@ protected void onCreate(Bundle savedInstanceState) { | |||
| PreferenceManager.setDefaultValues(this, R.xml.account_settings, false); | |||
| // AppPrefs.setAztecEditorAvailable(true); | |||
| // AppPrefs.setAztecEditorEnabled(true); | |||
| mShowGutenbergEditor = false; // hardcode to disabled for now. | |||
There was a problem hiding this comment.
It'd be great if we can introduce an enum for the currently selected editor. It's very hard to tell what each of these combinations of showXEditor does in the code.
Also, I realize that this is following the current pattern, but it'd be good to set these flags (or the enum) in the property declaration and preferably set it to final. If these values are not being changed, we shouldn't have to look into onCreate to find the value for it.
There was a problem hiding this comment.
Makes sense, it has gotten out of hand now that there are 4 editors in place. There's already some good consensus to try to remove 2 of the for by the way. See #8611.
I think I'll create a ticket to revise the flag situation to an enum and tackle it in a separate PR if that's OK.
| @@ -353,6 +359,8 @@ protected void onCreate(Bundle savedInstanceState) { | |||
| PreferenceManager.setDefaultValues(this, R.xml.account_settings, false); | |||
| // AppPrefs.setAztecEditorAvailable(true); | |||
| // AppPrefs.setAztecEditorEnabled(true); | |||
| mShowGutenbergEditor = false; // hardcode to disabled for now. | |||
| // Manually set it to true (and the flags below to false) to force Gutenberg | |||
There was a problem hiding this comment.
I couldn't fully understand what this comment means at the current state of the code, most likely because I don't have the context for it. If this is not actually in reference to the current code, I'd suggest either removing it or making it a // TODO instead so we don't get confused. I prefer creating an issue for this, or adding it as an item to an existing issue, but I don't mind the TODO either.
If we go with the enum approach, I don't think this comment will be necessary anymore.
There was a problem hiding this comment.
Sorry it wasn't clear, the comment is just an attempt to give instructions to someone that would like to try out Gutenberg today by manually enabling it in the code.
There's already work in progress to surface a UI switch to enable/disable the block editor so, I think we could just leave it as is as it's a matter of days to land the proper code. WDYT?
| @@ -1150,7 +1158,8 @@ public boolean onOptionsItemSelected(final MenuItem item) { | |||
| } else { | |||
| // Disable other action bar buttons while a media upload is in progress | |||
| // (unnecessary for Aztec since it supports progress reattachment) | |||
| if (!mShowAztecEditor && (mEditorFragment.isUploadingMedia() || mEditorFragment.isActionInProgress())) { | |||
| if (!(mShowAztecEditor || mShowGutenbergEditor) | |||
There was a problem hiding this comment.
If we go with an enum approach, can we add a property in it for this check, so the logic is easier to follow. It could be something like hasCapabilityX. These minor improvements will help us make an even bigger step later and making the editor selection a sealed class so we can extract all the logic from this fragment.
There was a problem hiding this comment.
This kinda falls out of the intent of this PR so I won't answer with either yes or no. It sounds like a good thing to have in mind for when we refactor this part of the code 👍
| @@ -319,6 +321,10 @@ | |||
| // for keeping the media uri while asking for permissions | |||
| private ArrayList<Uri> mDroppedMediaUris; | |||
|
|
|||
| private boolean isModernEditor() { | |||
There was a problem hiding this comment.
We might be able to get rid of this if we use the enum approach for the editor selection. It might be worth adding some capabilities to the editors instead of having a general check like isModernEditor, so it's both easier to understand why we are checking for a modern editor and easier to customize them if the need arises.
| import java.util.Set; | ||
| import java.util.UUID; | ||
|
|
||
| public class GutenbergEditorFragment extends EditorFragmentAbstract implements |
There was a problem hiding this comment.
It'd be really good if we use Kotlin and ViewModel instead of introducing another massive Java fragment before it's too late. This is of course not a comment on this PR, but just my opinion on the matter going forward. It becomes very hard to refactor these classes after a while and it becomes a nightmare to maintain them if we were to keep it as is.
I realize that Gutenberg team doesn't really get a lot of chances to play with the tools like ViewModel, LiveData and Kotlin coroutines, so I am always happy to help in any way I can if it's something you all are interested in.
There was a problem hiding this comment.
I like the idea of moving to kotlin and VMs as soon as possible, especially since apparently we're already adding features from scratch to Gutenberg's case. Yes, not something to tackle in this PR but defo a good idea to pursue soon!
There was a problem hiding this comment.
Great to hear! As we also talked on the help, I am happy to help in any way I can with this.
| ToastUtils.showToast(getActivity(), R.string.alert_action_while_uploading, ToastUtils.Duration.LONG); | ||
| } | ||
|
|
||
| if (mSource.isFocused()) { |
There was a problem hiding this comment.
Do we want to show double toasts if isActionInProgress() and isFocused() are both true?
| if (mSource.isFocused()) { | ||
| ToastUtils.showToast(getActivity(), R.string.alert_insert_image_html_mode, ToastUtils.Duration.LONG); | ||
| } else { | ||
| getActivity().runOnUiThread(new Runnable() { |
There was a problem hiding this comment.
I think you can add the @MainThread annotation to the method to remove this Runnable.
|
|
||
| setHasOptionsMenu(true); | ||
|
|
||
| mInvalidateOptionsHandler = new Handler(); |
There was a problem hiding this comment.
We can do this initialization during the declaration since it doesn't have any extra benefit to do it during onCreateView since it'll still block the UI. A better approach would be to make it lazy through a method (or lazy modifier in Kotlin 😍 ) so it's not done until it's necessary.
| */ | ||
| @Override | ||
| public CharSequence getContent(CharSequence originalContent) { | ||
| return mWPAndroidGlueCode.getContent(originalContent, new OnGetContentTimeout() { |
There was a problem hiding this comment.
Can we create a property for the OnGetContentTimeout so a new instance is not created each time getContent is called? We should be able to move the Thread.currentThread().interrupt(); to a method which will be called through this property.
| android:inputType="textNoSuggestions|textMultiLine" | ||
| android:layout_height="match_parent" | ||
| android:layout_width="match_parent" | ||
| android:paddingTop="16dp" |
There was a problem hiding this comment.
Nitpick: We can use padding instead since all padding values are the same.
|
@hypest I have gone over my comments as we discussed on the call and I am merging this PR to make it easier to review the changes we might make on them. (since I am afk tomorrow) Could you make sure to either reply or add a reaction to each of my comments just to make sure you have seen them all? Thanks! |
|
Thanks for the review and the merge @oguzkocer ! I have addressed some comments and planning to address or work on the others tomorrow 👍 |
This PR adds the gutenberg-mobile block editor to the main WP Android app. The editor is still in an Alpha stage and so, even though it's added it's not exposed to the UI in any way. There is separate effort to introduce the UI affordances.
Changes introduced by this PR:
wp.BUILD_GUTENBERG_FROM_SOURCEto "switch" to building the app with source level support. This is useful for developing Gutenberg-mobile with fast iteration speed.WordPressmoduleGutenbergEditorFragmentclass file has lots of commented out code still, and it will be cleaned up in a separate PR and while the features are added to Gutenberg editor tooTo test A (no Gutenberg)
wp.BUILD_GUTENBERG_FROM_SOURCEgradle flag set to false or not existing at allDownloading JS bundle from https://s3-us-west-1.amazonaws.com/gutenberg-mobile-js-bundle/wordpress-mobile/gutenberg-mobile/2f024a4e835512dd73b60f9a4ea98081db388543/android/App.js)To test B (Gutenberg forced)
EditPostActivity.javaand setmShowGutenbergEditorto true, andmShowAztecEditorandmShowNewEditorto false to force GutenbergTo test C (compile gutenberg-mobile from source)
git submodule update --init --recursiveto pull all gutenberg related codelibs/gutenberg-mobilefolder and runyarn install && yarn start:reset. This will compile the React Native JS app and start the Metro web server to deliver the JS bundle.wp.BUILD_GUTENBERG_FROM_SOURCEflag totruein the rootgradle.propertiesfile, to enable the source-level build pipelineEditPostActivity.javaand setmShowGutenbergEditorto true, andmShowAztecEditorandmShowNewEditorto false to force Gutenberg