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

Add UIBarButtonItem extension #22

Closed
wants to merge 8 commits into from
Closed

Add UIBarButtonItem extension #22

wants to merge 8 commits into from

Conversation

danielt1263
Copy link
Collaborator

Made a UIBarButtonItem extension with an rx_tap method.

// MVVM
//
// Created by Daniel Tartaglia on 5/25/15.
//
Copy link
Member

Choose a reason for hiding this comment

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

Sry for asking this, but could we just make headers uniform with other headers. In this case it should probably be

//
//  UIBarButtonItem+Rx.swift <-- file name
//  RxCocoa <-- project name
//
//  Created by Daniel Tartaglia on 5/28/15. <-- this + git blame acknowledges and appreciates contribution to the project
//  Copyright (c) 2015 Krunoslav Zaher. All rights reserved. <-- I'm hoping this protects me from being sued by any contributor :), but will need to check to make sure that is the case
//

I'll probably change license later to be BSD, and compatible with other Rx distributions. MIT license this project is under shouldn't limit anyone from using it in any way.

Hope this is not a problem :) Maybe it looks pedantic, but really want to make this project great.

And I definitely need to investigate legal issues better.

@manumax
Copy link

manumax commented May 28, 2015

@kzaher If it could be of any help http://choosealicense.com (feel free to delete this message since it shouldn't be here but I didn't know how to send you the link :))

@danielt1263
Copy link
Collaborator Author

Fixed the rem and also added a new method on UITextField. This observer will fire when the user taps the return key on the keyboard, and provide the complete text the user tapped.

@orta
Copy link
Collaborator

orta commented May 29, 2015

With respect to the license discussion, you do not need it in files, this file - https://github.com/kzaher/RxSwift/blob/master/LICENSE.md covers your entire code base 👍

return observableForControlEvent(.EditingChanged)
}

public func rx_endOnExit() -> Observable<String> {
Copy link
Member

Choose a reason for hiding this comment

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

Would it maybe make more sense to name it rx_editingDidEndOnExit?

When I was reading the name it was kind of hard to understand. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can go either way with it. I named it endOnExit because you used text for EditingChanged so I assumed you were trying to keep function names terse. Just let me know.

Copy link
Member

Choose a reason for hiding this comment

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

Oh :)

The name of the property describes the thing that you are trying to observe. In this case I wanted to observe text, and to do that I needed a way to be notified when text changes. I've implemented it using EditingChanged on osx :) It's just an implementation detail.

When I think about your use case, maybe the correct thing would be just making observableForControlEvent public method and renaming it to rx_textWhen(event: UIControlEvent) -> Observable<String>. rx_endOnExit seems too specific, I don't think we should have a method per event.

You could use that interface like this rx_textWhen(.EditingChanged). Seems readable and simple to me :)

I think that method name should describe thing that you are observing.

When adding methods to extensions (private methods included), one of the reasons why there is rx always in front is to avoid naming collisions in extensions :)

What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, don't forget that I added an rx_ in UIControl to handle all the events. It's just that the UIControl method doesn't return the value contained in the string.

@kzaher
Copy link
Member

kzaher commented May 29, 2015

@manumax thnx for the link. Did check it out. I don't have much experience with open source projects, so would want make sure that everybody can use this code freely, that I can't be sued, and it would be nice if they would mention me somewhere, but if somebody uses this, and doesn't mention this project, or my name, I don't care much, just glad they like it.

It looks like MIT should cover these cases. The only thing I wasn't sure was can some contributor claim that his contribution is under different license, and by that causing that who ever uses this code should pay him, or sue me for copyright infringement.

I really want to prevent that possible nightmare. MIT license does allow sublicensing, so what it's not clear to me is can some court interpret that license in such a way that somebody's contribution is sublicensed and he holds all the rights for that code, and I don't have rights to distribute his contribution.

I am obviously not a native english speaker :), so it's even harder for me to interpret legal documents in another language, and I'm assuming that smart people from MIT thought of that case, but when I can't prove something myself, I feel very insecure :(

I had to deal with some devious clients, so I've learned my lesions :)

@DTartaglia is an awesome guy and has helped me much on this project, and nothing I've written here relates to him.

I just like to solve general problem, so that doesn't drain my time and focus in future.

Thnx @orta for your comment. If the license is good for CocoaPods project with 160 contributors, I'm sure it will be fine for my project.

@danielt1263
Copy link
Collaborator Author

The commit named "Change 0 arg funcs into var getters." is a breaking change. I only went through RxCocoa with these changes (and fixed the sample apps.) There are several places in RxSwift where changing the func to a getter would make the name seem silly, like run, and I didn't want to make changes to actual names without knowing more about the reference Rx project.

@danielt1263
Copy link
Collaborator Author

I also removed rx_endOnExit as requested.

@kzaher
Copy link
Member

kzaher commented May 31, 2015

It was easier for me to cherry pick some of the commits and push it to develop, so I'm going to close this one. Pushed changed to develop.

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.

4 participants