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 convenience methods for assigning lambdas to event listeners #194

Closed
benedictleejh opened this issue Apr 29, 2015 · 6 comments
Closed

Comments

@benedictleejh
Copy link

Currently, ScalaFX event listener assignment methods (e.g. onAction_=) take an EventHandler[_], so when trying to assign lambdas to the event listeners, an implicit conversion is needed (by importing scalafx.Includes._. However, implicit conversions are not considered doing type inference, so doing

onAction = { ae => doSomething() }

would cause a compile error. Instead, we must annotate ae with the type ActionEvent, as such:

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

in order for the implicit conversion to take place and thus for the assignment to be successful.

This is obviously not ideal. Because this is Scala, and we have reasonable local type inference, I believe we should defer to type inference whenever it is reasonable to do so, and in this case, it seems extremely reasonable to allow the compiler to infer the type of ae.

Depending on the number of such event listener assignment methods, doing this could be tedious. If I can get a list of these methods and where to find them, I'd like to help in adding these methods.

@jpsacha
Copy link
Member

jpsacha commented Apr 30, 2015

There is no list of onAction handlers. Possible way is to search for def onAction_=( or similar. When I do something like that I use regular expressions to capture relevant code and either replace or add new methods across files.

Another possibility would be to define a trait that will add the functionality (def onAction, def onAction_=, etc), like it was done with scalafx.delegate.DimensionDelegate to add "height" and "width".

@benedictleejh
Copy link
Author

A quick scan of the JavaFX documentation shows a lot of such methods are located in Node (onMouseDragged, onDragOver, etc.). onAction is in ButtonBase. I'll probably do a grep across the code base to see if I can't find any more.

@jpsacha
Copy link
Member

jpsacha commented Oct 11, 2015

Actually there is a simple solution that may not require any modifications to ScalaFX code. It relies on the new future introduced in Scala 2.11 - support for SAM (Single Abstract Method). This corresponds to Java 8 syntax for using lambdas instead implementing interfaces with simple abstract method, like Runnable. For instance rather than creating anonymous class implementing Runnable, in Java 8, we can pass a lambda:

new Thread(() -> println("hi")).run()

In Scala 2.11 you can do it too when you enable experimental features of the compiler:

scalacOptions += "-Xexperimental"

Here is a complete example using SAM for an event handler, exactly the way you were asking for:

import scalafx.application.JFXApp
import scalafx.application.JFXApp.PrimaryStage
import scalafx.scene.Scene
import scalafx.scene.control.Button
import scalafx.scene.layout.HBox

object SAMDemo extends JFXApp {
  stage = new PrimaryStage {
    title = "SAM Demo"
    scene = new Scene {
      root = new HBox {
        children = Seq(
          new Button {
            text = "Print message (2)"
            onAction = { ae => println("some message") }
          }
        )
      }
    }
  }
}

Note the imports used. There is nothing related to event handling. You can import it but it is not necessary as the handler is created by Scala compiler using SAM feature.

Going forward we should focus on making sure that ScalaFX correctly supports SAM. If you notice any places that this is not working properly, please report.

@benedictleejh
Copy link
Author

I have been using Scala's SAM conversion for a while now, and yes, while it works nice enough in most cases, There are times when SAM conversion doesn't work because of the interaction between that and other features of Scala (e.g. implicits, overloading, etc.).

Case in point: When I upgraded from 8.0.40-R8 to 8.0.60-R9, my function for assigning cell factories broke because of the new assignment method that overloaded the old one. The new one took a Java callback, which because of SAM conversion, the compiler can no longer correctly infer the type of the parameter of the function, and a type must be provided to the function parameter. I.e.,

cellFactory = { tableColumn => ... }

doesn't work because the compiler cannot figure out if you want to use the SAM or the function related method, so you must annotate the type of tableColumn.

I'd argue that Scala idioms should be given priority over Java since ScalaFX is a Scala library, and if interop is needed. have a Scala method that wraps the Java into the appropriate Scala idiom. I'm not entirely sure this is possible without a complete overhaul of the way ScalaFX does things, so I guess at least making sure SAM's don't conflict with overloads would be much appreciated

@jpsacha
Copy link
Member

jpsacha commented Oct 18, 2015

I assume that you are referring to TableColumn.cellFactory. It get new assignment overload to resolve issue #199. For SAM to work better, the cellFactory and other based on JavaFX use of Callback, may need to be changed to the same way event handlers are done (like onAction) - assignment is done from EventHandler and there is an implicit conversion of a lambda to EventHandler. The downside is that without SAM they require specifying type for argument, like (ae:ActionEvent) => .... Your PR #196 makes it possible to remove the type, but does not work well when SAMs are enabled.

Scala 2.12 plans to enable SAMs by default. Whatever fixes are done now should be tested with SAMs too. In particular using SAMs should make things simpler, rather than more verbose. To make it more clear. I rather do not modify much EventHandlers as they currently compile nice with SAMs allowing skipping type, like onAction = { ae => doSomething() }. Implantation of callbacks, like cellFactory may need to be fixed. The tricky issue is to also support default cell factories, like issue #199.

This may need to shelved till SAMs in 2.12 are completed (enabled by default). Currently it is planned for 2.12.0-M4.

@jpsacha
Copy link
Member

jpsacha commented Sep 30, 2018

The SAM expressions are now nicely supported in Scala 2.12 and newer. Statements like:

onAction = { ae => doSomething() }

or

cellFactory = { tableColumn => ... }

work out of the box without changes to ScalaFX.

@jpsacha jpsacha closed this as completed Sep 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants