jfx
jfx copied to clipboard
openjdk
8299753: Tree/TableView: Column Resizing With Fractional Scale
Modified the resize algorithm to work well with fractional scale, thanks for deeper understanding of the problem thanks to @hjohn and @mstr2 .
Removed earlier manual tester in favor of the monkey tester.
It is important to note that even though the constraints are given by the user in unsnapped coordinates, they are converted to snapped values, since the snapped values correspond to the actual pixels on the display. This means the tests that validate honoring constraints should, in all the cases where (scale != 1.0), assume possibly error not exceeding (1.0 / scale).
Progress
- [x] Change must not contain extraneous whitespace
- [x] Commit message must refer to an issue
- [ ] Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)
Issues
- JDK-8299753: Tree/TableView: Column Resizing With Fractional Scale (Bug - P4)
- JDK-8299755: Tree/TableView: Cursor Decouples From Divider When Resizing With Fractional Scale (Bug - P4)
Reviewers
- Karthik P K (@karthikpandelu - Committer) 🔄 Re-review required (review applies to 8eab7970)
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1156/head:pull/1156
$ git checkout pull/1156
Update a local copy of the PR:
$ git checkout pull/1156
$ git pull https://git.openjdk.org/jfx.git pull/1156/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1156
View PR using the GUI difftool:
$ git pr show -t 1156
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1156.diff
Webrev
: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.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
Webrevs
- 14: Full (e48038c7)
- 13: Full (b9a105ee)
- 12: Full (197cfaf0)
- 11: Full (25533552)
- 10: Full - Incremental (4071e2f6)
- 09: Full - Incremental (48a1bb5d)
- 08: Full - Incremental (9105b009)
- 07: Full - Incremental (80286f26)
- 06: Full - Incremental (5e855022)
- 05: Full - Incremental (abd9af92)
- 04: Full - Incremental (df87de91)
- 03: Full - Incremental (908203cd)
- 02: Full - Incremental (2e06578e)
- 01: Full - Incremental (8eab7970)
- 00: Full (be67d858)
@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).
@hjohn would you mind taking a look at this? using the ideas from your SpaceDistributor
@hjohn would you mind taking a look at this? using the ideas from your SpaceDistributor
Will take a look this weekend
My observation is that this algorithm seems unable to provide a proper user resizing experience as it seems to discard important information it would need to do so.
please elaborate, or point to a specific problem. It is entirely possible that a better algorithm might exist, but it might be out of scope at the moment, as this is a follow-up for a specific issue.
The
ResizeHelperis short-lived, and created/recreated many times during a resize operation. I'm not even sure why it is being instantiated at all (it literally is created and discarded in a single method).
convenience - it does have some data, and it's easier to keep code along with the data.
The
SMALL_DELTAconstant that changes behavior on how large a "drag" was registered is a red flag; this shouldn't matter. The sizes should always be based on what they were initially (at the start of the drag), and where the cursor is currently, regardless of what path the cursor took to get there (ie. there should be no memory effects, the algorithm should only need the initial sizes + current position).
This is not my experience. Specifically, the difference in behavior between small changes (when the users resizes the columns slowly and we get small deltas of 1 pixel) and large changes (e.g. when initially resizing the table, or the user moves the mouse quickly) are significant.
For example, I tried to use your SpaceDistributor from #1111 and it suffered from the same problem as bypassing the small delta path (by setting SMALL_DELTA=0) - when the user resizes the columns slowly, the same column gets the pixel and grows wider than the rest.
My observation is that this algorithm seems unable to provide a proper user resizing experience as it seems to discard important information it would need to do so.
please elaborate, or point to a specific problem. It is entirely possible that a better algorithm might exist, but it might be out of scope at the moment, as this is a follow-up for a specific issue.
It's hard to point to a specific problem when most of the algorithm used would be unnecessary if it used the initial conditions + current resize position as its basis for calculating the column sizes. My problem with this implementation is that it takes what is fundamentally a very simple algorithm (columns have sizes X,Y,Z and Y is resized 10 pixels larger, what should the layout be?) and turns it into a frame rate dependent, mouse movement dependent delta calculation. The initial conditions are discarded, and so a single drag resize of 10 pixels is NOT the same as a drag resize that captured several individual steps (1 + 2 + 3 + 4 pixels), while it really should be...
On top of that, if indeed the algorithm is flawed, as I think it is, then there is no way to really fix it apart from some cosmetic changes. This then would be a lot of wasted effort. As I noted, there is no JUnit test for this code as of yet, and for such a complicated algorithm to be verified to be correct, I think it would need one to pass review. If we're willing to forego that, then I suppose a casual fix is in the cards, but I can't really see whether or not it would be correct (within its fundamental limitations) without extensive manual testing.
The
SMALL_DELTAconstant that changes behavior on how large a "drag" was registered is a red flag; this shouldn't matter. The sizes should always be based on what they were initially (at the start of the drag), and where the cursor is currently, regardless of what path the cursor took to get there (ie. there should be no memory effects, the algorithm should only need the initial sizes + current position).This is not my experience. Specifically, the difference in behavior between small changes (when the users resizes the columns slowly and we get small deltas of 1 pixel) and large changes (e.g. when initially resizing the table, or the user moves the mouse quickly) are significant.
Yes, it would be needed with this algorithm as it is dependent on mouse cursor speed and frame rate as it has no idea of what the initial positions were and how it arrived at the current state.
For example, I tried to use your SpaceDistributor from #1111 and it suffered from the same problem as bypassing the small delta path (by setting SMALL_DELTA=0) - when the user resizes the columns slowly, the same column gets the pixel and grows wider than the rest.
It would not be sufficient to just replace ResizeHelper with something that uses the space distributor as the information it needs would still be lost. When writing algorithms that resize a UI, using the current size of the elements in your UI + a resize amount will never result in a consistent looking resize. It always must be based on their size constraints (which can be min/pref/max based for controls that can't be individually resized) or the actual sizes when the action started (for splitters or columns). Once the action is finished, only then do the new sizes become the initial sizes for the next action -- not at every mouse event or frame that elapses.
HBox for example doesn't use its current sizes to calculate its "new" size -- it always goes back to the basic min/pref/max sizes, then looks at the space available and computes new sizes based on only that information. If it used the current sizes as its base, you would notice that in certain scenario's the final sizes (if you slowly vs quickly resized a window) would not necessarily always be the same for the same available width -- that's exactly what I think is happening with the column sizes; it's not deterministic and can't be because it lacks the information to do so and hence has to resort to SMALL_DELTA tricks to differentiate between "fast" and "slow" resizes. Relying on such things (which can vary with mouse hardware, current framerate, CPU/GPU load, amount of visible Nodes) makes for an inconsistent resize experience, even when the column was resized by the exact same amount of pixels when the resize ends.
This can I think be fixed relatively easily; all it really would take is to track when the drag starts, store the sizes, use these sizes to calculate new sizes each time, and when the drag ends, discard the stored sizes. The ResizeHelper would then only need a single much simpler method that takes an array of sizes, the resizing mode, the space available and which column was resized and by how much -- or if you want, you can associate the ResizeHelper with the drag resize start, and use the same helper while the resize is in progress.
Thank you for a detailed write up, @hjohn .
One thing to note: the new code is a modification of the earlier work on constrained column resize policies, that logic is unaffected. The only difference is to recognize the fact that in an over-constrained situation sone constraints must be relaxed, and that eliminates multiple passes.
Another consideration is that we are not re-writing Tree/TableView skin column resizing code. The fact that the current public APIs do not provide us with the initial condition and instead invoke the policy resize methods giving small deltas is a limitation this PR is not going to address.
@andy-goryachev-oracle
Adding additional issue to issue list: 8299755: Tree/TableView: Cursor Decouples From Divider When Resizing With Fractional Scale.
I agree with John that a layout algorithm that uses incremental calculations will always be flawed in principle. The correct approach is to store the initial configuration, and then for each configuration change, go back to the initial configuration and recompute the layout solution.
Now, we might still accept a bugfix for a flawed algorithm. But JDK-8299753 is an enhancement, not a bugfix. I'm not sure what to make of this: it's obviously a flawed approach, and basing an enhancement on a flawed approach means that someone would have to come back to this issue in the future and solve it correctly.
I don't think that the issue at hand is so severe that it's a forced move to integrate this interim solution.
I respectfully disagree, @mstr2 .
This fix is not an interim solution - unlike any theoretical considerations of "flawed approach", in practice this code does work as expected with integer and fractional scales, and, given our situation, it's highly unlikely that any alternative solutions will ever be considered, especially those that would affect existing public APIs.
I respectfully disagree, @mstr2 .
This fix is not an interim solution - unlike any theoretical considerations of "flawed approach", in practice this code does work as expected with integer and fractional scales, and, given our situation, it's highly unlikely that any alternative solutions will ever be considered, especially those that would affect existing public APIs.
It isn't just a theoretical issue. The proposed patch fails to keep the divider precisely at the cursor location, depending on frame rate and mouse movement speed. This is how the behavior manifests on my machine:
https://github.com/openjdk/jfx/assets/43553916/79cf04be-bd18-4cfe-9c34-912978ee96ee
Thank you for the video, @mstr2
This is an intrinsic problem with the Tree/TableView - you can observe it even with the UNCONSTRAINED_RESIZE_POLICY. You both are right in that it's impossible to fix this without remembering the initial MOUSE_PRESSED position. I wonder if it's possible to fix it without making API changes.
However, fixing this is out of scope for this PR. This PR deals with correctly handling of fractional scale (within the constraints of the current resize policy API) and cursor decoupling even when moving very slowly.
@andy-goryachev-oracle This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
⚠️ @andy-goryachev-oracle This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).
@andy-goryachev-oracle This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
@andy-goryachev-oracle This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
@karthikpandelu @hjohn @kevinrushforth could you please review?
Tested the changes in Windows 11 with different scale values. The fix works as expected. I will review the code soon and complete the review.
@andy-goryachev-oracle This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
@andy-goryachev-oracle This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.
❗ This change is not yet ready to be integrated. See the Progress checklist in the description for automated requirements.
@andy-goryachev-oracle This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!