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

Fri Dec 9 19:43:55 UTC 2022

On Tue, 6 Dec 2022 16:41:05 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 71 commits:
> 
>  - Merge remote-tracking branch 'origin/master' into 8293119.constrained
>  - 8293119: small delta
>  - Merge remote-tracking branch 'origin/master' into 8293119.constrained
>  - 8293119: pref
>  - Merge remote-tracking branch 'origin/master' into 8293119.constrained
>  - 8293119: step1
>  - 8293119: more integers
>  - 8293119: more integers
>  - 8293119: use integers for rounded values
>  - 8293119: tester
>  - ... and 61 more: https://git.openjdk.org/jfx/compare/6f36e704...0122d5fc

I left a few minor doc comments. The docs otherwise looks good to me.

modules/javafx.controls/src/main/java/javafx/scene/control/ResizeFeaturesBase.java line 64:

> 62:   public double getContentWidth() {
> 63:       // not available in the base class
> 64:       throw new UnsupportedOperationException("method not available in the base class");

Since the default implementation of this method throws `UnsupportedOperationException`, I recommend documenting that subclasses must override this method.

modules/javafx.controls/src/main/java/javafx/scene/control/ResizeFeaturesBase.java line 76:

> 74:   public Control getTableControl() {
> 75:       // not available in the base class
> 76:       throw new UnsupportedOperationException("method not available in the base class");

Ditto.

modules/javafx.controls/src/main/java/javafx/scene/control/TableView.java line 400:

> 398:     /**
> 399:      * A policy that tries to adjust other columns in order to fit the table width.
> 400:      * <p>

For all of the constrained policies, I would say `A resize policy that adjusts...`. I think the word "resize" is important in the first sentence, and I don't think you need to say "tries", since it will adjust it, subject to the minimum size of the columns being adjusted (and the details of what happens when it hits the limit are explained later). Also, I don't think the `<p>` is really needed after the first sentence.

modules/javafx.controls/src/main/java/javafx/scene/control/TableView.java line 401:

> 399:      * A policy that tries to adjust other columns in order to fit the table width.
> 400:      * <p>
> 401:      * During UI adjustment, proportionately resizes all columns.

You might want to add `to preserve the total width` here since you say it for some of the other policies.

modules/javafx.controls/src/main/java/javafx/scene/control/TableView.java line 413:

> 411: 
> 412:     /**
> 413:      * A policy that tries to adjust last column in order to fit the table width.

That should be "_the_ last column"

modules/javafx.controls/src/main/java/javafx/scene/control/TableView.java line 415:

> 413:      * A policy that tries to adjust last column in order to fit the table width.
> 414:      * <p>
> 415:      * During UI adjustment, resizes the last column only.

You might want to add `to preserve the total width` here since you say it for some of the other policies.

modules/javafx.controls/src/main/java/javafx/scene/control/TableView.java line 427:

> 425: 
> 426:     /**
> 427:      * A policy adjusts the next column in the opposite way in order to fit the table width.

I would remove "in the opposite way" here, since it is equally true of most of the other policies, and is covered later. When looking at just the summary list of the first sentences for each policy I think it reads better without it.

modules/javafx.controls/src/main/java/javafx/scene/control/TableView.java line 429:

> 427:      * A policy adjusts the next column in the opposite way in order to fit the table width.
> 428:      * <p>
> 429:      * During UI adjustment, resizes the next column the opposite way.

Here, it seems OK to say "the opposite way", since it adds more information.

modules/javafx.controls/src/main/java/javafx/scene/control/TableView.java line 443:

> 441:      * A policy that tries to adjust subsequent columns in order to fit the table width.
> 442:      * <p>
> 443:      * During UI adjustment, resizes subsequent columns to preserve the total width.

Maybe say `proportionately resizes...` to match the language in `CONSTRAINED_RESIZE_POLICY_ALL_COLUMNS`?

-------------

PR: https://git.openjdk.org/jfx/pull/897