8299753: Tree/TableView: Column Resizing With Fractional Scale #1156
Conversation
|
👋 Welcome back angorya! A progress list of the required criteria for merging this PR into |
Webrevs
|
|
/reviewers 2 |
|
@kevinrushforth |
| if (delta < 0) { | ||
| while (delta < -halfPixel) { | ||
| double d = -pixel; | ||
| if(smallShrink(d, desired)) { |
There was a problem hiding this comment.
Minor: Add space after if -> if (
| } else { | ||
| while (delta > halfPixel) { | ||
| double d = pixel; | ||
| if(smallGrow(d, desired)) { |
There was a problem hiding this comment.
Add space after if -> if (
|
fixed, thanks! |
|
@hjohn would you mind taking a look at this? using the ideas from your SpaceDistributor |
| } else { | ||
| distribute(delta, pref); | ||
| } | ||
| } else if (delta > 0.0) { |
There was a problem hiding this comment.
Do we need delta = 0.0 condition here?
There was a problem hiding this comment.
excellent question. we don't - zero delta means no change, so no action is needed.
Will take a look this weekend |
There was a problem hiding this comment.
I only reviewed this partially.
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. The ResizeHelper is 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). The SMALL_DELTA constant 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).
I'd expect:
- User starts a resize
- All sizes and positions are stored -- this may be a good time to create something like the
ResizeHelper. ThisResizeHelpershould never modify the initial sizes, as they're needed for each new calculation until the drag ends. - User drags the cursor all over the place
- Depending on where the cursor is + the initial stored values, a new set of column sizes is calculated and displayed; the algorithm should not have "memory" effects of non-final column sizes of positions where the cursor was before
- When the drag ends, the final position is exactly what would have happened if the drag was a single instant move, in other words, moving the cursor from
0 -> 100should be the same as if it was moved from0 -> -10 -> 90 -> 200 -> 100
I haven't checked completely how the column resizing works, but I don't see how this could possible be a good algorithm currently. So I'm left wondering what value this change then brings as I think these changes would more than likely all be discarded once the UX is implemented correctly.
A direct JUnit test on this complicated code that verifies all the edge cases would be something I'd like to see added as a minimum, but looking at the code, I don't see this being a good algorithm at all...
| double cmin = c.getMinWidth(); | ||
| double cmax = c.getMaxWidth(); | ||
| double cmin = snapCeil(c.getMinWidth()); // always honor min width! | ||
| double cmax = snapFloor(c.getMaxWidth()); // TableColumnBase.doSetWidth() clamps to (min,max) range | ||
| min[i] = cmin; |
There was a problem hiding this comment.
Looking at doSetWidth I see that it clamps using unsnapped values, so the column can still be given an unsnapped size. When scale is 1.0, and the column for example has its min/max width set to 20.1 and 20.9, then snapCeil is 21 and snapFloor is 20 (so maximum is less than minimum, which may already be a bit dubious). When doSetWidth is called it will be clamped, resulting in 20.1 or 20.9.
There was a problem hiding this comment.
For some reason, I've decided to leave that as is, but since you pointed it out, I think it ought to be fixed.
I am going to implement proper snapping logic in ResizeFeaturesBase.setColumnWidth() using private APIs, with the intent to eventually convert them to public APIs with https://bugs.openjdk.org/browse/JDK-8311527
| @@ -56,7 +58,7 @@ public ResizeHelper(ResizeFeaturesBase rf, | |||
| this.snap = (rf.getTableControl().isSnapToPixel() ? rf.getTableControl() : null); | |||
There was a problem hiding this comment.
Obtaining the Region here is a bit hacky, this may be out of scope, but I would say snap should be a boolean, and the snapXXX helper methods in this class should call ScaledMath.
There was a problem hiding this comment.
Here i would disagree. I specifically do not want to replicate the snap code in the Region, since the Region publishes its snapping API. If there is a change in the snapping APIs in the Region, that change would need to be replicated again here, which I think is a bad idea.
| double threshold = snapRound(SMALL_DELTA); | ||
| if (Math.abs(delta) > threshold) { |
There was a problem hiding this comment.
The threshold is pretty arbitrary, I don't think it benefits from snap rounding here. I'd be more inclined to eliminate the difference between these two functions; I would expect that if a user drags a column resizer for 100 pixels, that it should make no difference whether that was a slow drag or a fast one, and the end visuals and column sizes should be exactly the same regardless.
There was a problem hiding this comment.
theoretically, may be. I could not achieve it with the algorithm that distributes space for large deltas, as it breaks down with small deltas, see my previous comment.
snapRound(SMALL_DELTA) is indeed unnecessary.
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.
convenience - it does have some data, and it's easier to keep code along with the data.
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. |
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.
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.
It would not be sufficient to just replace
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 |
|
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. |
|
/issue JDK-8299755 |
|
@andy-goryachev-oracle |
|
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. |
|
@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! |
|
please 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! |
|
@karthikpandelu @hjohn @kevinrushforth could you please review? |
|
Tested the changes in Windows 11 with different scale values. The fix works as expected. |
|
@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 |
|
@andy-goryachev-oracle This pull request is now open |
|
❗ This change is not yet ready to be integrated. |
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
Issues
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1156/head:pull/1156$ git checkout pull/1156Update a local copy of the PR:
$ git checkout pull/1156$ git pull https://git.openjdk.org/jfx.git pull/1156/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1156View PR using the GUI difftool:
$ git pr show -t 1156Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1156.diff
Webrev
Link to Webrev Comment