Reader: Add Discover Channels #23676
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| import Foundation | ||
| import CoreData | ||
| import Combine | ||
|
|
||
| public final class ManagedObjectsObserver<T: NSManagedObject>: NSObject, NSFetchedResultsControllerDelegate { | ||
| @Published private(set) public var objects: [T] = [] | ||
|
|
||
| private let controller: NSFetchedResultsController<T> | ||
|
|
||
| public convenience init( | ||
| predicate: NSPredicate, | ||
| sortDescriptors: [SortDescriptor<T>], | ||
| context: NSManagedObjectContext | ||
| ) { | ||
| let request = NSFetchRequest<T>(entityName: T.entity().name ?? "") | ||
| request.predicate = predicate | ||
| request.sortDescriptors = sortDescriptors.map(NSSortDescriptor.init) | ||
| self.init(request: request, context: context) | ||
| } | ||
|
|
||
| public init( | ||
| request: NSFetchRequest<T>, | ||
| context: NSManagedObjectContext, | ||
| cacheName: String? = nil | ||
| ) { | ||
| self.controller = NSFetchedResultsController(fetchRequest: request, managedObjectContext: context, sectionNameKeyPath: nil, cacheName: cacheName) | ||
| super.init() | ||
|
|
||
| try? controller.performFetch() | ||
| objects = controller.fetchedObjects ?? [] | ||
|
|
||
| controller.delegate = self | ||
| } | ||
|
|
||
| public func controllerDidChangeContent(_ controller: NSFetchedResultsController<NSFetchRequestResult>) { | ||
| objects = self.controller.fetchedObjects ?? [] | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| import UIKit | ||
|
|
||
| extension UITextView { | ||
| /// Creates a text view that behaves like a non-editable multiline label | ||
| /// but supports interaction and other text view features. | ||
| public static func makeLabel() -> UITextView { | ||
| let textView = UITextView() | ||
| textView.isScrollEnabled = false | ||
| textView.isEditable = false | ||
| textView.textContainerInset = .zero | ||
| textView.textContainer.lineFragmentPadding = 0 | ||
| return textView | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,116 @@ | ||
| import UIKit | ||
| import SwiftUI | ||
|
|
||
| extension UIView { | ||
| /// Pins edges of the view to the edges of the given target view or layout | ||
| /// guide. By default, pins to the superview. | ||
| /// | ||
| /// The view also gets enabled for Auto Layout by setting | ||
| /// `translatesAutoresizingMaskIntoConstraints` to `false`. | ||
| /// | ||
| /// Example uage: | ||
| /// | ||
| /// ```swift | ||
| /// subview.pinEdges() // to superview | ||
| /// subview.pinEdges(to: superview.safeAreaLayoutGuide) | ||
| /// ``` | ||
| @discardableResult | ||
| public func pinEdges( | ||
| _ edges: Edge.Set = .all, | ||
| to target: AutoLayoutItem? = nil, | ||
| insets: UIEdgeInsets = .zero, | ||
| relation: AutoLayoutPinEdgesRelation = .equal, | ||
| priority: UILayoutPriority? = nil | ||
| ) -> [NSLayoutConstraint] { | ||
| guard let target = target ?? superview else { | ||
| assertionFailure("view has to be installed in the view hierarchy") | ||
| return [] | ||
| } | ||
| translatesAutoresizingMaskIntoConstraints = false | ||
|
Contributor
There was a problem hiding this comment. Add an assertion ( // OK to set `subview.translatesAutoresizingMaskIntoConstraints = false`
// because subview's frame is set by auto-layout.
subview.pinEdges(to: superview)
// Not okay to set `superview.translatesAutoresizingMaskIntoConstraints = false`
// because we don't know if superview's frame is set by auto-layout or not.
superview.pinEdges(to: subview)
Contributor
Author
There was a problem hiding this comment.
It's not a strict requirement. You can pin a view to a layout guide or to a sibling view. It should be pretty clear from the method's name what it does. SnapKit, PureLayout, and a couple of other frameworks all pretty much work the same way, so it shouldn't surprise that that's how it works.
Contributor
There was a problem hiding this comment. If there is no hierarchical requirement in these two views, we should remove this line, as I mentioned in the example code above: // Not okay to set `superview.translatesAutoresizingMaskIntoConstraints = false`
// because we don't know if superview's frame is set by auto-layout or not.
superview.pinEdges(to: subview)
Contributor
There was a problem hiding this comment. IIRC UIViewController.view is not framed using auto layout. Maybe you can reproduce the issue I mentioned by using
Contributor
Author
There was a problem hiding this comment. Yup, you shouldn't pin the edges of the superview to its child, and I added a bit of docs explaining it. It even sounds kind of backwards, doesn't it? The default signature As a user, you want to avoid writing
Contributor
There was a problem hiding this comment.
That's true. I get it's a bit annoying that we are forced to write this one long line for pretty much every single view in the app. However, the framework (or convenient method in this case) should only set the property when it's appropriate to do so. As I mentioned above, it's not appropriate to call Here is a diff to reproduce the issue: diff --git a/WordPress/Classes/ViewRelated/Reader/ReaderDiscoverViewController.swift b/WordPress/Classes/ViewRelated/Reader/ReaderDiscoverViewController.swift
index af9eda30d5..ffa871d863 100644
--- a/WordPress/Classes/ViewRelated/Reader/ReaderDiscoverViewController.swift
+++ b/WordPress/Classes/ViewRelated/Reader/ReaderDiscoverViewController.swift
@@ -101,6 +101,11 @@ class ReaderDiscoverViewController: UIViewController, ReaderDiscoverHeaderViewDe
view.addSubview(streamVC.view)
streamVC.view.pinEdges()
streamVC.didMove(toParent: self)
+
+ let testView = UIView()
+ testView.backgroundColor = .red
+ view.insertSubview(testView, at: 0)
+ view.pinEdges(to: testView) // Reader goes blank.
}
/// TODO: (tech-debt) the app currently stores the responses from the `/discover`
Contributor
Author
There was a problem hiding this comment.
I added a runtime check to catch this early: #if DEBUG
if let target = target as? UIView {
assert(!target.isDescendant(of: self), "The target view can't be a descendant for the view")
}
#endif |
||
|
|
||
| #if DEBUG | ||
| if let target = target as? UIView { | ||
| assert(!target.isDescendant(of: self), "The target view can't be a descendant for the view") | ||
| } | ||
| #endif | ||
|
|
||
| var constraints: [NSLayoutConstraint] = [] | ||
|
|
||
| func pin(_ edge: Edge.Set, _ closure: @autoclosure () -> NSLayoutConstraint) { | ||
| guard edges.contains(edge) else { return } | ||
| constraints.append(closure()) | ||
| } | ||
|
|
||
| switch relation { | ||
| case .equal: | ||
| pin(.top, topAnchor.constraint(equalTo: target.topAnchor, constant: insets.top)) | ||
| pin(.trailing, trailingAnchor.constraint(equalTo: target.trailingAnchor, constant: -insets.right)) | ||
| pin(.bottom, bottomAnchor.constraint(equalTo: target.bottomAnchor, constant: -insets.bottom)) | ||
| pin(.leading, leadingAnchor.constraint(equalTo: target.leadingAnchor, constant: insets.left)) | ||
| case .lessThanOrEqual: | ||
| pin(.top, topAnchor.constraint(greaterThanOrEqualTo: target.topAnchor, constant: insets.top)) | ||
| pin(.trailing, trailingAnchor.constraint(lessThanOrEqualTo: target.trailingAnchor, constant: -insets.right)) | ||
| pin(.bottom, bottomAnchor.constraint(lessThanOrEqualTo: target.bottomAnchor, constant: -insets.bottom)) | ||
| pin(.leading, leadingAnchor.constraint(greaterThanOrEqualTo: target.leadingAnchor, constant: insets.left)) | ||
| } | ||
|
|
||
| if let priority { | ||
| for constraint in constraints { | ||
| constraint.priority = priority | ||
| } | ||
| } | ||
|
|
||
| NSLayoutConstraint.activate(constraints) | ||
| return constraints | ||
| } | ||
|
|
||
| /// Pins the view to the center of the given container. By default, | ||
| /// pins to the superview. | ||
| @discardableResult | ||
| public func pinCenter( | ||
| to target: AutoLayoutItem? = nil, | ||
| offset: UIOffset = .zero, | ||
| priority: UILayoutPriority? = nil | ||
| ) -> [NSLayoutConstraint] { | ||
| guard let target = target ?? superview else { | ||
| assertionFailure("view has to be installed in the view hierarchy") | ||
| return [] | ||
| } | ||
| translatesAutoresizingMaskIntoConstraints = false | ||
|
|
||
| let constraints = [ | ||
| centerXAnchor.constraint(equalTo: target.centerXAnchor, constant: offset.horizontal), | ||
| centerYAnchor.constraint(equalTo: target.centerYAnchor, constant: offset.vertical), | ||
| ] | ||
|
|
||
| if let priority { | ||
| for constraint in constraints { | ||
| constraint.priority = priority | ||
| } | ||
| } | ||
|
|
||
| NSLayoutConstraint.activate(constraints) | ||
| return constraints | ||
| } | ||
| } | ||
|
|
||
| public protocol AutoLayoutItem { | ||
| var leadingAnchor: NSLayoutXAxisAnchor { get } | ||
| var trailingAnchor: NSLayoutXAxisAnchor { get } | ||
| var leftAnchor: NSLayoutXAxisAnchor { get } | ||
| var rightAnchor: NSLayoutXAxisAnchor { get } | ||
| var topAnchor: NSLayoutYAxisAnchor { get } | ||
| var bottomAnchor: NSLayoutYAxisAnchor { get } | ||
| var widthAnchor: NSLayoutDimension { get } | ||
| var heightAnchor: NSLayoutDimension { get } | ||
| var centerXAnchor: NSLayoutXAxisAnchor { get } | ||
| var centerYAnchor: NSLayoutYAxisAnchor { get } | ||
| } | ||
|
|
||
| public enum AutoLayoutPinEdgesRelation { | ||
| case equal | ||
| case lessThanOrEqual | ||
| } | ||
|
|
||
| extension UIView: AutoLayoutItem {} | ||
| extension UILayoutGuide: AutoLayoutItem {} | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: update the signature to be
pin(edges: .all, to: view, ...)instead ofpinEdges(.all, to: view, ...)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It won't be clear what it does with a default argument
view.pin(to: view). It has to have the default argument because you'll use.all99% of the time.