Skip to content
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

Rework EditorThemeStore to be stored in CoreData #16282

Merged
merged 11 commits into from
Apr 16, 2021
Merged
26 changes: 26 additions & 0 deletions MIGRATIONS.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,32 @@
This file documents changes in the data model. Please explain any changes to the
data model as well as any custom migrations.

## WordPress 120

@chipsnyder 2021-04-12

- Created a new entity `BlockEditorSettings` with:
- `isFSETheme` (required, default `false`, `Boolean`) FSE = "Full Site Editing"
- `lastUpdated` (required, no default, `Date`)

- Created a new entity `BlockEditorSettingElement` with:
- `type` (required, no default, `String`)
- `value` (required, no default, `String`)
- `slug` (required, no default, `String`)
- `name` ( required, no default, `String`)

- Created one-to-many relationship between `BlockEditorSettings` and `BlockEditorSettingElement`
- `BlockEditorSettings`
- `elements` (optional, to-many, cascade on delete)
- `BlockEditorSettingElement`
- `settings` (required, to-one, nullify on delete)

- Created one-to-one relationship between `Blog` and `BlockEditorSettings`
- `BlockEditorSettings`
- `blockEditorSettings` (optional, to-one, cascade on delete)
- `BlockEditorSettings`
- `blog` (required, to-one, nullify on delete)

## WordPress 119

@mkevins 2021-03-31
Expand Down
6 changes: 3 additions & 3 deletions Podfile
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,9 @@ def wordpress_ui
end

def wordpress_kit
pod 'WordPressKit', '~> 4.30.0'
# pod 'WordPressKit', '~> 4.30.0'
# pod 'WordPressKit', :git => 'https://github.com/wordpress-mobile/WordPressKit-iOS.git', :tag => ''
# pod 'WordPressKit', :git => 'https://github.com/wordpress-mobile/WordPressKit-iOS.git', :branch => ''
pod 'WordPressKit', :git => 'https://github.com/wordpress-mobile/WordPressKit-iOS.git', :branch => 'task/addWrapperForRestAPIs'
# pod 'WordPressKit', :git => 'https://github.com/wordpress-mobile/WordPressKit-iOS.git', :commit => ''
# pod 'WordPressKit', :path => '../WordPressKit-iOS'
end
Expand Down Expand Up @@ -161,7 +161,7 @@ abstract_target 'Apps' do
## Gutenberg (React Native)
## =====================
##
gutenberg :tag => 'v1.50.0'
gutenberg :commit => '619aa0a7a2e776d1ffa89a676956948b87bb59f6'


## Third party libraries
Expand Down
179 changes: 92 additions & 87 deletions Podfile.lock

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import Foundation
import CoreData

@objc(BlockEditorSettingElement)
public class BlockEditorSettingElement: NSManagedObject {

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
import Foundation
import CoreData

enum BlockEditorSettingElementTypes: String {
case color
case gradient

var valueKey: String {
self.rawValue
}
}

extension BlockEditorSettingElement {

@nonobjc public class func fetchRequest() -> NSFetchRequest<BlockEditorSettingElement> {
return NSFetchRequest<BlockEditorSettingElement>(entityName: "BlockEditorSettingElement")
}

/// Stores the associated type that this object represents.
/// Available types are defined in `BlockEditorSettingElementTypes`
///
@NSManaged public var type: String

/// Stores the value for the associated type. The associated field in the API response might differ based on the type.
///
@NSManaged public var value: String

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

@mkevins mkevins Apr 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/// Stores a user friendly display name for the `slug`.
///
@NSManaged public var name: String

/// Stores a reference back to the parent `BlockEditorSettings`.
///
@NSManaged public var settings: BlockEditorSettings
}

extension BlockEditorSettingElement: Identifiable {
var rawRepresentation: [String: String]? {
guard let type = BlockEditorSettingElementTypes(rawValue: self.type) else { return nil }
return [
#keyPath(BlockEditorSettingElement.slug): self.slug,
#keyPath(BlockEditorSettingElement.name): self.name,
type.valueKey: self.value
]
}

convenience init(fromRawRepresentation rawObject: [String: String], type: BlockEditorSettingElementTypes, context: NSManagedObjectContext) {
self.init(context: context)

self.type = type.rawValue
self.value = rawObject[type.valueKey] ?? ""
self.slug = rawObject[#keyPath(BlockEditorSettingElement.slug)] ?? ""
self.name = rawObject[ #keyPath(BlockEditorSettingElement.name)] ?? ""
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import Foundation
import CoreData

@objc(BlockEditorSettings)
public class BlockEditorSettings: NSManagedObject {

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import Foundation
import CoreData

extension BlockEditorSettings {

@nonobjc public class func fetchRequest() -> NSFetchRequest<BlockEditorSettings> {
return NSFetchRequest<BlockEditorSettings>(entityName: "BlockEditorSettings")
}

/// 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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📓 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)


/// Stores a Bool indicating if the theme supports Full Site Editing (FSE) or not. `true` means the theme is an FSE theme.
/// Default is `false`
///
@NSManaged public var isFSETheme: Bool

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/// Stores a set of attributes describing values that are represented with arrays in the API request.
/// Available types are defined in `BlockEditorSettingElementTypes`
///
@NSManaged public var elements: Set<BlockEditorSettingElement>?

/// Stores a reference back to the parent blog.
///
@NSManaged public var blog: Blog
}

// MARK: Generated accessors for elements
extension BlockEditorSettings {

@objc(addElementsObject:)
@NSManaged public func addToElements(_ value: BlockEditorSettingElement)

@objc(removeElementsObject:)
@NSManaged public func removeFromElements(_ value: BlockEditorSettingElement)

@objc(addElements:)
@NSManaged public func addToElements(_ values: Set<BlockEditorSettingElement>)

@objc(removeElements:)
@NSManaged public func removeFromElements(_ values: Set<BlockEditorSettingElement>)
}

extension BlockEditorSettings: Identifiable {

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import Foundation
import Gutenberg

extension BlockEditorSettings: GutenbergEditorTheme {
public var colors: [[String: String]]? {
elementsByType(.color)
}

public var gradients: [[String: String]]? {
elementsByType(.gradient)
}

private func elementsByType(_ type: BlockEditorSettingElementTypes) -> [[String: String]]? {
return elements?.compactMap({ (element) -> [String: String]? in
guard element.type == type.rawValue else { return nil }
return element.rawRepresentation
})
}
}

extension BlockEditorSettings {
convenience init?(editorTheme: EditorTheme, context: NSManagedObjectContext) {
self.init(context: context)
self.lastUpdated = Date()
self.checksum = editorTheme.checksum

var parsedElements = Set<BlockEditorSettingElement>()
if let themeSupport = editorTheme.themeSupport {
themeSupport.colors?.forEach({ (color) in
parsedElements.insert(BlockEditorSettingElement(fromRawRepresentation: color, type: .color, context: context))
})

themeSupport.gradients?.forEach({ (gradient) in
parsedElements.insert(BlockEditorSettingElement(fromRawRepresentation: gradient, type: .gradient, context: context))
})
}

self.elements = parsedElements
}
}
10 changes: 10 additions & 0 deletions WordPress/Classes/Models/Blog+BlockEditorSettings.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import Foundation
import CoreData

extension Blog {

/// Stores the relationship to the `BlockEditorSettings` which is an optional entity that holds settings realated to the BlockEditor. These are features
/// such as Global Styles and Full Site Editing settings and capabilities.
///
@NSManaged public var blockEditorSettings: BlockEditorSettings?
}
31 changes: 18 additions & 13 deletions WordPress/Classes/Models/EditorTheme.swift
Original file line number Diff line number Diff line change
@@ -1,30 +1,35 @@
import Foundation
import Gutenberg

struct EditorTheme: Codable, Equatable {
struct EditorTheme: Codable {
static func == (lhs: EditorTheme, rhs: EditorTheme) -> Bool {
return lhs.description == rhs.description
return lhs.checksum == rhs.checksum
}

enum CodingKeys: String, CodingKey {
case themeSupport = "theme_supports"
case version
case stylesheet
}

let themeSupport: EditorThemeSupport?
let version: String?
let stylesheet: String?

var description: String {
return "\(stylesheet ?? "")-\(version ?? "")"
}
let checksum: String

init(from decoder: Decoder) throws {
let map = try decoder.container(keyedBy: CodingKeys.self)
self.themeSupport = try? map.decode(EditorThemeSupport.self, forKey: .themeSupport)
self.version = try? map.decode(String.self, forKey: .version)
self.stylesheet = try? map.decode(String.self, forKey: .stylesheet)
let parsedTheme = try? map.decode(EditorThemeSupport.self, forKey: .themeSupport)
self.themeSupport = parsedTheme
self.checksum = {
guard let parsedTheme = parsedTheme else { return "" }
let encoder = JSONEncoder()
encoder.outputFormatting = .sortedKeys
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

let result: String
do {
let data = try encoder.encode(parsedTheme)
result = String(data: data, encoding: .utf8) ?? ""
} catch {
result = ""
}
return result.md5()
}()
}
}

Expand Down
Loading