RFR: 8359599: Calling refresh() for all virtualized controls recreates all cells instead of refreshing the cells

[Marius Hanl]

On Tue, 17 Jun 2025 07:31:45 GMT, Johan Vos <jvos at openjdk.org> wrote:

> Hence, while I like the idea here (avoiding unneeded heavy-cost operations in VirtualFlow), I would like to avoid a number of follow-up issues once this is merged -- driven by a change in "expected" behavior.

I completely agree. We need to be careful with such changes.

> However, it is very likely that some code out there implicitly rely on the rebuild-from-scratch logic, and that code will then work different after applying this PR.

Since all rows (and cells) are reset and then updated, all changes that were not taken into account by the control are taken into account in any case then.
On a side note here: In any JavaFX project, I have overwritten the `refresh` method (since it is not final) and always implemented the lightweight method as proposed here. I never found any problem.

But I think there is one concrete case which breaks now.
Take the following example (slightly modified version of the code in the ticket):
<details>
<summary>Example</summary>


import java.util.concurrent.atomic.AtomicBoolean;

import javafx.application.Application;
import javafx.beans.property.SimpleStringProperty;
import javafx.collections.FXCollections;
import javafx.geometry.Insets;
import javafx.scene.Scene;
import javafx.scene.control.Button;
import javafx.scene.control.TableColumn;
import javafx.scene.control.TableRow;
import javafx.scene.control.TableView;
import javafx.scene.layout.Priority;
import javafx.scene.layout.VBox;
import javafx.stage.Stage;

public class TableRefresh extends Application {

    public static void main(String[] args) {
        launch(args);
    }

    @Override
    public void start(Stage stage) throws Exception {
        TableView<String> tv = new RefreshTableView<>();
        tv.setEditable(true);
        AtomicBoolean alternate = new AtomicBoolean(false);

        tv.setRowFactory(e -> {
            if (alternate.get()) {
                System.out.println("Creating alternative row");
                return new TableRow<>() {
                    {
                        setPadding(new Insets(5));
                    }
                };
            }

            System.out.println("Creating row");
            return new TableRow<>();
        });
        TableColumn<String, String> col = new TableColumn<>("Name");
        col.setCellValueFactory(i -> new SimpleStringProperty(i.getValue()));

        tv.getColumns().addAll(col);

        tv.setItems(FXCollections.observableArrayList("A", "B"));
        VBox.setVgrow(tv, Priority.ALWAYS);
        Button btn = new Button("Refresh");
        btn.setOnAction(e -> {
            alternate.set(true);
            tv.refresh();
        });
        Scene scene = new Scene(new VBox(tv, btn));

        stage.setScene(scene);
        stage.setTitle("Table Refresh");
        stage.show();
    }
}

</details>

Here, I'm aware that `refresh()` is recreating all rows, and update a boolean flag before calling `refresh()`, which leads to another path that is picked up and therefore other rows. 
In this case, it is much better to just set a new `TableRow` factory. This will do the same as what `refresh()` is doing right now (but not after this PR).
In never saw this code in the wild though.

But that is still a valid risk that we need to consider. This is the only problem I see right now.

-------------

PR Comment: https://git.openjdk.org/jfx/pull/1830#issuecomment-2983677523