0

I'm creating a simple control to browse for and sample Audio files. I want to use an ObjectProperty<File> so that I can bind some properties of the button responsible for playing the file:

PlayButton.disableProperty.bind(this.BGMFile.isNull());
PlayButton.textProperty.bind(this.BGMFile.asString());

So then there will be three things that I will need to override, two of which I have successfully done, and so will not get into

The third is the asString method:

new SimpleObjectProperty<File>(this, "BGM File", null){
    /*yadda yadda overrides*/
    @Override public StringBinding asString(){
        if (super.get() != null && super.get().exists())
            return (StringBinding) Bindings.format(
                super.get().getName(), this
            );
        else return (StringBinding) Bindings.format("[NONE]", this);
    }
}

This feels correct to me, and I even ripped the code from grepCode here, but when I browse for a file using the FileChooser I have setup and select the file I want to use, and then set it to the SimpleProperty the button text stays as [NONE].

This is the code for browsing for the file:

this.btnBrowseBGM.setOnAction((ActionEvent E) -> {
    FileChooser FC = new FileChooser();
    FC.getExtensionFilters().add(Filters.AudioExtensions());
    FC.setTitle("Browse for Background Audio File");
    File F = FC.showOpenDialog(this.getScene().getWindow());
    if (F != null && F.exists()) try {
        this.BGMFile.set(Files.copy(
            F.toPath(),
            Paths.get("Settings/Sound/", F.getName()),
            StandardCopyOption.REPLACE_EXISTING
        ).toFile());
    } catch(IOException ex) {
        Methods.Exception(
            "Unable to copy file to Settings Sound Directory.",
            "Failed to copy Sound File", ex);
        this.BGMFile.set(F);
    } else this.BGMFile.set(null);
    E.consume();
});

Because the path doesn't exist it yells at me (which I expected) but it still should be setting the BGMFile property to F. I know it does because the toggle button becomes active and pressing it plays the sound file.

So what am I missing/doing wrong here?

EDIT:

I think I may have an idea: One of the methods which I override is the set method:

@Override public void set(File newValue){
    if (newValue != null && newValue.exists())
        super.set(newValue);
    else super.set(null);
}

Could it be that overriding the set method causes it to not trigger the overridden asString method?

Will
  • 3,413
  • 7
  • 50
  • 107

1 Answers1

1

The problem is that you create a new Binding every time the asString() method is called. Since you first call it when the file is null, you get the binding created by Bindings.format("[NONE]", this). So your binding on the button is equivalent to:

playButton.textProperty().bind(Bindings.format("[NONE]", bgmFile));

So even though the string value gets reevaluated when the file property changes, it still formats "[NONE]".

You can see the opposite problem if you do

ObjectProperty<File> fileProperty = new SimpleObjectProperty<File>() { 
    /* your previous implementation */
};
fileProperty.set(new File("/path/to/some/valid/file"));
// now bind when you get the filename:
playButton.textProperty().bind(fileProperty.asString());
// setting the fileProperty to null will now invoke the binding that was provided when it wasn't null
// and you'll see a nice bunch of null pointer exceptions:
fileProperty.set(null);

Another way to say this is that the logic that checks if there's a valid file whose name you want doesn't exist in the binding, it is in the asString() method. That method is not invoked just because the property changes.

So you need to create a single StringBinding that processes all the logic (check if the file is null, if not check if it exists, if so get the name, etc) when it's get() method is called. You can do this either by subclassing StringBinding and putting the logic in the computeValue() method, or by using the utility Bindings.createStringBinding(...) method as in the following:

new SimpleObjectProperty<File>(this, "BGM File", null){

    final StringBinding string = Bindings.createStringBinding(() -> {
        File file = this.get();
        if (file != null && file.exists()) {
            return file.getName();
        } else {
            return "[NONE]";
        }
    }, this);

    @Override public StringBinding asString(){
        return string ;
    }
}

For what it's worth, I tend to prefer a style in which I avoid subclassing unless necessary. In this case, I'd make the StringBinding a separate object that merely binds to the file property. The choice here depends on the use case, but this will work for most use cases and you never find yourself asking "are my overridden methods interacting in a way that doesn't work", and in general bugs like the one you had are more obvious in this style:

ObjectProperty<File> bgmFile = new SimpleObjectProperty(this, "bgmFile", null);
StringBinding fileName = Bindings.createStringBinding( () -> {
    File file = bgmFile.get();
    if (file != null && file.exists()) {
        return file.getName();
    } else return "[NONE]";
}, bgmFile);

And then of course just do playButton.textProperty().bind(fileName);.

James_D
  • 201,275
  • 16
  • 291
  • 322