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 facilities for creating Observables from JavaFX events and ObservableValues #1182

Merged
merged 1 commit into from
May 9, 2014

Conversation

amazari
Copy link
Contributor

@amazari amazari commented May 9, 2014

Heavily inspired by the swing contrib.

Provides:

  • a Scheduler implementation ensuring execution in the JavaFX UI thread
  • a Subscription implementation ensuring that un-subscription happens in JavaFX UI thread
  • an Observable factory method for observing UI events on JavaFX nodes
  • an Observable factory method for observing value changes of a JavaFX ObservableValue

Also the rxjava-javafx is added to the gradle build.

…ableValues.

Provides:
 * a Scheduler implementation ensuring execution in the JavaFX UI thread
 * a Subscription implementation ensuring that un-subscription happens in JavaFX UI thread
 * an Observable factory method for observing UI events on JavaFX nodes
 * an Observable factory method for observing value changes of a JavaFX ObservableValue

Also the rxjava-javafx is added to the gradle build.
@amazari
Copy link
Contributor Author

amazari commented May 9, 2014

The build is failing on the bot. Looking at the output, it looks like the deployed jdk do not provide the javafx bits.
Every Oracle JDK >= 7u15 bundles JavaFX, so I guess we are in one of those sitiations:

  • the bot's jdk is outdated
  • the bot's jdk do no contains JavaFX, as it is the case with openjdk
  • somehow the javafx jars are not exported in the build.

Should the module gradle definition explicitly state dependency on JDK >= 7u15 ?

@cloudbees-pull-request-builder

RxJava-pull-requests #1093 FAILURE
Looks like there's a problem with this pull request

@amazari
Copy link
Contributor Author

amazari commented May 9, 2014

Answering to headinthebox remark on the previous PR:
"For subscribing on the UI thread there is subscribeon, so you should ideally not need a special subscription for that."

Do subscribeOn guarantees that the un subscription callback runs using the same scheduler as the OnSubscribe(Func) ?

@benjchristensen
Copy link
Member

Do subscribeOn guarantees that the un subscription callback runs using the same scheduler as the OnSubscribe(Func)

Based on previous discussions it has been considered that there may still be cases where unsubscribe could arrive from other threads.

See these discussions:

If you need unsubscribe to happen on a particular thread, you can use unsubscribeOn

I'm trying to remember why subscribeOn can't do it ... re-reading those previous posts to try and remember!

@benjchristensen
Copy link
Member

Should the module gradle definition explicitly state dependency on JDK >= 7u15

The build is using Java 7. If the version of Java 7 is old that will require more effort to resolve as we'll have to get CloudBees involved.

@amazari
Copy link
Contributor Author

amazari commented May 9, 2014

Hi @benjchristensen,
Thanks for your quick feedback!

If you need unsubscribe to happen on a particular thread, you can use unsubscribeOn

Do you think this responsibility should be left to the user code or enforced withing the Observable creation method?

@benjchristensen
Copy link
Member

In the swing module it is handled by the 'SwingObservable' implementation to ensure thread-safety and doesn't use 'unsubscribeOn'.


source.addEventHandler(eventType, handler);

subscriber.add(JavaFxSubscriptions.unsubscribeInEventDispatchThread(new Action0() {
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be safely handling unsubscribe by doing it in the right thread so 'unsubscribeOn' is not needed.

benjchristensen added a commit that referenced this pull request May 9, 2014
@benjchristensen benjchristensen merged commit 6c336e0 into ReactiveX:master May 9, 2014
@benjchristensen benjchristensen mentioned this pull request Jun 1, 2014
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.

3 participants