RFR: 8293119: Additional constrained resize policies for Tree/TableView [v31]

[Kevin Rushforth]

On Thu, 5 Jan 2023 21:53:56 GMT, Andy Goryachev <angorya at 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