[ASTraitCollection] Add missing properties to ASTraitCollection #625

ypogribnyi · 2017-10-20T19:22:44Z

In comparison to UITraitCollection, some properties were missing from ASTraitCollection.
These are needed for ASDisplayNodes to conveniently access information about current interface direction, DynamicType size category etc.

* ASTraitCollection now completely reflects UITraitCollection * Add ASContentSizeCategory enum that corresponds to UIContentSizeCategory and can be used inside a struct.

CLAassistant · 2017-10-20T19:22:51Z

All committers have signed the CLA.

ghost · 2017-10-20T19:56:18Z

🚫 CI failed with log

maicki

Thanks for the PR, really appreciated!

In general looks good to me, would like to have another team member to take a look over it though.

maicki · 2017-10-31T18:39:05Z

Source/Details/ASContentSizeCategory.m

+
+// UIContentSizeCategoryUnspecified is available only in iOS 10.0 and later.
+// This constant is used as a fallback for older iOS versions.
+UIContentSizeCategory const AS_UIContentSizeCategoryUnspecified = @"_UICTContentSizeCategoryUnspecified";


Can we make this static?

maicki · 2017-10-31T18:40:21Z

Source/Details/ASContentSizeCategory.m

+      @{
+        // We will fallback to ASContentSizeCategoryUnspecified so there is no need to include it in this dictionary.
+
+        [NSNumber numberWithInteger:ASContentSizeCategoryExtraSmall]: UIContentSizeCategoryExtraSmall,


Can we use object literals in this case? @(ASContentSizeCategoryExtraSmall)?

Adlai-Holler

Sorry for the long delay getting a review on this @ypogribnyi and thank you for contributing!

Adlai-Holler · 2017-10-31T18:44:02Z

Source/Details/ASContentSizeCategory.h

+/**
+ * ASContentSizeCategory is a UIContentSizeCategory that can be used in a struct.
+ */
+typedef NS_ENUM(NSInteger, ASContentSizeCategory) {


Let's do something clever, and use __unsafe_unretained UIContentSizeCategory as a struct member:

typedef struct { // unretained is required because ARC can't manage struct memory __unsafe_unretained UIContentSizeCategory category; BOOL flag; } ASTestStruct;

* Use __unsafe_unretained UIContentSizeCategory instead of the enum.

ypogribnyi · 2017-11-01T13:03:48Z

Updated ASPrimitiveTraitCollection to use an unretained pointer to UIContentSizeCategory. Thanks for the suggestion.

nguyenhuy

Some small changes and we're good to go!

nguyenhuy · 2018-01-16T08:47:32Z

Source/Details/ASTraitCollection.h

-                                   horizontalSizeClass:(UIUserInterfaceSizeClass)horizontalSizeClass
-                                     verticalSizeClass:(UIUserInterfaceSizeClass)verticalSizeClass
-                                  forceTouchCapability:(UIForceTouchCapability)forceTouchCapability
-                                         containerSize:(CGSize)windowSize;


This can be considered a breaking API change. Instead, I think we should preserve this API and call the full version internally with default values. We can also deprecate it, but that's as far as we can go for now, IMHO.

nguyenhuy · 2018-01-16T08:53:38Z

Source/Details/ASTraitCollection.h

+                                           userInterfaceIdiom:(UIUserInterfaceIdiom)userInterfaceIdiom
+                                         forceTouchCapability:(UIForceTouchCapability)forceTouchCapability
+                                              layoutDirection:(UITraitEnvironmentLayoutDirection)layoutDirection
+#if TARGET_OS_TV


I understand your reasoning here. However, for maintainability, I think it's best to declare a separate API for tvOS with this extra param.

nguyenhuy · 2018-01-16T08:55:58Z

Source/Details/ASTraitCollection.m

+  if (AS_AT_LEAST_IOS10) {
+    return UIContentSizeCategoryUnspecified;
+  }
+  else {


Nit: Merge this line to the previous one.

nguyenhuy · 2018-01-16T08:58:45Z

Source/Details/ASTraitCollection.m

+    return UIContentSizeCategoryAccessibilityExtraExtraExtraLarge;
+  }
+
+  if (AS_AT_LEAST_IOS10) {


This fallback logic is used multiple times in this diff. I think we can encapsulate it into a function and use everywhere.

nguyenhuy · 2018-01-16T09:13:21Z

Source/Details/ASTraitCollection.m

+
+    // preferredContentSizeCategory always points to one of UIContentSizeCategory constants.
+    // Assuming their values do not duplicate, we can simply compare pointers, avoiding string comparison.
+    lhs.preferredContentSizeCategory == rhs.preferredContentSizeCategory &&


Thank you for the detailed comment. I actually expect -[NSString isEqualToString:] to compare memory addresses early on. Without concrete numbers, I'd consider this optimization unnecessary, also because calling -isEqualToString: gives us better maintainability.

Interesting find: https://gist.github.com/0xced/2275014#file-nsstring-m-L76

… ASTraitCollection-missing-properties # Conflicts: # CHANGELOG.md # Source/Details/ASTraitCollection.m

* Restore one of the ASTraitCollection constructors with a deprecation notice. * Clean up API by the separation of tvOS-specific interfaces. * Use [NSString -isEqualToString:] for ASPrimitiveContentSizeCategory equality tests for better readability. * Encapsulate fallback logic for UIContentSizeCategoryUnspecified.

nguyenhuy

Thank you! 🎉

nguyenhuy · 2018-01-16T14:19:42Z

Source/Details/ASTraitCollection.m

+      lhs.userInterfaceStyle == rhs.userInterfaceStyle &&
+    #endif
+
+    [leftSizeCategory isEqualToString:rightSizeCategory] && // Simple pointer comparison should be sufficient here


…ureGroup#625) * [ASTraitCollection] Add missing properties to ASTraitCollection * ASTraitCollection now completely reflects UITraitCollection * Add ASContentSizeCategory enum that corresponds to UIContentSizeCategory and can be used inside a struct. * * Remove enum ASContentSizeCategory. * Use __unsafe_unretained UIContentSizeCategory instead of the enum. * Added ASPrimitiveTraitCollection lifetime test * Changes requested at code review: * Restore one of the ASTraitCollection constructors with a deprecation notice. * Clean up API by the separation of tvOS-specific interfaces. * Use [NSString -isEqualToString:] for ASPrimitiveContentSizeCategory equality tests for better readability. * Encapsulate fallback logic for UIContentSizeCategoryUnspecified. * Fix failing test

[ASTraitCollection] Add missing properties to ASTraitCollection

70fbac1

* ASTraitCollection now completely reflects UITraitCollection * Add ASContentSizeCategory enum that corresponds to UIContentSizeCategory and can be used inside a struct.

maicki reviewed Oct 31, 2017

View reviewed changes

Adlai-Holler reviewed Oct 31, 2017

View reviewed changes

ypogribnyi added 3 commits November 1, 2017 12:04

Merge branch 'master' into ASTraitCollection-missing-properties

5afe689

* Remove enum ASContentSizeCategory.

e0c395e

* Use __unsafe_unretained UIContentSizeCategory instead of the enum.

Added ASPrimitiveTraitCollection lifetime test

185a139

nguyenhuy requested changes Jan 16, 2018

View reviewed changes

ypogribnyi added 3 commits January 16, 2018 14:29

Merge branch 'master' of https://github.com/TextureGroup/Texture into…

10f3d8f

… ASTraitCollection-missing-properties # Conflicts: # CHANGELOG.md # Source/Details/ASTraitCollection.m

Fix failing test

11dc888

nguyenhuy approved these changes Jan 16, 2018

View reviewed changes

nguyenhuy merged commit a3136b0 into TextureGroup:master Jan 16, 2018

ypogribnyi deleted the ASTraitCollection-missing-properties branch January 16, 2018 18:21

ay8s mentioned this pull request Jan 18, 2018

ASTraitCollection crash Inserting nil into dictionary #754

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ASTraitCollection] Add missing properties to ASTraitCollection #625

[ASTraitCollection] Add missing properties to ASTraitCollection #625

ypogribnyi commented Oct 20, 2017

CLAassistant commented Oct 20, 2017 •

edited

Loading

ghost commented Oct 20, 2017

maicki left a comment

maicki Oct 31, 2017

maicki Oct 31, 2017

Adlai-Holler left a comment

Adlai-Holler Oct 31, 2017

ypogribnyi commented Nov 1, 2017

nguyenhuy left a comment

nguyenhuy Jan 16, 2018

nguyenhuy Jan 16, 2018

nguyenhuy Jan 16, 2018

nguyenhuy Jan 16, 2018

nguyenhuy Jan 16, 2018

nguyenhuy left a comment

nguyenhuy Jan 16, 2018

[ASTraitCollection] Add missing properties to ASTraitCollection #625

[ASTraitCollection] Add missing properties to ASTraitCollection #625

Conversation

ypogribnyi commented Oct 20, 2017

CLAassistant commented Oct 20, 2017 • edited Loading

ghost commented Oct 20, 2017

maicki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adlai-Holler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ypogribnyi commented Nov 1, 2017

nguyenhuy left a 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

nguyenhuy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented Oct 20, 2017 •

edited

Loading