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

FileChooser does not handle a the value returned when the user cancels file selection #178

Closed
RomanHargrave opened this issue Jan 25, 2015 · 3 comments
Milestone

Comments

@RomanHargrave
Copy link
Contributor

FileChooser.showOpenMultipleDialog is a bridge method of return type Seq[File] that calls javafx.stage.FileChooser, which has a return type of java.util.List[File]. In the event that the user has selected 0 files (or the system toolkit returns null), the method will return null implicitly cast as java.util.List[String]. This will happen during runtime when the user is presented with a multiple file selection dialog and chooses the Cancel option (documentation for com.sun.glass.ui.CommonDialogs.FileChooserResult verifies that an empty list signifies cancellation). When said implicitly cast null value reaches the caller in scalafx, an implicit conversion is made from java.util.List[java.io.File] to scala.collection.Seq[java.io.File]. Because the implicit conversion process involves performing operations on the underlying collection, a NullPointerException is thrown originating in the scala.collection API, which travels back up to the caller of scalafx.stage.FileChooser.showOpenMultipleDialog. Due to the nature of NullPointerException, it makes it hard to programmatically determine whether or not this error was caused by the selection of Cancel. This makes it exceptionally hard to develop a case to handle scenarios where you must perform an action after the potential cancellation has occurred, as the only way to do so is to place the file selection dialog call inside of a try/catch block with a blanket catch for NullPointerException.

In the interest of maintaining method signature, I would suggest one of the following solutions:

  1. Perform the conversion manually, checking for a null value and returning null without proceeding.
  2. Perform the conversion manually, checking for a null value and returning an empty Seq[File]
  3. Allow the implicit conversion, but catch the exception, and return null
  4. Allow the implicit conversion, but catch the exception, and return an empty Seq[File]

Solution one offers what a good solution, as it retains signature, but returns null as opposed to halting the thread. New implementers may use Option.apply(Any) to wrap the return value and deal with the problem in a sane manner.

Solution two offers also offers a good solution, as it retains signature, and returns an empty Seq of the expected type. This means that any functions called against the sequence, most likely iterators, will act upon no file as opposed to resulting in a NullPointerException.

I am aware that this issue is more of a fault of scala's JavaConversions bundle than scalafx, and I plan to report this to the scala bug tracker as well.

This Gist demonstrates some scalafx-agnostic code that will generate a similar exception.

@RomanHargrave
Copy link
Contributor Author

This was found to occur in SFX-2. The 8._ branch has not been tested.

jpsacha added a commit that referenced this issue Jan 26, 2015
…d user cancelled selection an NPE was thrown.

Also clatify in Scaladoc that null may be returned.
@jpsacha
Copy link
Member

jpsacha commented Jan 26, 2015

Fixed using option 1 at best aligns with conventions currently used in ScalaFX (return null is JavaFX documents returning null).

@jpsacha jpsacha closed this as completed Jan 26, 2015
@RomanHargrave
Copy link
Contributor Author

Thanks for the quick fix! For reference purposes, SI-9113, the related language issue, now exists.

@jpsacha jpsacha modified the milestone: 2.2.*-R11 Jan 27, 2015
RomanHargrave added a commit to RomanHargrave/CUEComposer that referenced this issue Jan 28, 2015
adriaanm pushed a commit to adriaanm/scala that referenced this issue Feb 13, 2015
`scala.collection.{JavaConversions, JavaConverters}` no longer wrap
a `null` collection, but return `null` instead. Before, a wrapper would
hold the original `null` collections, delaying the NPE until the first
operation on the collection was called, with no way of knowing whether we
were holding such a time bomb wrapper until the NPE was thrown.

For example, this now holds: `(null : java.util.Iterator[_]).asScala ==
null`. Before, a wrapper would silently be created that would NPE on any
further interaction.

An example of this issue in the wild can be seen at
scalafx/scalafx#178, and an example of code
that generates such a wrapper [before the issue was fixed](https://github.com/scalafx/scalafx/blob/ad60d5faee687b71d3c0474510d
3f3dd081ea351/scalafx/src/main/scala/scalafx/stage/FileChooser.scala#L138).
adriaanm pushed a commit to adriaanm/scala that referenced this issue Feb 13, 2015
`scala.collection.{JavaConversions, JavaConverters}` no longer wrap
a `null` collection, but return `null` instead. Before, a wrapper would
hold the original `null` collections, delaying the NPE until the first
operation on the collection was called, with no way of knowing whether we
were holding such a time bomb wrapper until the NPE was thrown.

For example, this now holds: `(null : java.util.Iterator[_]).asScala ==
null`. Before, a wrapper would silently be created that would NPE on any
further interaction.

An example of this issue in the wild can be seen at
scalafx/scalafx#178, and an example of code
that generates such a wrapper [before the issue was fixed](https://github.com/scalafx/scalafx/blob/ad60d5faee687b71d3c0474510d
3f3dd081ea351/scalafx/src/main/scala/scalafx/stage/FileChooser.scala#L138).
adriaanm pushed a commit to adriaanm/scala that referenced this issue Feb 19, 2015
`scala.collection.{JavaConversions, JavaConverters}` no longer wrap
a `null` collection, but return `null` instead. Before, a wrapper would
hold the original `null` collections, delaying the NPE until the first
operation on the collection was called, with no way of knowing whether we
were holding such a time bomb wrapper until the NPE was thrown.

For example, this now holds: `(null : java.util.Iterator[_]).asScala ==
null`. Before, a wrapper would silently be created that would NPE on any
further interaction.

An example of this issue in the wild can be seen at
scalafx/scalafx#178, and an example of code
that generates such a wrapper [before the issue was fixed](https://github.com/scalafx/scalafx/blob/ad60d5faee687b71d3c0474510d
3f3dd081ea351/scalafx/src/main/scala/scalafx/stage/FileChooser.scala#L138).
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

No branches or pull requests

2 participants