On Thu, 7 Nov 2019 12:48:47 GMT, Jeanette Winzenburg <faste...@openjdk.org> wrote:
> 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). that's what the first step guards against :) PR: https://git.openjdk.java.net/jfx/pull/6
