RFR: 8293119: Alternative CONSTRAINED_RESIZE_POLICY [v4]

Wed Nov 23 20:43:31 UTC 2022

On Mon, 21 Nov 2022 23:32:52 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 incrementally with one additional commit since the last revision:
> 
>   8293119: newline

API feedback:

The set of new policies looks good. All of them need an `@since` tag. Also, the docs for each policy should describe _how_ that policy distributes the space on a resize. Maybe you can borrow some of the language from the Swing docs.

I think `CONSTRAINED_RESIZE_POLICY_FLEX_HEAD` and `CONSTRAINED_RESIZE_POLICY_FLEX_TAIL` could benefit from a more descriptive name. The idea is that `CONSTRAINED_RESIZE_POLICY_FLEX_HEAD` is a flexible variant of "next column", meaning it is the same as `CONSTRAINED_RESIZE_POLICY_NEXT_COLUMN` until that column is at its minimum; then, rather than stopping, it takes the column after that. Similarly, `CONSTRAINED_RESIZE_POLICY_FLEX_TAIL` is a flexible "last column", meaning it is the same as `CONSTRAINED_RESIZE_POLICY_LAST_COLUMN` until that column is at its minimum; then, rather than stopping, it takes the column before that. So how about `CONSTRAINED_RESIZE_POLICY_NEXT_COLUMN_FLEX` and `CONSTRAINED_RESIZE_POLICY_LAST_COLUMN_FLEX` as names (alternatively, `FLEX` can go before `NEXT_COLUMN` and `LAST_COLUMN`)?

The `ConstrainedColumnResize` and `ResizeHelper` classes look like implementation details that don't need to be in the public API.

Other API comments are inline.

modules/javafx.controls/src/main/java/javafx/scene/control/ConstrainedColumnResize.java line 25:

> 23:  * questions.
> 24:  */
> 25: package javafx.scene.control;

Minor: add a blank line.

modules/javafx.controls/src/main/java/javafx/scene/control/ConstrainedColumnResize.java line 38:

> 36:  * @since 20
> 37:  */
> 38: public class ConstrainedColumnResize extends ConstrainedColumnResizeBase {

This class is an implementation detail. I see no reason for it to be part of the public API. Since it isn't used outside this package, you can probably just remove the "public" from the class. Alternatively, you can move it somewhere under `com.sun.javafx.scene.control`. Either way, once this is no longer part of the public API surface, you would remove the `@since`.

modules/javafx.controls/src/main/java/javafx/scene/control/ConstrainedColumnResize.java line 39:

> 37:  */
> 38: public class ConstrainedColumnResize extends ConstrainedColumnResizeBase {
> 39:     public enum ResizeMode {

And this enum is _definitely_ an implementation detail.

modules/javafx.controls/src/main/java/javafx/scene/control/ConstrainedColumnResizeBase.java line 25:

> 23:  * questions.
> 24:  */
> 25: package javafx.scene.control;

Minor: please add a blank line between the copyright header and package declaration.

modules/javafx.controls/src/main/java/javafx/scene/control/ConstrainedColumnResizeBase.java line 30:

> 28:  * Base class for a constrained column resize policy.
> 29:  * Setting any policy that extends this class on a Tree/TableView results in
> 30:  * disabling of its horizontal scroll bar.

It would be helpful to add `@see` tags for `Tree/TableView::columnResizePolicyProperty`

modules/javafx.controls/src/main/java/javafx/scene/control/ConstrainedColumnResizeBase.java line 34:

> 32:  * @since 20
> 33:  */
> 34: public abstract class ConstrainedColumnResizeBase {

Do you think this class would benefit from a generic parameter for the specific type (`TableView.ResizeFeatures` or `TreeTableView.ResizeFeatures`)? In order for that to be useful, it would also need to extend `Callback` and declare an abstract call method using the generic type. I don't know whether this is worth it or not.

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

> 57:    * Returns the width of the area available for columns.
> 58:    */
> 59:   public double getContentWidth() {

You need to add the missing `@return` and `@since` tags.

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

> 65:    * Returns the associated TreeView or TreeTableView
> 66:    */
> 67:   public Node getTableNode() {

I recommend making the return type and name `Control` rather than `Node`. So:

  public Control getTableControl()

Also, since the (two) subclasses need to override this method, you might add a sentence to that effect.

FInally, the docs are missing a trailing period at the end of the description, and missing the `@return` tag (which should not end with a period) and the `@since` tag.

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

> 88:   /**
> 89:    * Sets the column width during the resizing pass.
> 90:    */

You need to add the missing `@param`, `@return`, and `@since` tags.

modules/javafx.controls/src/main/java/javafx/scene/control/ResizeHelper.java line 33:

> 31:  * Helps resize Tree/TableView columns.
> 32:  */
> 33: public class ResizeHelper {

This is an implementation detail. It should not be part of the public API. As with `ConstrainedColumnResize`, you can either remove the "public" or move the class somewhere under `com.sun.javafx.scene.control`.

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

> 402:      * scroll bar.
> 403:      */
> 404:     public static final Callback<TableView.ResizeFeatures, Boolean> CONSTRAINED_RESIZE_POLICY_ALL_COLUMNS =

You need to add an `@since` tag to all of the new policies.

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

> 467:      * resized column any more.
> 468:      */
> 469:     @Deprecated(since="20")

In addition to the `@Deprecated` annotation, can you add an `@deprecated` javadoc tag indicating that it is replaced by `CONSTRAINED_RESIZE_POLICY_FLEX_TAIL`?

modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableView.java line 605:

> 603:      * resized column any more.
> 604:      */
> 605:     @Deprecated(since="20")

Same comment about also adding a `@deprecated` javadoc tag.

modules/javafx.controls/src/test/java/test/javafx/scene/control/ResizeHelperTest.java line 25:

> 23:  * questions.
> 24:  */
> 25: package test.javafx.scene.control;

Minor: add a blank like before and after the `package line`.

tests/manual/tester/src/com/oracle/javafx/tester/ATableViewResizeTester.java line 25:

> 23:  * questions.
> 24:  */
> 25: package com.oracle.javafx.tester;

Minor: add blank line before and after the `package` line.

tests/manual/tester/src/com/oracle/javafx/tester/ATableViewResizeTester.java line 44:

> 42: import javafx.scene.control.ComboBox;
> 43: import javafx.scene.control.ConstrainedColumnResize;
> 44: import javafx.scene.control.ConstrainedColumnResize.ResizeMode;

These two classes should not be needed or used by applications (see my earlier comments).

tests/manual/tester/src/com/oracle/javafx/tester/ATableViewResizeTester.java line 213:

> 211:         switch(p) {
> 212:         case AUTO_RESIZE_FLEX_HEAD:
> 213:             return ConstrainedColumnResize.forTable(ResizeMode.AUTO_RESIZE_FLEX_HEAD);

Use the public API fields, e.g., `TableView.CONSTRAINED_RESIZE_POLICY_FLEX_HEAD`

tests/manual/tester/src/module-info.java line 1:

> 1: module andy_test {

If you intend this to be part of the final proposed PR, we need a different name. This also needs a standard Oracle copyright header.

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

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