On Wed, 30 Oct 2019 13:59:08 GMT, Hadzic Samir <shad...@openjdk.org> wrote:
> The pull request has been updated with additional changes. > > ---------------- > > Added commits: > - 2b088993: Add @implSpec tag for javadoc of TableColumnHeader > > Changes: > - all: https://git.openjdk.java.net/jfx/pull/6/files > - new: https://git.openjdk.java.net/jfx/pull/6/files/1f1f7c44..2b088993 > > Webrevs: > - full: https://webrevs.openjdk.java.net/jfx/6/webrev.04 > - incr: https://webrevs.openjdk.java.net/jfx/6/webrev.03-04 > > Issue: https://bugs.openjdk.java.net/browse/JDK-8207957 > Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 mod > Patch: https://git.openjdk.java.net/jfx/pull/6.diff > Fetch: git fetch https://git.openjdk.java.net/jfx pull/6/head:pull/6 modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java line 54: > 53: private MyTableColumnHeader tableColumnHeader; > 54: > 55: @Before If I understand the intention correctly, the only reason for subclassing the TableView with a custom skin, headerRow, etc deep down into a custom columnHeader is to access its protected resizeColumnToFitContent? If so, an alternative approach might be to a) add a accessing method to TableColumnHeaderUtil in the shims hierarchy b) don't subclass but use that new accessor modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java line 61: > 60: @Test > 61: public void test_resizeColumnToFitContent() { > 62: ObservableList<Person> model = FXCollections.observableArrayList( This method actually is three-in-one :) It's testing a) trigger a resize initially doesn't change the with b) change content to larger and trigger a resize increases column width c) change content to smaller and trigger a resize decreases column width Not sure about conventions here, but I would separate them out into distinct methods. modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java line 117: > 116: column.getOnEditCommit().handle(new > TableColumn.CellEditEvent<Person, String>( > 117: tableView, new TablePosition<Person, String>(tableView, > 0, column), (EventType) eventType, "This is a big text inside that column")); > 118: tableColumnHeader.resizeCol(); that looks like a rather convoluted way of changing content - what's wrong with straightforward changing the firstName of the first item :) modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java line 121: > 120: width < column.getWidth()); > 121: > 122: column.getOnEditCommit().handle(new > TableColumn.CellEditEvent<Person, String>( feels a bit unspecific (though I can't think of how to do it better - the internal measuring is ... doohoo ;) what I usually do in such a case is to take this as a first step, then change content back to original and again trigger a resize: now the current width should be the same as the initial width. modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java line 133: > 132: assertTrue("Column width must be smaller", > 133: width > column.getWidth()); > 134: } same as above but the other way round. Until, the tests verified that the column width is effected by cell content and goes into the course direction of what is expected. What's still missing are tests that cover the complete specification (we now have one :), that is verify that/how resize is indeed depend on - header content - maxRows Plus maybe that custom implementations are respected: f.i. doing nothing, making them constant, header only An open question for me is: what is the standard procedure here? a) there are no tests around sizing (that I could find, maybe overlooked them) b) the change in this pull request is (more or less) simply moving around old code and pulling from private into protected scope, no changes in implementation/functionality Now is the contributor responsible for writing those missing tests? It can be considered a good opportunity ;) And strictly speaking is mandatory (probably?) for all public/protected api. On the other extreme, doing nothing (no tests because basically nothing changed) might be appropriate as well. ---------------- Disapproved by fastegal (Author). PR: https://git.openjdk.java.net/jfx/pull/6
