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). hmm ... tend to disagree: I see the resize feature/api as independent on how the data that is measured is changed. There's no automatic resize on editing (nor on changing content), you manually trigger the method, so keep the test as simple as possible. PR: https://git.openjdk.java.net/jfx/pull/6
