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

Exposed NavigationDrawerController methods for objc #1248

Merged
merged 2 commits into from
Jun 24, 2019
Merged

Exposed NavigationDrawerController methods for objc #1248

merged 2 commits into from
Jun 24, 2019

Conversation

OrkhanAlikhanov
Copy link
Contributor

Adding @objcs for #1247

Copy link
Member

@daniel-jonathan daniel-jonathan left a comment

Choose a reason for hiding this comment

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

Awesome!

@emauro
Copy link

emauro commented Jun 10, 2019

Great! But is necessary to add to some other methods:

  • init(rootViewController...

  • isOpened

  • isLeftViewOpened

  • isRightViewOpened

@daniel-jonathan
Copy link
Member

@OrkhanAlikhanov I will wait to merge this in until all the necessary function declarations have been updated.

@OrkhanAlikhanov
Copy link
Contributor Author

Hey @emauro! I added @objcMembers to whole class instead of marking individual members objc.

@objcMembers

Apply this attribute to a class declaration, to implicitly apply the objc attribute to all Objective-C compatible members of the class, its extensions, its subclasses, and all of the extensions of its subclasses.

Most code should use the objc attribute instead, to expose only the declarations that are needed. If you need to expose many declarations, you can group them in an extension that has the objc attribute. The objcMembers attribute is a convenience for libraries that make heavy use of the introspection facilities of the Objective-C runtime. Applying the objc attribute when it isn’t needed can increase your binary size and adversely affect performance.

@daniel-jonathan
Copy link
Member

daniel-jonathan commented Jun 13, 2019

@OrkhanAlikhanov Is this something we want to do to other classes across the board? As well, remove any @objc declarations where it is redundant now?

Copy link
Member

@daniel-jonathan daniel-jonathan left a comment

Choose a reason for hiding this comment

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

Nice cleanup version of the same affect.

@@ -132,6 +132,7 @@ public protocol NavigationDrawerControllerDelegate {
}

@objc(NavigationDrawerController)
@objcMembers
Copy link
Member

Choose a reason for hiding this comment

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

Hey! Can we add this feature to more classes and cleanup the single @objc declarations? As well to the other frameworks?

Copy link
Contributor Author

@OrkhanAlikhanov OrkhanAlikhanov Jun 13, 2019

Choose a reason for hiding this comment

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

If @emauro confirms that it works properly, we surely can!

Copy link
Member

Choose a reason for hiding this comment

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

Cool, so let's wait for that. Thank you!

Copy link

Choose a reason for hiding this comment

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

For me it is ok. @objc permits to fine grain the access to class methods but I think here it is not an issue.

Copy link
Member

Choose a reason for hiding this comment

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

@emauro so you tested it and it works?

Copy link

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Great, then we will add this where needed and make a new release. Thank you!

@daniel-jonathan daniel-jonathan merged commit 730e96b into CosmicMind:development Jun 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants