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

Plugin Support for MessageInputBar #2

Merged
merged 2 commits into from
Jun 15, 2018
Merged

Conversation

nathantannar4
Copy link
Member

Forked the plugins I made in InputBarAccessoryView and make them subspecs for MessageInputBar

@nathantannar4 nathantannar4 self-assigned this Jun 4, 2018
@nathantannar4 nathantannar4 requested review from SD10 and zhongwuzw June 4, 2018 02:55
@nathantannar4 nathantannar4 added the enhancement New feature or request label Jun 4, 2018
@nathantannar4
Copy link
Member Author

Its a lot of lines but mostly documentation and views. Core logic in AttachmentManager and AutocompleteManager. AutocompleteManager was vetted by MessageViewController (Githawk)

@SD10
Copy link
Member

SD10 commented Jun 4, 2018

@nathantannar4 This looks good to me, a few questions though:

@SD10
Copy link
Member

SD10 commented Jun 4, 2018

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

@nathantannar4
Copy link
Member Author

This is pretty much the last of the changes. The merged PRs in your referenced issues are in MessageInputBar.

Sent with GitHawk

@nathantannar4
Copy link
Member Author

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

@SD10
Copy link
Member

SD10 commented Jun 4, 2018

@nathantannar4 Good point on the naming conflicts 🤔 This is something I’ll think about this week

Sent with GitHawk

@SD10
Copy link
Member

SD10 commented Jun 4, 2018

@nathantannar4 So this looks pretty safe to support MessageInputBar/Core in MessageKit for version 2.0 🤔 There are some API breaking changes so I don't think I can get it in the first version.

@nathantannar4
Copy link
Member Author

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 {
Copy link
Member

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 {
Copy link
Member

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 {
Copy link
Member

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 {
Copy link
Member

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 {

Copy link
Member

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

Copy link
Member Author

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 {
Copy link
Member

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

Copy link
Member Author

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 {
Copy link
Member

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

Copy link
Member Author

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?

Copy link
Member

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

@SD10 SD10 requested a review from cwalo June 13, 2018 22:26
@SD10 SD10 removed the enhancement New feature or request label Jun 15, 2018
@SD10
Copy link
Member

SD10 commented Jun 15, 2018

@nathantannar4 Please make these changes when you can so we can do
MessageKit/MessageKit#730

@nathantannar4 nathantannar4 merged commit 9224f13 into development Jun 15, 2018
@nathantannar4 nathantannar4 deleted the feature/plugins branch June 15, 2018 23:09
nathantannar4 added a commit that referenced this pull request Jun 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants