Adding an Objective-C compatible API to LayoutKit #180

staguer · 2018-02-05T22:46:13Z

This is related to issue #178 regarding adding support for Objective-C consumers.
This objc-api branch is now ready for review.

Two of the open questions are:

How do we make these long initializers more convenient for use from Objective-C? I have several options in mind.
Should we make these new classes unavailable from Swift somehow? My understanding is that there's a way to use pods to exclude some directories at import time?

The builders for (1) can be added as a separate follow-up change.
Currently working on figuring out what we can do for (2).

nicksnyder · 2018-02-06T15:03:19Z

My guess is that the best thing to do will be to make initializers take a single object. That object will have instance properties for the properties that you would normally pass in. This will save you from making a ridiculous number of initializer variants.
e.g.
```
LOKLinearLayoutOptions *linearLayoutOptions = [LOKLinearLayoutOptions new];
linearLayoutOptions.arrangement = /* */
linearLayoutOptions.size = CGSizeMake(10, 10);
LOKLinearLayout *linearLayout = [LOKLinearLayout newWithOptions:options];
```
I don't remember if this is possible or how to do it. If it is possible, it seems like it would be nice to do to avoid confusion in Swift.

…ayout to its own file.

…hecking.

staguer · 2018-02-12T19:27:47Z

This PR should be ready for review now. Two things are still missing from it: documentation comments and some support for splitting out the ObjC support code. I'll be working on the first one and investigating the second one.

staguer · 2018-02-12T20:29:01Z

Regarding question (2) of how to avoid the overhead of this new code for Swift consumers, @chenxiao0228 's suggestion is to make a separate GitHub repo and Cocoapod named something like LayoutKit-objc. These wrapper classes and sample app in this PR would not go into this LayoutKit repo but into the new LayoutKit-objc repo/pod and the new pod would import the current LayoutKit pod. Swift consumers will continue to use this repo/pod without incurring the weight of this new code.

jingwei-huang1 · 2018-02-12T05:26:30Z

LayoutKitObjCSampleApp/AppDelegate.m

+- (BOOL)application:(UIApplication *)application didFinishLaunchingWithOptions:(NSDictionary *)launchOptions {
+    // Override point for customization after application launch.
+    self.window = [[UIWindow alloc] initWithFrame:UIScreen.mainScreen.bounds];
+    ViewController* viewController = [[ViewController alloc] init];


Based on our style guideline, we should have a space between ViewController and *, no space between * and ViewController. Please also apply the rest of them.

In this case, the style should like ViewController *viewController = [[ViewController alloc] init];

3.4 Pointer spacing
Pointers should have a space between the type and the *. There should be no space between variable names and the *.

Thanks! I'll fix that.

jingwei-huang1 · 2018-02-12T05:29:24Z

LayoutKitObjCSampleApp/RotationLayout.h

+#import <LayoutKit/LayoutKit-Swift.h>
+
+@interface RotationLayout: NSObject <LOKLayout>
+-(nonnull instancetype)initWithSublayout:(nonnull id<LOKLayout>)sublayout alignment:(nullable LOKAlignment *)alignment viewReuseId:(nullable NSString *)viewReuseId;


Could you reorder the methods under the property declarations?

Thanks! I'll fix that.

jingwei-huang1 · 2018-02-12T05:30:28Z

LayoutKitObjCSampleApp/RotationLayout.h

+
+@interface RotationLayout: NSObject <LOKLayout>
+-(nonnull instancetype)initWithSublayout:(nonnull id<LOKLayout>)sublayout alignment:(nullable LOKAlignment *)alignment viewReuseId:(nullable NSString *)viewReuseId;
+@property (readonly, nonatomic, nonnull) id<LOKLayout> sublayout;


Please also reorder attributes based on 3.5.1.1 Attribute ordering. Also the others.

Thanks! I'll fix that.

jingwei-huang1 · 2018-02-12T05:32:33Z

LayoutKitObjCSampleApp/RotationLayout.m

+@interface RotationView: UIView
+@end
+@implementation RotationView
+-(instancetype)init {


-> 3.9 Method declarations and definitions

One space should be used between the - or + and the return type. Also the others.

Thanks! I'll fix that.

jingwei-huang1 · 2018-02-12T05:35:28Z

LayoutKitObjCSampleApp/RotationLayout.m

+}
+
+- (void)configureWithBaseTypeView:(nonnull UIView *)baseTypeView {
+


Why is this method empty?

This implements the method @objc func configure(baseTypeView: View) on the LOKLayout protocol.

Do you think if we should move this method to optional?

jingwei-huang1 · 2018-02-12T23:16:38Z

LayoutKitObjCSampleApp/ViewController.m

+//    [self.view addSubview:tableView];
+//    self.adapter = [[LOKReloadableViewLayoutAdapter alloc] initWithTableView:tableView];
+//    tableView.delegate = self.adapter;
+//    tableView.dataSource = self.adapter;


Why do you comment out those code?

Oops, I was using it to verify that the wrapper worked for UITableView, I'll remove it.

jingwei-huang1 · 2018-02-12T23:17:57Z

LayoutKitObjCSampleApp/ViewController.m

+//    tableView.delegate = self.adapter;
+//    tableView.dataSource = self.adapter;
+
+    LOKSizeLayout *cellLayout = [[LOKSizeLayout alloc] initWithMinWidth:self.view.bounds.size.width


Have you considered creating a few convenience initializers?

There are too many parameters with default properties, so making convenience initializers even for the common combinations would be too many. A builder pattern will be more appropriate and that's coming in a separate change.

jingwei-huang1 · 2018-02-12T23:21:55Z

Sources/Layouts/BaseLayout.swift

@@ -30,10 +30,21 @@ open class BaseLayout<V: View> {
        return config != nil
    }

+    private let viewClass: View.Type


The reason for this is objective-c doesn't support V?

Objective C has some support for generics but it's mostly for defining neat interfaces for Swift, and some mild type checking on Obj C side, not nearly as full featured as Swift generics.

jingwei-huang1 · 2018-02-12T23:25:00Z

Sources/ObjCSupport/LOKBaseLayout.swift

+public class LOKBaseLayout: NSObject, LOKLayout {
+    let layout: Layout
+
+    init(layout: Layout) {


We keep all init internally for wrapper use?

jingwei-huang1 · 2018-02-12T23:28:53Z

Sources/ObjCSupport/LOKButtonLayout.swift

+
+import UIKit
+
+@objc public class LOKButtonLayout: LOKBaseLayout {


I am wondering why we can't directly create objective-c classes for wrappers? With objective-c wrappers, it might be much faster with generating swift interfaces. Also, we can separate them to different pod, so the LayoutKit is purely swift LayoutKit without objc support.

Swift was chosen for these wrappers since this codebase already uses Swift and for simplicity I'd like to keep it single-language. I doubt there would much difference in speed, either at editing, build, or run time.

Regarding Pods, I'm planning to exclude these wrappers from the current Pod and include them in a new Pod.

jingwei-huang1 · 2018-02-12T23:38:35Z

Additional to @chenxiao0228 comment, we can still keep them in the same repo, but 2 x pods and that would be easier to manage.

jingwei-huang1 · 2018-02-12T23:39:33Z

Also, we need to setup the test for wrappers because the current CI doesn't compile objective-c wrappers

$ pod lib lint -> LayoutKit (6.0.1) LayoutKit passed validation. -> LayoutKitObjC (6.0.1) LayoutKitObjC passed validation.

jingwei-huang1 · 2018-02-13T22:28:55Z

Sources/ObjCSupport/ReverseWrappedLayout.swift

+
+import CoreGraphics
+
+class ReverseWrappedLayout: Layout {


It seems ReverseWrappedLayout.swift and WrappedLayout isn't being used for the public. Could you create a Internal folder for those two classes?

jingwei-huang1 · 2018-02-13T23:10:24Z

LayoutKitObjCSampleApp/RotationLayout.m

+}
+
+- (void)configureWithBaseTypeView:(nonnull UIView *)baseTypeView {
+


Do you think if we should move this method to optional?

jingwei-huang1 · 2018-02-13T23:11:57Z

Sources/ObjCSupport/LOKButtonLayout.swift

+                      image: UIImage?,
+                      imageSize: CGSize,
+                      font: UIFont? = nil,
+                      contentEdgeInsets: NSValue?, // UIEdgeInsets


Instead of NSValue, we might consider wrapping it a class. To compare with NSValue, it would be easier to understand and configure.

Sergei Taguer added 12 commits January 27, 2018 13:09

Starting Objective-C sample app.

0913e65

Starting Layout/Measurement/Arrangement wrappers

41386cb

LabelLayout and InsetLayout

4ea1c02

Fleshing out label and inset layouts a bit.

6872aa3

Sample code illustrating usage of custom view classes.

43bf916

Fixing nested wrap/unwrap classes.

e3a9769

Demonstrating ObjC custom layout.

8626593

Adding LOKSizeLayout

ca11db5

Starting LOKStackLayout

e5c2d5a

Fleshing out LOKStackLayout

d58bb7f

LOKAxis becomes an enum

b54c3d6

LOKStackLayoutDistribution becomes an enum

7a89619

staguer requested review from nicksnyder and jingwei-huang1 February 5, 2018 22:46

Sergei Taguer added 6 commits February 8, 2018 20:22

Adding LOKReloadableViewLayoutAdapter and moving the sample RotationL…

8d5ad9e

…ayout to its own file.

Making some parameters optional, since ObjC doesn’t have strong nil c…

482493c

…hecking.

Adding LOKButtonLayout

0a1fb6e

Adding LOKOverlayLayout

3567629

Making OverlayLayout helper initializer internal

124f221

Adding LOKTextViewLayout

40a64dc

jingwei-huang1 suggested changes Feb 12, 2018

View reviewed changes

Sergei Taguer added 3 commits February 13, 2018 14:12

Removing @objc visibility for one of the enums.

a0cd150

Styling fixes as per review comments.

ff5383d

Adding LayoutKitObjC.podspec

bd95914

$ pod lib lint -> LayoutKit (6.0.1) LayoutKit passed validation. -> LayoutKitObjC (6.0.1) LayoutKitObjC passed validation.

jingwei-huang1 reviewed Feb 13, 2018

View reviewed changes

jingwei-huang1 approved these changes Feb 13, 2018

View reviewed changes

Sergei Taguer added 5 commits February 13, 2018 16:20

Moving two files to Internal folder.

3b37f5e

Merge branch 'master' into objc-api

a107226

Adding support for new lineHeight parameter

f95d17a

Making the LOKLayout.configureView protocol method optional.

a82cbab

Updating the podspec after the creation of a nested directory.

c08e051

staguer merged commit 0113630 into master Feb 15, 2018

staguer deleted the objc-api branch February 27, 2018 01:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding an Objective-C compatible API to LayoutKit #180

Adding an Objective-C compatible API to LayoutKit #180

staguer commented Feb 5, 2018 •

edited

nicksnyder commented Feb 6, 2018

staguer commented Feb 12, 2018

staguer commented Feb 12, 2018

jingwei-huang1 Feb 12, 2018

staguer Feb 13, 2018

jingwei-huang1 Feb 12, 2018

staguer Feb 13, 2018

jingwei-huang1 Feb 12, 2018

staguer Feb 13, 2018

jingwei-huang1 Feb 12, 2018

staguer Feb 13, 2018

jingwei-huang1 Feb 12, 2018

staguer Feb 13, 2018

jingwei-huang1 Feb 13, 2018

jingwei-huang1 Feb 12, 2018

staguer Feb 13, 2018

jingwei-huang1 Feb 12, 2018

staguer Feb 13, 2018

jingwei-huang1 Feb 12, 2018

staguer Feb 13, 2018

jingwei-huang1 Feb 12, 2018

staguer Feb 13, 2018

jingwei-huang1 Feb 12, 2018

staguer Feb 13, 2018

jingwei-huang1 commented Feb 12, 2018

jingwei-huang1 commented Feb 12, 2018

jingwei-huang1 Feb 13, 2018

jingwei-huang1 Feb 13, 2018

jingwei-huang1 Feb 13, 2018

		}

		- (void)configureWithBaseTypeView:(nonnull UIView *)baseTypeView {


		import UIKit

		@objc public class LOKButtonLayout: LOKBaseLayout {

Adding an Objective-C compatible API to LayoutKit #180

Adding an Objective-C compatible API to LayoutKit #180

Conversation

staguer commented Feb 5, 2018 • edited

nicksnyder commented Feb 6, 2018

staguer commented Feb 12, 2018

staguer commented Feb 12, 2018

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

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

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

jingwei-huang1 commented Feb 12, 2018

jingwei-huang1 commented Feb 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staguer commented Feb 5, 2018 •

edited