[Bug] Double click in FxFileChooserDialog does not return selected file

ArchibaldBienetre commented 2 years ago

Hi, first of all thank ou for this piece of software, it will be very useful for me :)

However, I think I found a small bug. It occurs for me, running Ubuntu Linux 21.10 Impish (I don't know if that makes a difference anywhere).

How it normally works

When I use FXFileChooserDialog#showOpenDialog this way, it works:

select the file I want
close the dialog with the "OK" button
resullt: the file got chosen, it is returned as an Optional

What I think is broken

When I instead do this I get an empty optional as the result:

select the flie I want
double-click the file
resullt: an empty Optional gets returned

I don't exactly know why this happens - I debugged a bit, and double-click handling in net.raumzeitfalle.fx.filechooser.FileChooserController#handleDoubleClickInFilesList definitely gets triggered. Just javafx.scene.control.Dialog#getResult does not seem to get the result then.

Workaround

I can work around the issue by getting FXFileChooserDialog.model.getSelectedFile() using reflection:

     private Optional<Path> getSelectedFile(FXFileChooserDialog fileChooser) {
        Optional<Path> selectedFile = fileChooser.showOpenDialog(getWindow());
        if (selectedFile.isEmpty()) {
            // workaround for bug https://github.com/Oliver-Loeffler/FXFileChooser/issues/44
            try {
                Field modelField = FXFileChooserDialog.class.getDeclaredField("model");
                modelField.setAccessible(true);
                Object model = modelField.get(fileChooser);
                Method getSelectedFileMethod = model.getClass().getDeclaredMethod("getSelectedFile");
                getSelectedFileMethod.setAccessible(true);
                Path result = (Path) getSelectedFileMethod.invoke(model);
                return Optional.ofNullable(result);
            } catch (NoSuchFieldException | NoSuchMethodException | IllegalAccessException | InvocationTargetException e) {
                throw new RuntimeException(e);
            }
        }
        return selectedFile;
    }

(the model is working, as verified by a test case)

Drawback:
- With workaround in place, a file is selected even if the user clicks "Cancel" :disappointed:
- A breakpoint in net.raumzeitfalle.fx.filechooser.FileChooserController#cancelAction gets no hits, I would have expected that code to prevent this
- it seems usually the result converter defined in net.raumzeitfalle.fx.filechooser.FXFileChooserDialog#FXFileChooserDialog does the work to get things to the result, but it does not get called in case of the double click:
```
  setResultConverter(dialogButton -> {
      if (dialogButton  == okay) {
          this.hide();
          return model.getSelectedFile();
      }
      return null;
  });
```

More context

So far, I'm using FxFileChooserDialog in a playground project only. Find the latest code here as part of this PR: https://github.com/ArchibaldBienetre/javaFxTestGradle/pull/1/files

Oliver-Loeffler commented 2 years ago

Thanks for reporting this! Well, I will try to reproduce the issue and see that there will be a fix.

Oliver-Loeffler commented 2 years ago

Well yes. I can reproduce this. And you are right, the result converter is not triggered on double click. I need to look into the dialog API to understand how to call this. Very good, thanks for pointing this out.

Nevertheless, I was planning to completely remove the Dialog part. Do you actually prefer the Dialog version over the Stage version?

Oliver-Loeffler commented 2 years ago

Please try out the branch issue-44. It is quite a hack as the controller now also has a reference to the dialog. In general, a rethinking of the current structure is needed.
But, if this works for you, I'd release it as 0.0.7 to Maven Central.

I was testing it manually with net.raumzeitfalle.fx.demos.DemoJavaFxDialog.java.

Beside the double click issue, the dark mode in the Dialog is not working properly - need to put some work in there as well.

ArchibaldBienetre commented 2 years ago

Do you actually prefer the Dialog version over the Stage version?

You mean, this? https://github.com/Oliver-Loeffler/FXFileChooser#a-version-with-a-completely-customizable-stage I have not given it a try, yet. I was glad to get something to work with TestFX ':D

I can have a look.

Oliver-Loeffler commented 2 years ago

Yes this one. Was playing with both approaches, using a modal stage vs. using a JavaFX dialog. Nevertheless, this project deserves some more love over next time. It should now work with the modal Stage and the dialog,

TestFX is awesome - but I think it needs more people working on it. It is really underrated and underappreciated.

Btw, which Java version are you using? Would this be still useful to you if the project would move to Java 11?

ArchibaldBienetre commented 2 years ago

... I was testing it manually with net.raumzeitfalle.fx.demos.DemoJavaFxDialog.java.

Beside the double click issue, the dark mode in the Dialog is not working properly - need to put some work in there as well.

I can confirm both

it's working in the demo with the doubleclick
the dark layout demo is broken :(

It was a bit of a struggle to make it work. I installed JDK 8 as that is the target version, and then some dependencies were still missing (Java 8 does not support modules yet, I'd rule out those):

```diff Index: pom.xml IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/pom.xml b/pom.xml --- a/pom.xml (revision 437f9a90fbd3fa8f46898ae0348e9e296baa12c5) +++ b/pom.xml (date 1639946873593) @@ -19,6 +19,7 @@ 1.7.1 5.7.1 4.0.3 + 18-ea+8 4.0.6-alpha 8u76-b04 @@ -241,6 +242,31 @@ + + org.openjfx + javafx-base + ${javaFx.version} + + + org.openjfx + javafx-controls + ${javaFx.version} + + + org.openjfx + javafx-fxml + ${javaFx.version} + + + org.openjfx + javafx-graphics + ${javaFx.version} + + + org.openjfx + javafx-swing + ${javaFx.version} + org.awaitility awaitility ```

Thank you for putting in your time, I'd be superglad for a 0.0.7 alpha that I could use without the workaround :) .

ArchibaldBienetre commented 2 years ago

Do you actually prefer the Dialog version over the Stage version?

You mean, this? https://github.com/Oliver-Loeffler/FXFileChooser#a-version-with-a-completely-customizable-stage I have not given it a try, yet. I was glad to get something to work with TestFX ':D

I can have a look.

Nice, it seems I can just use it as a drop-in replacement, that's fine then.

Oliver-Loeffler commented 2 years ago

Hi, so there is now a new version 0.0.7, just uploaded it. It contains the fix for the broken double click in the Dialog. The fix for the broken dark mode will then become part of 0.0.8 or later. This change comprises the internal API, hence going with 0.0.7 should be okay.

<dependency>
   <groupId>net.raumzeitfalle.fx</groupId>
   <artifactId>filechooser</artifactId>
   <version>0.0.7</version>
</dependency>

or

implementation 'net.raumzeitfalle.fx:filechooser:0.0.7'

Thanks for sharing your setup details - so I will get this project into a little better shape and ready for Java11 soon, there will be also a module-info then but this is not a mandatory requirement. There will be a solution for darkmode in Dialog within a few days,

The stage version It works as a drop-in, looks a little different. Well, there is the directory chooser, which is in a quite early stage. It should then also be suitable for huge directory trees.

For development on this I am currently using the Azul Java 8 JDK, zulu where JavaFX is included. Think Azul also offers Java-11 and Java-17 with FX included. This is quite convenient for development.

Oliver-Loeffler commented 2 years ago

Kudos for your work-around!

Oliver-Loeffler commented 2 years ago

The dark mode inside Dialog is now working again. Also the resize behavior improved for the dialog as the minimum size is now properly constrained. There will be a 0.0.8 release tomorrow bringing the improvements.

Oliver-Loeffler commented 2 years ago

Dark mode and resizing are now properly working with version 0.0.8. Closed with 5e27cb456567fc196b0731d2ec7aa60df32ddf6d and bebd9dd795e25a44d8c44a902fc617fe4e7c3d45.

New version is available at Maven Central.

<dependency>
   <groupId>net.raumzeitfalle.fx</groupId>
   <artifactId>filechooser</artifactId>
   <version>0.0.8</version>
</dependency>

Oliver-Loeffler commented 2 years ago

@ArchibaldBienetre The new version 0.0.8 is now available. Its been tested on MacOS and Windows 10, works for both. Would be glad if you could share if it eventually works as intended on Ubuntu!

ArchibaldBienetre commented 2 years ago

Thanks for checking in!

I've been using the 0.0.8 since this afternoon, the update introduced no serious problems (just the default skin, I had to change that to MODENA now), my integration tests (testfx) still work as before. :heavy_check_mark:

Manual test works, dark layout too.

My project uses FXFileChooserStage now, but I'll take the Dialog implementation for another spin :mag:

EDIT: Dialog implementation is still (i.e., like the 0.0.7) working like a charm :heavy_check_mark:

Oliver-Loeffler / FXFileChooser