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

A few basic Swing observables #262

Merged
merged 13 commits into from
May 7, 2013
Merged

Conversation

jmhofer
Copy link
Contributor

@jmhofer jmhofer commented May 7, 2013

This adds a few observables for observing common button click, keyboard and mouse events.

@benjchristensen
Copy link
Member

I haven't played with Swing in a very long time but this looks great!

* See the License for the specific language governing permissions and
* limitations under the License.
*/
package rx;
Copy link
Member

Choose a reason for hiding this comment

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

I'm considering the placement in the rx. package.

Does it make more sense in rx.observables or rx.swing?

Thus far we've kept rx to be only the most foundational classes so this seems out of place in that package (and I realize it's coming from a contrib module which is what makes me further wonder if it should be completely namespaced into rx.swing).

@cloudbees-pull-request-builder

RxJava-pull-requests #133 SUCCESS
This pull request looks good

@jmhofer
Copy link
Contributor Author

jmhofer commented May 7, 2013

I don't know.

I intuitively placed it directly under rx to avoid the rx.swing.SwingObservable naming redundancy because it's already called SwingObservable.

I would also be okay with putting everything under rx.swing.

@jmhofer
Copy link
Contributor Author

jmhofer commented May 7, 2013

This would mean moving the SwingScheduler, too, though, to something like rx.swing.concurrency. Do we want a parallel package structure for each contributed library?

@benjchristensen
Copy link
Member

A namespaced package makes sense for things that don't fit in the other packages (as you've done), but I agree with your assessment of rx.swing.SwingObservable and rx.swing.concurrency.SwingScheduler being awkward and redundant.

Perhaps SwingObservable moves into rx.observables.SwingObservable so that it matches the convention of all Observables other than the main one being inside rx.observables?

@jmhofer
Copy link
Contributor Author

jmhofer commented May 7, 2013

Placing it inside rx.observables makes sense to me, too. I guess it's probably better than putting it directly under rx according to the principle of least surprise. I'll move it there.

@benjchristensen
Copy link
Member

Okay ... sorry about being pedantic on this but whatever we do here sets precedent for all modules we add in the future.

Even this decision leaves open the possibility of naming collisions between modules but I'm fine with dealing with that as this approach leaves the usage more natural.

The only other thing I've considered is whether it should be SwingObservables with an s at the end similar to Schedulers and Subscriptions since this is just a factory class. It doesn't actually extend from Observable so is not an Observable like ConnectedObservable or GroupedObservable.

@jmhofer
Copy link
Contributor Author

jmhofer commented May 7, 2013

No problem. I agree that these decisions are quite important. I don't mind at all changing this around in order to make the API better.

Concerning SwingObservable vs SwingObservables: I called it like that so that the methods are more DSL-like (for example SwingObservable.fromButtonAction).

I'm okay with naming it SwingObservables, too, but how to call the methods then? Maybe observeButtonActions? Or buttonActionObservable (assuming the user will probably want to statically import SwingObservables.*)?

@benjchristensen
Copy link
Member

Interesting consideration, Subscriptions and Schedulers don't do that great a job in this regard.

I like the 'observe' verb prefix though it implies it immediately subscribes which it does not, it returns a lazy Observable you then observe by subscribing to it.

I think 'from', 'toObservable', 'create' are the right type of prefixes.

Still thinking ...

@benjchristensen
Copy link
Member

Another consideration, nothing prevents extending from Observable in the future to add custom operators specific to Swing.

Keep it as SwingObservable with 'from' factory methods I think.

@jmhofer
Copy link
Contributor Author

jmhofer commented May 7, 2013

Alright, good.

benjchristensen added a commit that referenced this pull request May 7, 2013
A few basic Swing observables
@benjchristensen benjchristensen merged commit 7ffc515 into ReactiveX:master May 7, 2013
@jmhofer jmhofer deleted the swing branch May 7, 2013 15:39
rickbw pushed a commit to rickbw/RxJava that referenced this pull request Jan 9, 2014
A few basic Swing observables
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.

None yet

3 participants