jfx 8293119: Alternative CONSTRAINED_RESIZE

trafficstars

The current CONSTRAINED_RESIZE_POLICY has a number of issues as explained in 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

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.
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.

Progress

[ ] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
[x] Change must not contain extraneous whitespace
[x] Commit message must refer to an issue

Issue

JDK-8293119: Alternative CONSTRAINED_RESIZE_POLICY

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx pull/897/head:pull/897
$ git checkout pull/897

Update a local copy of the PR:
$ git checkout pull/897
$ git pull https://git.openjdk.org/jfx pull/897/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 897

View PR using the GUI difftool:
$ git pr show -t 897

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/897.diff

Sep 12 '22 21:09 andy-goryachev-oracle

:wave: Welcome back angorya! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

Sep 12 '22 22:09 bridgekeeper[bot]

@andy-goryachev-oracle this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout 8293119.constrained
git fetch https://git.openjdk.org/jfx master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

Sep 16 '22 17:09 openjdk[bot]

/reviewers 2

Oct 08 '22 15:10 kevinrushforth

@kevinrushforth The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

Oct 08 '22 15:10 openjdk[bot]

Webrevs

Oct 12 '22 15:10 mlbridge[bot]

Overall I quite like this enhancement. I'll focus my initial review on the public API. I'll then provide comments on the behavior of the new policies. Later I'll review the code.

Nov 23 '22 15:11 kevinrushforth

Behavioral issues:

All the policies work much better than the existing (buggy) CONSTRAINED_RESIZE_POLICY. I did notice a few oddities, though. More manual testing is needed by a few folks.

The divider being dragged doesn't track the cursor. All policies other than CONSTRAINED_RESIZE_POLICY_ALL_COLUMNS should be able to track (the Swing implementation does).
I found a bug in the implementation of CONSTRAINED_RESIZE_POLICY_SUBSEQUENT_COLUMNS with data = "fixed in the middle". To reproduce, drag the divider between C3 and C4 to the right. It will not move much at first, and then it will jump. Dragging it back to the left is even more odd in that it will immediately jump and resize the previous columns. That part is definitely a bug, since C1 and C2 shouldn't be resized at all.
The space doesn't seem to be distributed evenly with CONSTRAINED_RESIZE_POLICY_ALL_COLUMNS. The easiest way to see this is to start dragging the divider between C2 and C3 to the right. C1 doesn't start resizing at all until its made C3 and C4 quite a bit smaller. And when dragging back and forth a bit it doesn't look quite right.

Nov 23 '22 22:11 kevinrushforth

I reworded the title of the JBS Enhancement slightly to reflect that we are adding new policies (not just providing a single alternative to the existing policy). Can you update the title of this PR to match?

Nov 23 '22 22:11 kevinrushforth

The divider being dragged doesn't track the cursor. All policies other than CONSTRAINED_RESIZE_POLICY_ALL_COLUMNS should be able to track (the Swing implementation does).

Could you describe the exact scenario please? It does track, unless one hits a constraint somewhere, in which case the cursor obviously decouples from the divider.

Nov 28 '22 20:11 andy-goryachev-oracle

The divider being dragged doesn't track the cursor. All policies other than CONSTRAINED_RESIZE_POLICY_ALL_COLUMNS should be able to track (the Swing implementation does).

Could you describe the exact scenario please? It does track, unless one hits a constraint somewhere, in which case the cursor obviously decouples from the divider.

This turns out to be only reproducible on Windows with a screen scale > 1 (my default config is Windows 10 with a scale of 1.25). I suspect that this is a preexisting bug, so I'll file a new bug after confirming that.

Nov 28 '22 21:11 kevinrushforth

Added a different algorithm for distributing small changes, which produces a better result that does not suffer from a few columns getting wider at the expense of others.

Dec 06 '22 16:12 andy-goryachev-oracle

The docs look good. Go ahead and update the CSR and move it to Proposed.

JDK-8294398 updated, please take a look.

Dec 09 '22 22:12 andy-goryachev-oracle

@karthikpandelu would you be able to take a look at this PR, specifically check the new behavior using ATableViewResizeTester app? Please note anything you don't like or find questionable. Thanks!

Dec 12 '22 22:12 andy-goryachev-oracle

The API changes to make the class abstract look good.

Dec 13 '22 22:12 kevinrushforth

@karthikpandelu would you be able to take a look at this PR, specifically check the new behavior using ATableViewResizeTester app? Please note anything you don't like or find questionable. Thanks!

I checked the behavior with some of the combinations of Data and Policy options. Please find my observations below. This was run in Windows 11 system.

In ATableViewResizeTester app, JTable present in the bottom half gets displayed only when I change Policy or Data values or drag the app window using title bar of the app. Not sure if this issue is from app side.
Observed inconsistent behavior with Data value "fixed in the middle" and Policy value "CONSTRAINED_RESIZE_POLICY". Steps to reproduce:
- Run the app and select CONSTRAINED_RESIZE_POLICY policy.
- Increase and decrease the TableView size using vertical SplitPane. Column C6 size is not consistent each time when TableView size is changed. In some cases, C6 width is same as other columns like C7, C8, other times it is more than other columns.
Similar to AUTO_RESIZE_SUBSEQUENT_COLUMNS is there a provision to resize previous columns? I was just wondering if there is an option for that.

Dec 14 '22 16:12 karthikpandelu

Thank you @karthikpandelu for your work and helpful comments!

In ATableViewResizeTester app, JTable present in the bottom half gets displayed only when I change Policy or Data values or drag the app window using title bar of the app. Not sure if this issue is from app side.

@kevinrushforth mentioned this is a know problem with Swing interop on Windows (do we have a jira ticket?), can be ignored for the purposes of this PR.

Observed inconsistent behavior with Data value "fixed in the middle" and Policy value "CONSTRAINED_RESIZE_POLICY". Steps to reproduce:

made some changes, please take a look.

Similar to AUTO_RESIZE_SUBSEQUENT_COLUMNS is there a provision to resize previous columns? I was just wondering if there is an option for that.

There is no such policy (neither here nor in JTable). I suppose it is possible. I find the other policy CONSTRAINED_RESIZE_POLICY_ALL_COLUMNS is less useful than the the new default policy CONSTRAINED_RESIZE_POLICY_FLEX_LAST_COLUMN - something about changes applied from left to right feeling more natural.

Dec 14 '22 18:12 andy-goryachev-oracle

In ATableViewResizeTester app, JTable present in the bottom half gets displayed only when I change Policy or Data values or drag the app window using title bar of the app. Not sure if this issue is from app side.

@kevinrushforth mentioned this is a know problem with Swing interop on Windows (do we have a jira ticket?), can be ignored for the purposes of this PR.

The bug I had originally filed was closed as a duplicate of another bug that was later closed as Incomplete. I discovered that it was still reproducible a couple months ago, and thought I had re-filed it, but I hadn't. I just now filed a new bug JDK-8298796 to track this. So yes, this can be ignored for the purpose of this PR.

Dec 14 '22 19:12 kevinrushforth

Observed inconsistent behavior with Data value "fixed in the middle" and Policy value "CONSTRAINED_RESIZE_POLICY". Steps to reproduce:

made some changes, please take a look.

This looks fine now.

Similar to AUTO_RESIZE_SUBSEQUENT_COLUMNS is there a provision to resize previous columns? I was just wondering if there is an option for that.

There is no such policy (neither here nor in JTable). I suppose it is possible. I find the other policy CONSTRAINED_RESIZE_POLICY_ALL_COLUMNS is less useful than the the new default policy CONSTRAINED_RESIZE_POLICY_FLEX_LAST_COLUMN - something about changes applied from left to right feeling more natural.

Okay. Agreed, CONSTRAINED_RESIZE_POLICY_FLEX_LAST_COLUMN looks more natural

Dec 15 '22 04:12 karthikpandelu

While the first warning is expected, we should fix others.

Thank you for noticing!

Tried to address some of the warnings, but at this particular moment some warnings are turned off in my Eclipse. Turning on 'unchecked generic type operation' check results in 5,371 warnings - perhaps we need to wait for all the parts in JDK-8297300 to be fixed first.

Dec 19 '22 19:12 andy-goryachev-oracle

The API changes look good. I confirm that the javadoc warning is gone.

Dec 19 '22 19:12 kevinrushforth

Reworked to support fractional scaling. Left debug statements and some debugging code in for the testing/review. Will remove once everyone agrees that the code works correctly in all the scenarios.

Please take a look.

Dec 23 '22 21:12 andy-goryachev-oracle

The Windows HiDPI behavior (on my system with a screen scale of 1.25) is much better with the latest version. With a policy of NEXT or LAST, the divider tracks the mouse cursor as I would expect. With a policy of FLEX_NEXT or FLEX_LAST, it tracks until the first (or last) column being resized hits its minimum and it switches to another column. Similarly, with a policy of SUBSEQUENT, it doesn't quite track as expected when one of the columns gets small (i.e., close to its minimum). This might just be error accumulation. It seems like a minor problem.

I am seeing a new behavior, unrelated to HiDPI, that I don't recall seeing before. The "flex" policies will start out correctly taking the extra space from the next (or last) column, then unexpectedly switch to a different column, then later switch back to the next (or last) column. As far as I can tell, it only happens when the window is resized from its initial size. The easiest way to see this is:

Resize the window such that the width is larger
Resize the FX split pane making the table wider
Select "Data" -> "pref only"
Select "Policy" -> "AUTO_RESIZE_FLEX_LAST_COLUMN"
Drag the divider between "C1" and "C2" to the right

It will start out taking the space needed to make C1 larger from C4 (as expected), then switch to one of C2 or C3 before C4 has resized to its minimum, then after a while, it will switch back to C4 until it has hit its minimum.

This happens on both Mac and Windows.

Jan 03 '23 22:01 kevinrushforth

I am seeing a new behavior,

could you check the latest version please?

I think the latest version works as expected, except for one scenario with fractional size, where both AUTO_RESIZE_FLEX_LAST_COLUMN and AUTO_RESIZE_FLEX_NEXT_COLUMN result in the adjustment of the columns to the left of the one being resized (data=many columns, same pref)

Jan 03 '23 23:01 andy-goryachev-oracle

I am seeing a new behavior,

could you check the latest version please?

I just confirmed that I was testing the latest version yesterday afternoon.

I think the latest version works as expected, except for one scenario with fractional size, where both AUTO_RESIZE_FLEX_LAST_COLUMN and AUTO_RESIZE_FLEX_NEXT_COLUMN result in the adjustment of the columns to the left of the one being resized (data=many columns, same pref)

I can reproduce the bug I reported above with flex policies using a screen scale of 1 on Mac, Windows, and Linux.

Jan 04 '23 21:01 kevinrushforth

Kevin, I now see what you mean (one has to resize the table almost to the full screen width, to make sure all columns are wider than their preferred widths).

It's working as expected, actually. It first tries to use excess space (over preferred width) so it will switch from next to the following column, then, once all the columns are at their preferred widths, it starts with the column next to one being resized.

In other words, rather than squishing the next or last column to an unusable width, it first tries to use the excess space (thus the name "flex" you so aptly suggested).

Jan 04 '23 21:01 andy-goryachev-oracle

OK, that makes complete sense to me, and is, I agree, a better choice.

We might consider a follow-up doc bug to clarify this, but it isn't critical.

I also don't think the remaining HiDPI issue is critical, so that also could be a follow-up. I'll continue testing it, and also review the code.

Jan 04 '23 21:01 kevinrushforth

I'm also seeing the mouse cursor tracking issue mentioned by Kevin while dragging the column divider.

I wanted to check about following behavior. When AUTO_RESIZE_FLEX_LAST_COLUMN policy and pref only data are selected, column divider between C1 and C2 is dragged to the right. C4 size will be reduced first and reaches minimum. Then C3 and C2 column size reduces. After all the 3 columns reach minimum width, if same divider is dragged to the left , C4 gets expanded first, then C3 and C2. Where as in AUTO_RESIZE_ALL_COLUMNS policy, the column whose width was reduced last gets expanded first. Should we keep the behavior consistent across all policies? The behavior in CONSTRAINED_RESIZE_POLICY is same as FLEX policies.

I ran these test in Windows 11 system. I will try to do some more testing on the same machine.

Jan 05 '23 17:01 karthikpandelu

Where as in AUTO_RESIZE_ALL_COLUMNS policy, the column whose width was reduced last gets expanded first. Should we keep the behavior consistent across all policies?

thank you for testing, @karthikpandelu !

not really, the idea of providing several different policies with different behavior is that the app developer can choose the one that fits the requirements best.

AUTO_RESIZE_ALL_COLUMNS tries to allocate extra space proportionally to the preferred width of each column, so the behavior you are describing is expected.

Jan 05 '23 18:01 andy-goryachev-oracle

AUTO_RESIZE_ALL_COLUMNS tries to allocate extra space proportionally to the preferred width of each column, so the behavior you are describing is expected.

Understood. Thanks for the clarification.

Jan 06 '23 05:01 karthikpandelu

I realized that, in presence of snapping and fractional scale, the implemented algorithm would never work correctly. The reason is that the distance between snapped positions is not the same. As a result, changing a single column width requires shifting of subsequent or all the columns, which in turn might require a change in their widths, and so on.

This makes me think that a different algorithm might be needed, a simulated annealing perhaps.

Jan 06 '23 17:01 andy-goryachev-oracle

jfx jfx copied to clipboard

8293119: Alternative CONSTRAINED_RESIZE_POLICY

Progress

Issue

Reviewing

Webrevs

jfx
jfx copied to clipboard