-
Notifications
You must be signed in to change notification settings - Fork 44
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
Plugin Support for MessageInputBar #2
Conversation
Refinements and Consistency Changes
Its a lot of lines but mostly documentation and views. Core logic in AttachmentManager and AutocompleteManager. AutocompleteManager was vetted by MessageViewController (Githawk) |
@nathantannar4 This looks good to me, a few questions though:
|
We should also add to the README that this project is not being used in MessageKit yet, it would be great if we could replace it in the next ~1.5 months though. This Summer for sure. Unit tests would give me more confidence of course. This is a 2.0 feature |
This is pretty much the last of the changes. The merged PRs in your referenced issues are in MessageInputBar. Sent with GitHawk |
We should definitely have MessageKit using this repo for the v1.0 otherwise there could be name conflicts for people trying to use both Sent with GitHawk |
@nathantannar4 Good point on the naming conflicts 🤔 This is something I’ll think about this week Sent with GitHawk |
@nathantannar4 So this looks pretty safe to support |
Any API breaking changes I can revert Sent with GitHawk |
import UIKit | ||
|
||
/// `InputPlugin` is a protocol that makes integrating plugins to the `MessageInputBar` easy. | ||
public protocol InputPlugin: class { |
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.
Let's use AnyObject
here
|
||
} | ||
|
||
extension NSAttributedString { |
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.
Can we explicitly mark all helper extensions in the library as internal
?
import UIKit | ||
|
||
/// AutocompleteManagerDelegate is a protocol that more precisely define AutocompleteManager logic | ||
public protocol AutocompleteManagerDelegate: class { |
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.
AnyObject
import UIKit | ||
|
||
/// AutocompleteManagerDataSource is a protocol that passes data to the AutocompleteManager | ||
public protocol AutocompleteManagerDataSource: class { |
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.
AnyObject
} | ||
|
||
extension AutocompleteManager: UITableViewDelegate, UITableViewDataSource { | ||
|
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 may be a better approach to stop using this pattern of confirming to a protocol via extension. This doesn't allow users to override any of the methods declared here
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.
Yup I agree
// range.length > 0: Backspace/removing text | ||
// range.lowerBound < textView.selectedRange.lowerBound: Ignore trying to delete | ||
// the substring if the user is already doing so | ||
if range.length > 0, range.lowerBound < textView.selectedRange.lowerBound { |
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.
I think I fixed some bugs in GitHawk related to this logic 🤔 but I'm fine with leaving it for now. we will need unit tests one day
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.
This is copied from the githawk fixes
import Foundation | ||
|
||
/// AttachmentManagerDataSource is a protocol to passes data to the AttachmentManager | ||
public protocol AttachmentManagerDataSource: class { |
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.
AnyObject, anywhere to constrain a protocol really
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.
What's the benefit just curious?
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.
@nathantannar4 no clue, it's what the Swift Programming Language Guide book shows when constraining a protocol to be class
only, so this is what I've started using
@nathantannar4 Please make these changes when you can so we can do |
Forked the plugins I made in InputBarAccessoryView and make them subspecs for MessageInputBar