SearchableComboBox bugfix for blank selections (nulls) on focus lost

9 views
Skip to first unread message

Mark Schmieder

unread,
Apr 1, 2023, 2:07:11 AM4/1/23
to ControlsFX
I discovered this bug right before the pandemic and have been too much in survival mode since then to report and contribute, but I do not see it in current GitHub code so it may not have been detected by others yet.

Basically, if you move focus from the search field without making a selection, this can result in unknown state, blank selection (cached to the data model!), null pointers, etc.

The solution is quite simple, and I believe it should be done as a pull request and integrated into all active ControlsFX branches. 

See the end of the bindSearchFieldAndFilteredComboBox() method in SearchableComboBoxSkin. My apologies for any formatting changes that Eclipse did to me (I now use IntelliJ, but not yet for my GitHub contributions -- soon, I hope!).

As it is now past 2am, I'm running out of energy for restructuring code to match the original. Only the last few lines are functionally different in this correction:

private void bindSearchFieldAndFilteredComboBox() {

// Set the items of the filtered ComboBox.

filteredComboBox.setItems( createFilteredList() );


// Keep it up to date, even if the original list changes.

final ComboBox< T > comboBox = getSkinnable();

final FilteredList< T > filteredList = createFilteredList();

comboBox.itemsProperty().addListener( ( observable, oldValue, newValue ) -> filteredComboBox

.setItems( filteredList ) );


// Update the filter, when the text in the search field changes.

searchField.textProperty().addListener( observable -> updateFilter() );


// The search field must only be visible, when the pop-up is showing.

searchField.visibleProperty().bind( filteredComboBox.showingProperty() );


filteredComboBox.showingProperty().addListener( ( observable, oldValue, newValue ) -> {

if ( newValue ) {

// When the filtered ComboBox pop-up is showing, we must also

// set the showing property of the original ComboBox. And here

// we must remember the previous value for the ESCAPE behavior.

// And we must transfer the focus to the search field, because

// otherwise the search field would not allow typing in the

// search text.

comboBox.show();

previousValue = comboBox.getValue();

searchField.requestFocus();

}

else {

// When the filtered ComboBox pop-up is hidden, we must also

// set the showing property of the original ComboBox to false,

// and clear the search field.

comboBox.hide();

searchField.setText( "" );

}

} );


// When the search field is focused, the pop-up must still be shown.

searchField.focusedProperty().addListener( ( observable, oldValue, newValue ) -> {

if ( newValue ) {

filteredComboBox.show();

}

else {

// If focus is lost from the search field, but nothing was

// selected (usually this is due to invalid search criteria

// leading to an empty drop-list), we must restore the previous

// selection in order to avoid a blank selection. Note that

// checking for an empty drop-list doesn't work, as it is

// state-dependent due to when, where, and why it gets cached.

if ( filteredComboBox.getSelectionModel().isEmpty() ) {

comboBox.setValue( previousValue );

}

filteredComboBox.hide();

}

} );

}




Reply all
Reply to author
Forward
0 new messages