Re: 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 <j...@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