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

Move some content group and styles to FormattableContentKit #24265

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

mokagio
Copy link
Contributor

@mokagio mokagio commented Mar 20, 2025

These are needed by the WordPressData files, see #24242.

These are needed by the WordPressData files.
@dangermattic
Copy link
Collaborator

dangermattic commented Mar 20, 2025

2 Warnings
⚠️ Modules/Package.swift was changed without updating its corresponding Package.resolved. Please resolve the Swift packages as appropriate to your project setup (e.g. in Xcode or by running swift package resolve).
⚠️ This PR is assigned to the milestone 25.9. The due date for this milestone has already passed.
Please assign it to a milestone with a later deadline or check whether the release for this milestone has already been finished.

Generated by 🚫 Danger

Comment on lines +79 to +82
"WordPressUI",
.product(name: "Gridicons", package: "Gridicons-iOS"),
// TODO: Remove — It's here just for a NSMutableParagraphStyle init helper
.product(name: "WordPressKit", package: "WordPressKit-iOS"),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mokagio mokagio requested review from kean and crazytonyli March 20, 2025 09:43
@mokagio mokagio added this to the 25.9 milestone Mar 20, 2025
@wpmobilebot
Copy link
Contributor

Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr24265-8c75698
Version25.8
Bundle IDcom.jetpack.alpha
Commit8c75698
App Center Buildjetpack-installable-builds #10782
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot
Copy link
Contributor

WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr24265-8c75698
Version25.8
Bundle IDorg.wordpress.alpha
Commit8c75698
App Center BuildWPiOS - One-Offs #11756
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@kean
Copy link
Contributor

kean commented Mar 20, 2025

There are a few types like Notification that are dependent on on FormattableContentKit but mainly as a convenient way to cache some data:

/// Subject Blocks Transient Storage.
///
fileprivate var cachedSubjectContentGroup: FormattableContentGroup?

Would it be possible to extract these extensions and move them to the app target? They could use objc_assocciatedObject for caching.

Files like these do not belong to FormattableContentKit:

Modules/Sources/FormattableContentKit/Styles/WPStyleGuide+Notifications.swift

@mokagio
Copy link
Contributor Author

mokagio commented Mar 21, 2025

Thanks @kean.

In my haste of getting WordPressData to build and run, I might be spending too little time evaluating what should be moved and what could be reworked.

I'll have a look here after closing #24267

@mokagio
Copy link
Contributor Author

mokagio commented Mar 21, 2025

So I've been looking into this a bit and it's not straightforward. The main blocker is that Notification (an NSManagedObject which we need to move to WordPressData) uses the logic directly related to FormattableContentKit in its deinit.

I'm going to play around with this more on my Monday.

@kean
Copy link
Contributor

kean commented Mar 21, 2025

the logic directly related to FormattableContentKit in its deinit.

I see. So it observes some of its own properties to reload the cached attributes. These properties can be observed from the outside – similarly to how we use ImageLoadingController with its lifetime tied to the associated image view.

// Name arbitrary 
final class NotificationFormattableContentController {
    weak var notification: Notification?

    private var cachedSubjectContentGroup: FormattableContentGroup?
    // ... 
 
    deinit {
        // Remove KVO observation
    }

    init(notification: Notification) {
        // 1. Set up obsever
        // 2. Set as associated object 
    }
}

ideally, I would just add NotificationViewModel, put the "formattable" code there, and use it in the UI. I'm not sure if it's going to be easy though. I'd just go with what's faster for now.

@kean kean self-requested a review March 21, 2025 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants