JDK-8215964 UnsupportedOperation thrown by NestedTableColumnHeader

[Maxoudela]

Issue can be found here : https://bugs.openjdk.java.net/browse/JDK-8215964

[Maxoudela]

Hi @aghaisas , if you run this modified version of TableColumnHeaderTest, you will see the exception:

import com.sun.javafx.tk.Toolkit;
import javafx.collections.FXCollections;
import javafx.collections.ObservableList;
import javafx.event.EventType;
import javafx.scene.Scene;
import javafx.scene.control.Skin;
import javafx.scene.control.TableColumn;
import javafx.scene.control.TableColumnBase;
import javafx.scene.control.TablePosition;
import javafx.scene.control.TableView;
import javafx.scene.control.cell.PropertyValueFactory;
import javafx.scene.control.skin.NestedTableColumnHeader;
import javafx.scene.control.skin.TableColumnHeader;
import javafx.scene.control.skin.TableHeaderRow;
import javafx.scene.control.skin.TableViewSkin;
import javafx.stage.Stage;
import org.junit.Before;
import org.junit.Test;
import test.com.sun.javafx.scene.control.test.Person;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;

public class TableColumnHeaderTest {

    private MyTableColumnHeader tableColumnHeader;

    @Before
    public void beforeTest() {
        tableColumnHeader = null;
    }

    @Test
    public void test_resizeColumnToFitContent() {
        ObservableList<Person> model = FXCollections.observableArrayList(
                new Person("Humphrey McPhee", 76),
                new Person("Justice Caldwell", 30),
                new Person("Orrin Davies", 30),
                new Person("Emma Wilson", 8)
        );
        TableColumn<Person, String> column = new TableColumn<>("Col ");
        column.setCellValueFactory(new PropertyValueFactory<Person, String>("firstName"));
        TableView<Person> tableView = new TableView<>(model) {
            @Override
            protected Skin<?> createDefaultSkin() {
                return new TableViewSkin(this) {
                    @Override
                    protected TableHeaderRow createTableHeaderRow() {
                        return new TableHeaderRow(this) {
                            @Override
                            protected NestedTableColumnHeader createRootHeader() {
                                return new NestedTableColumnHeader(null) {
                                    @Override
                                    protected TableColumnHeader createTableColumnHeader(TableColumnBase col) {
                                        tableColumnHeader = new MyTableColumnHeader(column);
                                        return new NestedTableColumnHeader(col);
                                    }
                                };
                            }
                        };
                    }
                };
            }
        };

        tableView.getColumns().add(column);

        Toolkit tk = Toolkit.getToolkit();

        Scene scene = new Scene(tableView);
        Stage stage = new Stage();
        stage.setScene(scene);
        stage.setWidth(500);
        stage.setHeight(400);
        stage.centerOnScreen();
        stage.show();

        tk.firePulse();

        double width = column.getWidth();
        tableColumnHeader.resizeCol();
        assertEquals("Width must be the same",
                width, column.getWidth(), 0.001);

        EventType<TableColumn.CellEditEvent<Person, String>> eventType = TableColumn.editCommitEvent();
        column.getOnEditCommit().handle(new TableColumn.CellEditEvent<Person, String>(
                tableView, new TablePosition<Person, String>(tableView, 0, column), (EventType) eventType, "This is a big text inside that column"));
        tableColumnHeader.resizeCol();
        assertTrue("Column width must be greater",
                width < column.getWidth());

        column.getOnEditCommit().handle(new TableColumn.CellEditEvent<Person, String>(
                tableView, new TablePosition<Person, String>(tableView, 0, column), (EventType) eventType, "small"));
        column.getOnEditCommit().handle(new TableColumn.CellEditEvent<Person, String>(
                tableView, new TablePosition<Person, String>(tableView, 1, column), (EventType) eventType, "small"));
        column.getOnEditCommit().handle(new TableColumn.CellEditEvent<Person, String>(
                tableView, new TablePosition<Person, String>(tableView, 2, column), (EventType) eventType, "small"));
        column.getOnEditCommit().handle(new TableColumn.CellEditEvent<Person, String>(
                tableView, new TablePosition<Person, String>(tableView, 3, column), (EventType) eventType, "small"));

        tableColumnHeader.resizeCol();
        assertTrue("Column width must be smaller",
                width > column.getWidth());
    }

    private class MyTableColumnHeader extends TableColumnHeader {

        public MyTableColumnHeader(final TableColumnBase tc) {
            super(tc);
        }

        public void resizeCol() {
            resizeColumnToFitContent(getTableColumn(), -1);
        }
    }
}

Then the exception thrown is :

test.javafx.scene.control.skin.TableColumnHeaderTest > test_resizeColumnToFitContent FAILED
    java.lang.UnsupportedOperationException
        at java.base/java.util.AbstractList.set(AbstractList.java:136)
        at javafx.controls/javafx.scene.control.skin.NestedTableColumnHeader.updateTableColumnHeaders(NestedTableColumnHeader.java:463)
        at javafx.controls/javafx.scene.control.skin.NestedTableColumnHeader.checkState(NestedTableColumnHeader.java:638)
        at javafx.controls/javafx.scene.control.skin.NestedTableColumnHeader.computePrefHeight(NestedTableColumnHeader.java:345)
        at javafx.graphics/javafx.scene.Parent.prefHeight(Parent.java:1031)
        at javafx.graphics/javafx.scene.layout.Region.prefHeight(Region.java:1559)
        at javafx.controls/javafx.scene.control.skin.NestedTableColumnHeader.computePrefHeight(NestedTableColumnHeader.java:351)
        at javafx.graphics/javafx.scene.Parent.prefHeight(Parent.java:1031)
        at javafx.graphics/javafx.scene.layout.Region.prefHeight(Region.java:1559)
        at javafx.controls/javafx.scene.control.skin.TableHeaderRow.computePrefHeight(TableHeaderRow.java:402)
        at javafx.controls/javafx.scene.control.skin.TableHeaderRow.computeMinHeight(TableHeaderRow.java:395)
        at javafx.graphics/javafx.scene.Parent.minHeight(Parent.java:1059)
        at javafx.graphics/javafx.scene.layout.Region.minHeight(Region.java:1525)
        at javafx.controls/javafx.scene.control.SkinBase.computeMinHeight(SkinBase.java:311)
        at javafx.controls/javafx.scene.control.Control.computeMinHeight(Control.java:512)
        at javafx.graphics/javafx.scene.Parent.minHeight(Parent.java:1059)
        at javafx.graphics/javafx.scene.layout.Region.minHeight(Region.java:1525)
        at javafx.graphics/javafx.scene.Scene.getPreferredHeight(Scene.java:1812)
        at javafx.graphics/javafx.scene.Scene.resizeRootToPreferredSize(Scene.java:1776)
        at javafx.graphics/javafx.scene.Scene.preferredSize(Scene.java:1747)
        at javafx.graphics/javafx.scene.Scene$2.preferredSize(Scene.java:393)
        at javafx.graphics/com.sun.javafx.scene.SceneHelper.preferredSize(SceneHelper.java:66)
        at javafx.graphics/javafx.stage.Window$12.invalidated(Window.java:1086)
        at javafx.base/javafx.beans.property.BooleanPropertyBase.markInvalid(BooleanPropertyBase.java:110)
        at javafx.base/javafx.beans.property.BooleanPropertyBase.set(BooleanPropertyBase.java:145)
        at javafx.graphics/javafx.stage.Window.setShowing(Window.java:1174)
        at javafx.graphics/javafx.stage.Window.show(Window.java:1189)
        at javafx.graphics/javafx.stage.Stage.show(Stage.java:273)
        at test.javafx.scene.control.skin.TableColumnHeaderTest.test_resizeColumnToFitContent(TableColumnHeaderTest.java:103)

[kleopatra]

hmm ... still in post-vacation mode, so probably missing something obvious, but: either this test is pathological or I don't understand at all what it is testing ;)

parts that I don't understand:

• why is the external field tableColumnHeader manually created in createTableColumnHeader but not returned as the header to use for that column?
• looks like it is never added to the scene, is it? If not, why should I expect the resizeCol (*cough .. not a good name, IMO) to measure the width (of the header itself) at all?
• returning a NestedColumnHeader for a not-nested column sounds like a - at least borderline - contract violation of createTableColumnHeader
• why is it using edit handlers? shouldn't updating the data have the same effect?

As to the barking code block as noted in the bug-report: it is indeed whacky but I always thought it unreachable as long as createTableColumnHeader is a "normal" implemention (which should return a header of type NestedTableColumnHeader only if the column has child columns at the time of creation and a plain TableColumnHeader otherwise). So the question is: what's the use-case to start with a NestedHeader for a childless column?

With a good use-case, the code block indeed needs to be revisited: it's not acceptable to let the header directly fiddle in the internals of parent's child headers by replacing itself with another version, IMO. In a way, it's good that it blows when the nested header tries :)

[Maxoudela]

Hi,

This original test is located here : https://github.com/javafxports/openjdk-jfx/pull/289

I simply took it and modify it in order to show that we can run into this current bug. Thus the weird code line since I coded that very quickly.

Please bear in mind that in order to provide pull requests for OpenJFX, I have to start an old Mac that I don't know how to use, then use IntelliJ that I don't know how to use, then fight to compile with a custom JDK and OpenJFX, then fight with gradle to build the whole thing. I have an extreme difficulty coding tests, building them and debug them! That's the reason why the code is a bit weird because I went around in other tests and stole some piece of code in order to have something working as quick as possible. So regarding the test, I do believe it's true and I suggest you go to the PR https://github.com/javafxports/openjdk-jfx/pull/289 and make direct comment on the file line and I'll gladly update them.

Regarding this very specific bug, I stumbled on it when I was trying to implement my custom TableView. I clearly know now that I was violating the API description when badly overriding the createTableColumnHeader. That's the very first thing I said in the original bug ticket, that this bug can happen but should not. So this code should deal with the case in a proper way, maybe an exception like you said.

[kleopatra]

no offence meant - sry! Let's try to get on the same page :)

• we agree that the code block you cited in the bug report is whacky: if reached somehow, it will blow because it's trying to modify an unmodifiable list
• we agree that this potential blast was introduced when moving the skins into public scope in fx9: at that point in time, the writable access to the columnHeaders was changed into a read-only access
• we agree that the current test code trying to expose that glitch is violating the contract of createTableColumnHeader
• we (hopefully :) agree that exposing a glitch by a contract violation is ... as bad as the original bug

Do we? If so, the question is what to do from here on. Options:

• actively try to find a valid use-case that blows
• ignore the bug for now and wait for a real-world valid use-case that blows
• the other way round: try to proof that the block is unreachable and remove it
• remove the potentially offending block and wait for real-world use-case that blows
• ?

Thoughts?

[Maxoudela]

I do agree on the 1st, 2nd and 4th point. However, the contract of createTableColumnHeader dictates :

The general pattern for implementing this method is as follows: {...} then it is suggested to return a {@link TableColumnHeader} instance

Therefore, IMHO, the Javadoc is not forcing or explicitly saying that a TableColumnHeader must be returned in the given use-case. So we are not doing what the javadoc is advising, but can we say that we are violating the contract?

Anyhow, we need to understand what was the purpose of the faulty code block. If its existence has no purpose regarding the createTableColumnHeader code block, I suggest we remove it and replace it with an exception or something similar. Because I believe it should blow if the implementation of createTableColumnHeader is badly overridden. And also clarify the createTableColumnHeader javadoc if necessary.

[kleopatra]

Would regard that as a doc error, not stronger enforcing the nested for columns with child columns :)

All (?) parts of the implementation assume that fact, except the code block in question. F.i. look at the private represents(tableColumn) that's used in the else block to cleanup headers if needed:

    // Used to test whether this column header properly represents the given column.
    // In particular, whether it has child column headers for all child columns
    boolean represents(TableColumnBase<?, ?> column) {
        if (column.getColumns().isEmpty()) {
            // this column has no children, but we are in a NestedTableColumnHeader instance,
            // so the match is bad.
            return false;
        }
     ...

As to the purpose: The "normal" direction of cleanup (after receiving changes from table.visibleLeafColumns) is top-down - the else-block - a parent adjusts its headers to the new state. In the if-block, that normal direction is inversed. Personally, I think it's left-over code from the very very early stages of dev that nobody dared to touch.

Throwing a IllegalStateException as well as clarifying the doc sounds like good ideas :)

[kleopatra]

*cough ... just noticed that my custom NestedTableColumnHeader even expects to be created for a leaf-column only ... doesn't reach the if code block because it takes over all internal updates completely

no, need more coffee, sry ;)