Rework EditorThemeStore to be stored in CoreData #16282

chipsnyder · 2021-04-12T18:50:47Z

Fixes wordpress-mobile/gutenberg-mobile#3163
This change fundamentally reworks how the EditorThemeStore works so this should solve the issue around #16014 as well.

Related PRs:

Gutenberg: [Task] Update theme updates to use the approach that guarantees delivery gutenberg-mobile#3376
Gutenberg-Mobile: [Task] Update theme updates to use the approach that guarantees delivery gutenberg-mobile#3376
WordPressKit: Adds a wrapper around the dotCom and Dot Org Rest API WordPressKit-iOS#383

Description

This PR reworks the current approach used to fetch and cache the Editor theme defined in theme_supports and reworks it into a structure that more closely aligns with the Android implementation. This doesn't do anything for GSS yet but I did add a few things in preparation for that set of work which I'll build once this merges.

To test:

This can be tested by running the regression test suite:

- [ ] Default Colors - Check that default colors still load - [steps](https://github.com/wordpress-mobile/test-cases/blob/master/test-cases/gutenberg/editor-theme.md#tc001) 
- [ ] Default Gradients - Check that default gradients still load - [steps](https://github.com/wordpress-mobile/test-cases/blob/master/test-cases/gutenberg/editor-theme.md#tc002)
- [ ] Custom Colors - Check that custom colors load in the editor - [steps](https://github.com/wordpress-mobile/test-cases/blob/master/test-cases/gutenberg/editor-theme.md#tc003)
- [ ] Custom Gradients - Check that custom gradients load in the editor - [steps](https://github.com/wordpress-mobile/test-cases/blob/master/test-cases/gutenberg/editor-theme.md#tc004)
- [ ] Offline Support - [steps](https://github.com/wordpress-mobile/test-cases/blob/master/test-cases/gutenberg/editor-theme.md#tc005)

Core Migrations

develop has the same CoreData model as the latest release so installing this build over a build from develop or the last release will be sufficient for testing any migrations.

Regression Notes

Potential unintended areas of impact
The only other possible area would be CoreData migration conflicts.
What I did to test those areas of impact (or what existing automated tests I relied on)
Same as the testing notes.
What automated tests I added (or what prevented me from doing so)
Added unit tests around the affected code.

PR submission checklist:

I have completed the Regression Notes.
I have considered adding unit tests for my changes.
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.

peril-wordpress-mobile · 2021-04-12T18:50:54Z

You can test the changes on this Pull Request by downloading it from AppCenter here with build number: 45524. IPA is available here. If you need access to this, you can ask a maintainer to add you.

peril-wordpress-mobile · 2021-04-12T18:50:55Z

	Warnings
⚠️	PR is not assigned to a milestone.
⚠️	PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

peril-wordpress-mobile · 2021-04-12T19:06:13Z

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

chipsnyder · 2021-04-14T17:48:08Z

WordPress/Classes/Models/BlockEditorSettings+CoreDataProperties.swift

+
+    /// Stores a n MD5 checksum representing the stored data. Used for a comparison to decide if the data has changed.
+    ///
+    @NSManaged public var checksum: String


📓 GSS doesn't really have a notion of a version to indicate if there are changes or not. I don't want to trigger a theme change if the data hasn't been modified. So Instead I generate and store a checksum to compare change sets.

Is this regarding avoiding re-renders after the async fetch response after the editor loads? My reason for asking is, I wonder if we should handle that on the JS side (either instead of or in addition to this). Benefits there are that with a partial change would only re-render the dependent components.

Is this regarding avoiding re-renders after the async fetch response after the editor loads?

Yup, in the old model, we were using a combination of the theme name and version number for the theme as a key to meet the same goal. Since themes that support FSE can be more easily modified without a change to the underlying theme, I felt that the original structure wouldn't be workable, so I updated this now since it's tied to the CoreDataModel changes.

My reason for asking is, I wonder if we should handle that on the JS side (either instead of or in addition to this).

This is a good discussion topic to revisit. Just to align on expectations, plan, and set the scope that this PR is focused on updating the underlying storing mechanism for custom colors and gradients, this is not intended to touch the expectations RN is making about the received data.

With that out of the way :). I would love to continue this conversation and let this PR move forward. We can tackle any outcomes in a future iteration, but let me know if that plan doesn't work.

Apologies for the novella; this would be a great chat over a cup of coffee ☕

Personally, I would be against starting with the instead mindset, but I would want to ask that question again after seeing a POC for the addition to mindset. Where we should do the check was briefly discussed in slack on the initial implementation. We felt handling this on the JS side would start to become very complicated, and although doable, we weren't sure any of us had a plan that would make it feel clean. If a parent object were to be updated like the main canvas, then TMK of RN all of the child components have to re-render given RN's base architecture. So to handle conditional renders, we would need to build in a mechanism at each layer so it can decide if it should rerender or not. We also don't expect the theme to change frequently, so I feel the work to support that wouldn't be worth the effort of taking it on globally on the Native side. If you have a vision for handling this, I would love to see a sample branch with a POC when you have time. It would help me learn a lot.

That approach to handle it on the RN side also raised a few questions about how we wanted the Bridge to communicate its expectations.

We created the bridge with two interfaces related to accepting these initial settings.

The first is the intialProperties which accepts basic theme data. I'm using "theme" to refer to custom colors and gradients because that's all we currently use, but I believe this structure works for font sizes as well, and we can examine other use cases as they arise.

The Bridge accepts that the data provided in [intialProperties](https://github.com/WordPress/gutenberg/blob/661548ae5f1965f5fbff1c40b54fd37fa57a6b79/packages/react-native-bridge/ios/Gutenberg.swift#L82-L89 might be stale data and offers the mechanism updateTheme to provide a way for the Native Side to say that it now has fresh data.

I'm not sure what the best interface would be if we shifted the "ownership" of the detecting changes to the Editor.
If we had the editor decide if it should apply those changes or not, the current interface would no longer be a guarantee that the new settings being sent are actually applied. If we removed the acceptance that the initial properties could be stale, this would increase the editor's load time.

Apologies for the novella; this would be a great chat over a cup of coffee coffee

No need for apologies! Thanks for taking the time to write out your thoughts here, it's great to get your perspective on this. And btw, I am having a cup of coffee as I read this and write a response, so this is like an async chat over a cup of coffee 😄 ☕ ☕

Personally, I would be against starting with the instead mindset, but I would want to ask that question again after seeing a POC for the addition to mindset. Where we should do the check was briefly discussed in slack on the initial implementation. We felt handling this on the JS side would start to become very complicated, and although doable, we weren't sure any of us had a plan that would make it feel clean.

That makes sense. Especially so if the relevant optimizations are not already present on the JS side. Even if they were, though, the checksum here would still add value, I think, since it could "preempt" such calculations, and in many cases avoid them entirely.

If you have a vision for handling this, I would love to see a sample branch with a POC when you have time. It would help me learn a lot.

I know there have been some recent improvements regarding re-renders of the whole block-list, but also that there were still plenty of things that could use improvements there. My vision for handling this is that we'd have our various HOCs / providers optimized in such a way that this kind of thing is not even a concern. Ideally, we should be able to pass whatever necessary props / data to the main container component, and only necessary updates to the hierarchy would take place. But, I fully understand we may not be there yet at the moment, since out-of-the-box things like memo won't fly for our recursive and often stateful child components.

In the context of this PR, which is more about theme settings specifically, I also agree that if there was much needed work to make this performant on the RN side, the payoff wouldn't be worth it, since the changes would likely affect many blocks in the editor (i.e. close to an all-or-nothing re-render anyway). My mindset about this is oriented toward the needs for editor settings in general, which seem to be more frequently cropping up. My thinking is that sometime in the near future, we might have a setting which is updated independently from the theme settings. Imagine, for example, if we had a flag in the settings with a new value in the endpoint response. In that case we'd want to avoid the full re-render that a theme update would trigger. We could implement custom logic in Swift and Kotlin to condition that, but it'd be great if the app was already optimized to be idempotent with that granularity.

With all that said, there is nothing in this PR that is incompatible with the ideal RN optimizations, so please consider this discussion as entirely non-blocking. It seems that the complications on the RN side were already discussed at the outset, so any improvements there probably deserve their own space for discussion. And thanks again for elaborating in detail about your thought process!

this is like an async chat over a cup of coffee 😄 ☕ ☕
😄 I have my coffee now too.

may not be there yet at the moment, since out-of-the-box things like memo won't fly for our recursive and often stateful child components.

Yeah if we can take on this problem then we'd really be able to optimize the codebase. That would be awesome.

Thank you for bringing up those other ides and responses :)

chipsnyder · 2021-04-14T17:48:53Z

WordPress/Classes/Models/BlockEditorSettings+CoreDataProperties.swift

+
+    /// Stores a date indicating the last time stamp that the settings were modified. 
+    ///
+    @NSManaged public var lastUpdated: Date


📓 This is unused but I'm storing it because I would like to eventually be smarter about how often we're fetching the theme.

chipsnyder · 2021-04-14T17:50:24Z

WordPress/Classes/Models/EditorTheme.swift

+        self.checksum = {
+            guard let parsedTheme = parsedTheme else { return "" }
+            let encoder = JSONEncoder()
+            encoder.outputFormatting = .sortedKeys


📓 While generating the checksum, this guarantees that the keys are ordered consistently.

mkevins · 2021-04-15T06:59:03Z

WordPress/Classes/Models/BlockEditorSettingElement+CoreDataProperties.swift

+
+    /// Stores a unique key associated to the `value`.
+    ///
+    @NSManaged public var slug: String


I appreciate that this is being implemented in a somewhat general way, with thought of future settings. I recently learned that there will be a need for an experimental feature flag setting for the Gallery block refactor work as well: https://github.com/WordPress/gutenberg/pull/25940/files#diff-e6bd94e1847aea4b70a6cc1ac9b2471dd3d882f6cbf1ab51438e23b02ca5194fR114-R128, but I'm not sure it fits this model, since there is no slug. Then again, there is no REST API yet for that, so perhaps id could be passed as slug 🤔 .

In any case, I'm mentioning here because there seem to a be a few upcoming cases with similar requirements (patterns will also need something similar, for example).

Edit
Nvm, just saw this comment. So, the element is meant to be general, but only a part of the BlockEditorSettings, which could easily store the additional experimental flag as well. Does that sound right?

So, the element is meant to be general, but only a part of the BlockEditorSettings, which could easily store the additional experimental flag as well. Does that sound right?

Yup this is meant to be a more generic container for the arrays that come down from the call /wp/v2/themes?status=active and the one being added to /wp-block-editor/v1/settings in wordpress-mobile/gutenberg-mobile#3163 the parent object is the BlockEditorSettings

The main uses I see for BlockEditorSettingElement are colors, gradients, and font sizes. If we got to a use case that doesn't have a need for one of the required values then we can provide an empty string. This would all be about how the consumer decides to use the stored data.

I recently learned that there will be a need for an experimental feature flag setting for the Gallery block refactor work as well ... Then again, there is no REST API yet for that, so perhaps id could be passed as slug 🤔 .

Let's dig into this use case more once we have an idea about how it will be delivered to the app. If it's included in the call /wp-block-editor/v1/settings my recommendation would be to add it to BlockEditorSettings but let's see.

chipsnyder · 2021-04-16T13:56:11Z

@mkevins Thanks for taking the time to review. Let me know if you have any other thoughts. @antonis if you have time can you also take a look here. Since this has CoreData changes I'd like to get this merged soon to prevent any collisions by being branched out for too long.

antonis

Great work @chipsnyder 👍
I tested this on an iPhone SE 2020 with iOS 14.4.1 and all work as expected:
✅ Default Colors
✅ Default Gradients
✅ Custom Colors
✅ Custom Gradients
✅ Offline Support
The code and the tests LGTM 🎉
Thank you for paving the way for the Android implementation 🙇

Add Core Data Structure for Block Settings

26b9713

chipsnyder added [Type] Enhancement Posting/Editing Core Data Issues related to Core Data labels Apr 12, 2021

chipsnyder added 4 commits April 12, 2021 15:45

Generate Core Data Entity subclasses

ab82459

Add Checksum to Block Settings to detect changes easier.

9ee35ec

Refactor Editor Theme Store to be a service that stores to CoreData

72fc6d0

Add tests for Editor Theme Changes

4216817

This was referenced Apr 14, 2021

[RNMobile] Update theme updates to use the approach that guarantees delivery WordPress/gutenberg#30847

Merged

[Task] Update theme updates to use the approach that guarantees delivery wordpress-mobile/gutenberg-mobile#3376

Merged

chipsnyder added 3 commits April 14, 2021 13:20

Update Podfile to reference related branches

bca43a3

Adjust BlockEditorSettingsService to make it more unit testable

e9424ad

Add unit tests for new Settings service

ec65d1b

chipsnyder mentioned this pull request Apr 14, 2021

Adds a wrapper around the dotCom and Dot Org Rest API wordpress-mobile/WordPressKit-iOS#383

Merged

1 task

chipsnyder requested review from antonis, geriux and mkevins April 14, 2021 17:45

chipsnyder commented Apr 14, 2021

View reviewed changes

chipsnyder marked this pull request as ready for review April 14, 2021 17:51

Update pod to align latest commits

1dac1d6

mkevins reviewed Apr 15, 2021

View reviewed changes

chipsnyder added 2 commits April 15, 2021 14:25

Merge branch 'develop' into gutenberg/issue/gss-3163-themeStore

31758e4

Update versions for beta dependencies

f5e1865

antonis approved these changes Apr 16, 2021

View reviewed changes

chipsnyder merged commit 0706c1a into develop Apr 16, 2021

chipsnyder deleted the gutenberg/issue/gss-3163-themeStore branch April 16, 2021 16:55

chipsnyder added this to the 17.2 milestone Apr 16, 2021

chipsnyder mentioned this pull request Apr 16, 2021

NSInvalidArgumentException: *** -[NSRegularExpression enumerateMatchesInString:options:range:usingBlock:]: nil argument #16014

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework EditorThemeStore to be stored in CoreData #16282

Rework EditorThemeStore to be stored in CoreData #16282

chipsnyder commented Apr 12, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Apr 12, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Apr 12, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Apr 12, 2021 •

edited

Loading

chipsnyder Apr 14, 2021

mkevins Apr 15, 2021

chipsnyder Apr 15, 2021

mkevins Apr 16, 2021

chipsnyder Apr 16, 2021

chipsnyder Apr 14, 2021

chipsnyder Apr 14, 2021

mkevins Apr 15, 2021 •

edited

Loading

chipsnyder Apr 15, 2021

chipsnyder commented Apr 16, 2021

antonis left a comment

Rework EditorThemeStore to be stored in CoreData #16282

Rework EditorThemeStore to be stored in CoreData #16282

Conversation

chipsnyder commented Apr 12, 2021 • edited Loading

Description

To test:

Core Migrations

Regression Notes

PR submission checklist:

peril-wordpress-mobile bot commented Apr 12, 2021 • edited Loading

peril-wordpress-mobile bot commented Apr 12, 2021 • edited Loading

peril-wordpress-mobile bot commented Apr 12, 2021 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkevins Apr 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chipsnyder commented Apr 16, 2021

antonis left a comment

Choose a reason for hiding this comment

chipsnyder commented Apr 12, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Apr 12, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Apr 12, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Apr 12, 2021 •

edited

Loading

mkevins Apr 15, 2021 •

edited

Loading