Feature/direct lambda assignment for event handlers

[benedictleejh]

This fixes #194. Also, along the way, I've correct some typos related to the lambda assignment.

This, however, seems to break anything using the handle function. handle seems to be returning a lot of EventHandler[Nothing] which type errors when applied against anything that expects an EventHandler[(not Nothing)] or a lambda.

Example:

scalafx-demos/src/main/scala/scalafx/VanishingCircles.scala:64: overloaded method value onMouseClicked_= with alternatives:
[error]   [T >: javafx.scene.input.MouseEvent <: javafx.event.Event](handler: T => Unit)Unit <and>
[error]   (v: javafx.event.EventHandler[_ >: javafx.scene.input.MouseEvent])Unit
[error]  cannot be applied to (javafx.event.EventHandler[Nothing])
[error]         onMouseClicked = handle {
[error]         ^

[benedictleejh]

After thinking about it for a bit, I think it would be better if the function types for the assignment method took in ScalaFX event types instead of JavaFX event types. It would allow for more natural access patterns of the event's methods when writing code.

On that note, are there implicit conversions from JavaFX types to ScalaFX types? If not, doing the conversion on a per-method basis might get a little hairy.

[benedictleejh]

Well, I found the answer to my question: yes, there are implicit conversions from JavaFX types to ScalaFX types, at least for the events (which is what really matters here).

The only real issue is that some method signatures become a fair bit more complicated. To handle the type bounds specified in JavaFX, I currently do this:

onDragDone_=[T >: jfxsi.DragEvent <: jfxe.Event](handler: T => Unit)

To use ScalaFX types instead, the signature turns into:

onDragDone_=[T >: DragEvent <: Event, U >: jfxsi.DragEvent <: jfxe.Event](handler: T => Unit)(implicit jfx2sfx: U => T)

Most of it is irrelevant if you just let the type inferencer do its work, but it might get a little complicated to read.

It'll probably take about a day or two to do all the conversion.

[jpsacha]

I am not able to build the code. Getting a couple of compilation errors, for instance:

[error] ...IncompleteClickHandler.scala:124: overloaded method value onAction_= with alternatives:
[error]   (handler: scalafx.event.ActionEvent => Unit)Unit <and>
[error]   (implicit aeh: javafx.event.EventHandler[javafx.event.ActionEvent])Unit
[error]  cannot be applied to (javafx.event.EventHandler[Nothing])
[error]               onAction = handle {
[error]               ^

Please make sure that the code compiles and all the test pass before submitting a PR. The Travis CI setup was fixed, so that should be able to run tests too (you will need to mere it from master.

I looked only briefly though the code and had some concerns. For instance:

• the lambdas you introduced return Unit, existing event handlers can return any type. Requiring Unit as return value may add extra verbosity in certain situations, for instance, when last instruction is call toa function that returns a value that is ignored.
• Overloaded x_=(...) methods can lead to compilation errors. They need to have tests excercising them.
• There seems to be incompatible changes, breaking existing code. Some care will need to be taken to first deprecate and then add new ways rather than just forcing users to rewrite their code.

I could not test/review in details since the code was not building.

[benedictleejh]

Regarding your point that existing event handlers can return any type. The function signature of the JavaFX event handler method handle is void handle(T event) (from https://docs.oracle.com/javase/8/javafx/api/javafx/event/EventHandler.html), which is a Unit return type. Hence regardless of what lambda is passed into the handle, the return type is always ignored. Making the lambda return Unit is simply matching the return type of the original handle method.

Also, I'm not sure what you mean by the Unit return value adding extra verbosity; any function can return Unit. When Unit is the return type, the return value of the last instruction is simply ignored. There is no need to specifically return Unit.

With regards to incompatible changes, the handle method is broken. There is simply no way around this because it never returns the correct type. This could mean that this pull request is pushed to whatever version a breaking change is acceptable.

[jpsacha]

Is commit 7728617 supposed to be part of this PR? It seems to require that lambdas have they argument types provided. Can you clarify what benefits for ScalaFX this PR brings in its latest form?

[benedictleejh]

7728617 is part of the PR, as it fixes tests that don't compile. The reason those lambdas have type annotations is due to the implicits from scalafx.Includes. Because I've left the assignment methods for normal EventHandlers in (for backward source compatibility, though I'd much prefer to have them gone), there now exist 2 possibilities when trying to assign lambdas: an implicit conversion from a lambda to an EventHandler, and a direct assignment method. These are mutually exclusive operations, and in order to allow the compiler to disambiguate them, those lambdas must be annotated.

The main benefit to ScalaFX is being able to use ScalaFX in a more Scala-like manner without resorting to implicits. As much as I love implicits, having too many implicits in a project increases the likelihood of implicit collision (with other implicits or with something non-implicit), and debugging errors caused by implicits isn't easy, because the error messages aren't always helpful, and it can be difficult to find the source of the error.

[jpsacha]

In you very first comment for this PR you state "This fixes #194.". After commit commit 7728617 this is no longer the case. You no longer can write:

onAction = { ae => doSomething() }

You need to write as before the PR, providing argument type:

onAction = { ae: ActionEvent => doSomething() }

I did implement alternative workaround for KeyFrame factory methods that is using PR without commit 7728617. It is available here [https://github.com/jpsacha/scalafx/tree/feature/PR196_Event_Lambdas]. IT let you use notation like:

onAction = { ae => doSomething() }

Though that key problem is that while it works well with current compilers, it does not work when SAMs are enabled ("-Xexperimental"). You have to again specify argument types.

I will try to extract correction to types tat are part of the PR like correction from

def onMouseDragEntered_=(v: jfxe.EventHandler[_ >: jfxsi.MouseEvent]) {...}

to

def onMouseDragEntered_=(v: jfxe.EventHandler[_ >: jfxsi.MouseDragEvent]) {...}

but overloaded assignment operators that add explicit lambdas should not be merged as they do not improve notation as requested by #194 and do not work as well with SAMs as the current code.

By the way, a version of ProScalaFX examples modified to use simpler event handler using SAMs is here [https://github.com/scalafx/ProScalaFX/tree/SAM]. It used regular ScalaFX code from the master branch.

[jpsacha]

This no longer needed in Scala 2.12 and newer. Statements like

cellFactory = { tableColumn => ... }

are handled out of the box.