On Thu, 5 Jan 2023 21:53:56 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> The current CONSTRAINED_RESIZE_POLICY has a number of issues as explained in >> [JDK-8292810](https://bugs.openjdk.org/browse/JDK-8292810). >> >> We propose to address all these issues by replacing the old column resize >> algorithm with a different one, which not only honors all the constraints >> when resizing, but also provides 4 different resize modes similar to >> JTable's. The new implementation brings changes to the public API for >> Tree/TableView and ResizeFeaturesBase classes. Specifically: >> >> - create a public abstract javafx.scene.control.ConstrainedColumnResizeBase >> class >> - provide an out-of-the box implementation via >> javafx.scene.control.ConstrainedColumnResize class, offeting 4 resize modes: >> AUTO_RESIZE_NEXT_COLUMN, AUTO_RESIZE_SUBSEQUENT_COLUMNS, >> AUTO_RESIZE_LAST_COLUMN, AUTO_RESIZE_ALL_COLUMNS >> - add corresponding public static constants to Tree/TableView >> - make Tree/TableView.CONSTRAINED_RESIZE_POLICY an alias to >> AUTO_RESIZE_SUBSEQUENT_COLUMNS (a slight behavioral change - discuss) >> - add getContentWidth() and setColumnWidth(TableColumnBase<S,?> col, double >> width) methods to ResizeFeatureBase >> - suppress the horizontal scroll bar when resize policy is instanceof >> ConstrainedColumnResizeBase >> - update javadoc >> >> >> Notes >> >> 1. The current resize policies' toString() methods return >> "unconstrained-resize" and "constrained-resize", used by the skin base to >> set a pseudostate. All constrained policies that extend >> ConstrainedColumnResizeBase will return "constrained-resize" value. >> 2. The reason an abstract class ( ConstrainedColumnResizeBase) was chosen >> instead of a marker interface is exactly for its toString() method which >> supplies "constrained-resize" value. The implementors might choose to use a >> different value, however they must ensure the stylesheet contains the same >> adjustments for the new policy as those made in modena.css for >> "constrained-resize" value. > > Andy Goryachev has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 101 commits: > > - Merge remote-tracking branch 'origin/master' into 8293119.constrained > - 8293119: removed debug printouts > - Merge remote-tracking branch 'origin/master' into 8293119.constrained > - width indicator in tester > - 2023 > - 2023 > - small delta > - whitespace > - whitespace > - Merge remote-tracking branch 'origin/master' into 8293119.constrained > - ... and 91 more: https://git.openjdk.org/jfx/compare/ca29cc61...795d196e I did some additional testing, and other than the minor issues already identified with fractional screen scales (and tracked by follow-up bugs), everything looks good. I left a few mostly minor comments inline. The only thing I'd definitely like to see is at least some testing of `TreeTableView`. modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ResizeHelper.java line 36: > 34: * Helper class for constrained column resize policies. > 35: * > 36: * https://bugs.openjdk.org/browse/JDK-8293119 Minor: I don't see the need to put a pointer to the JBS bug here. modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ResizeHelper.java line 146: > 144: } > 145: } > 146: } while (needsAnotherPass); I presume this won't get stuck in an infinite loop? I think it's fine, since the only time it will set `needsAnotherPass` is when it removes a column from consideration, and eventually, if all columns get removed, it will return anyway because `total` will be zero. So I think this is fine. modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ResizeHelper.java line 310: > 308: protected boolean distributeDelta(int ix, double delta) { > 309: int ct = count - skip.cardinality(); > 310: switch(ct) { Minor: add space after `switch` modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ResizeHelper.java line 322: > 320: double adj; > 321: > 322: switch(mode) { Minor: add space after `switch` modules/javafx.controls/src/test/java/test/javafx/scene/control/ResizeHelperTest.java line 49: > 47: * > 48: * TODO rename > 49: * TODO parallel class with TreeTableView, or as part of each test? What is your plan to test `TreeTableView`? modules/javafx.controls/src/test/java/test/javafx/scene/control/ResizeHelperTest.java line 214: > 212: */ > 213: //@Test // this test takes too much time! > 214: public void testWidthChange() { Do you think testing a pseudo-random subset of the combinations would be interesting? Loosely related to this, it might be useful follow-up test enhancement to add a generic `long.running.test` system property (or similar) that could be set when a `LONG_RUNNING_TEST` gradle property is set (similar to what we do for `unstable.test`). That way this, and other similarly exhaustive tests, could be run explicitly when desired. tests/manual/tester/src/com/oracle/javafx/tester/ATableViewResizeTester.java line 63: > 61: * Tests TableView/JTable constrained column resize modes. > 62: */ > 63: public class ATableViewResizeTester extends Application { Minor: Maybe drop the initial `A` and just call it `ATableViewResizeTester`? The "A" seems odd. Suggestion: it would be useful to have a `TreeTableView` tester. I presume you have done at least some testing with `TreeTableView`? ------------- PR: https://git.openjdk.org/jfx/pull/897
