jfx
jfx copied to clipboard
openjdk
8089009: TableView with CONSTRAINED_RESIZE_POLICY incorrectly displays a horizontal scroll bar.
Issue:
When the TableView is set with built-in CONSTRAINED_RESIZE_POLICY, the horizontal scroll bar keeps flickering when the TableView width is reduced.
Cause:
The table columns widths are recalculated only if the difference of the TableView width and the total columns width is greater than 1px. Because of this improper calculations, if the TableView width is reduced by 1px, the columns widths are not updated. Which results to computing for horizontal scroll bar visibility in VirtualFlow to improper results and let the horizontal scroll bar to be visible.
Where as if the TableView width is reduced by more than 1px, the calculations are done correctly and the horizontal scroll bar visibility is turned off. Because of this behaviour, it looks like the horizontal scroll bar is flickering when the TableView width is reduced (let’s say by dragging).
To confirm this behaviour, please find the below example that showcases the issue: When the tableView width is reduced/increased by 1px, the column widths are recalculated only after every alternate 1px change. Whereas if the tableView width is reduced/increased by more than 1px (say 10px), the column widths are calculated correctly.
import javafx.application.Application;
import javafx.beans.property.SimpleStringProperty;
import javafx.beans.property.StringProperty;
import javafx.collections.FXCollections;
import javafx.collections.ObservableList;
import javafx.geometry.Insets;
import javafx.scene.Group;
import javafx.scene.Scene;
import javafx.scene.control.*;
import javafx.scene.layout.HBox;
import javafx.scene.layout.VBox;
import javafx.stage.Stage;
public class ConstrainedResizePolicyIssueDemo extends Application {
@Override
public void start(Stage primaryStage) throws Exception {
TableColumn<Person, String> fnCol = new TableColumn<>("First Name");
fnCol.setMinWidth(100);
fnCol.setCellValueFactory(param -> param.getValue().firstNameProperty());
TableColumn<Person, String> priceCol = new TableColumn<>("Price");
priceCol.setMinWidth(100);
priceCol.setMaxWidth(150);
priceCol.setCellValueFactory(param -> param.getValue().priceProperty());
TableColumn<Person, String> totalCol = new TableColumn<>("Total");
totalCol.setMinWidth(100);
totalCol.setMaxWidth(150);
totalCol.setCellValueFactory(param -> param.getValue().totalProperty());
ObservableList<Person> persons = FXCollections.observableArrayList();
persons.add(new Person("Harry", "200.00", "210.00"));
persons.add(new Person("Jacob", "260.00", "260.00"));
TableView<Person> tableView = new TableView<>();
tableView.getColumns().addAll(fnCol, priceCol, totalCol);
tableView.setItems(persons);
tableView.setColumnResizePolicy(TableView.CONSTRAINED_RESIZE_POLICY);
tableView.setMinWidth(500);
tableView.maxWidthProperty().bind(tableView.minWidthProperty());
Button button1 = new Button("Reduce 1px");
button1.setOnAction(e -> tableView.setMinWidth(tableView.getMinWidth() - 1));
Button button2 = new Button("Reduce 10px");
button2.setOnAction(e -> tableView.setMinWidth(tableView.getMinWidth() - 10));
Button button3 = new Button("Reset");
button3.setOnAction(e -> tableView.setMinWidth(500));
Button button4 = new Button("Increase 1px");
button4.setOnAction(e -> tableView.setMinWidth(tableView.getMinWidth() + 1));
Button button5 = new Button("Increase 10px");
button5.setOnAction(e -> tableView.setMinWidth(tableView.getMinWidth() + 10));
HBox row = new HBox(button1, button2, button3, button4, button5);
row.setSpacing(10);
TextArea output = new TextArea();
addWidthListeners(tableView, output);
VBox root = new VBox(row, new Group(tableView), output);
root.setSpacing(10);
root.setPadding(new Insets(10));
Scene scene = new Scene(root);
primaryStage.setScene(scene);
primaryStage.setTitle("Constrained Resize Policy Issue TableView");
primaryStage.show();
}
private void addWidthListeners(TableView<Person> tableView, TextArea output) {
tableView.widthProperty().addListener((obs, old, val) -> {
String str = "Table width changed :: " + val + "\n";
output.setText(output.getText() + str);
output.positionCaret(output.getText().length());
});
tableView.getColumns().forEach(column -> {
column.widthProperty().addListener((obs, old, width) -> {
String str = " ---> " + column.getText() + " : width changed to : " + column.getWidth()+"\n";
output.setText(output.getText() + str);
output.positionCaret(output.getText().length());
});
});
}
class Person {
private StringProperty firstName = new SimpleStringProperty();
private StringProperty price = new SimpleStringProperty();
private StringProperty total = new SimpleStringProperty();
public Person(String fn, String price, String total) {
setFirstName(fn);
setPrice(price);
setTotal(total);
}
public StringProperty firstNameProperty() {
return firstName;
}
public void setFirstName(String firstName) {
this.firstName.set(firstName);
}
public StringProperty priceProperty() {
return price;
}
public void setPrice(String price) {
this.price.set(price);
}
public StringProperty totalProperty() {
return total;
}
public void setTotal(String total) {
this.total.set(total);
}
}
}
Fix: On investigating the code, it is noticed that there is an explicit line of code to check if the difference in widths is greater than 1px. I think this should be greater than 0px. Because we need to recompute the columns widths even if the width is increased by 1px.
The part of the code that need to be updated is in TableUtil.java -> constrainedResize(..) method -> line 226
if (Math.abs(colWidth - tableWidth) > 1) {
If I update the value 1 to 0, the issue is fixed and the horizontal scroll bar visibility is computed correctly.
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)
Issue
- JDK-8089009: TableView with CONSTRAINED_RESIZE_POLICY incorrectly displays a horizontal scroll bar.
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx pull/848/head:pull/848
$ git checkout pull/848
Update a local copy of the PR:
$ git checkout pull/848
$ git pull https://git.openjdk.org/jfx pull/848/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 848
View PR using the GUI difftool:
$ git pr show -t 848
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/848.diff
Hi @SaiPradeepDandem, welcome to this OpenJDK project and thanks for contributing!
We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.
If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user SaiPradeepDandem" as summary for the issue.
If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
Webrevs
- 06: Full - Incremental (a9a689e0)
- 05: Full - Incremental (b066026a)
- 04: Full - Incremental (37c78a4b)
- 03: Full - Incremental (330e0368)
- 02: Full - Incremental (03f484f5)
- 01: Full - Incremental (b2e1c2bc)
- 00: Full (c5942695)
Two comments:
- when doing a floating point arithmetic (which is imprecise) it is possible to get a value which is close, but not exactly the same as expected result. In other words (colWidth - tableWidth) may be close to 0, but != 0. Should we take this into account?
- With TableView.CONSTRAINED_RESIZE_POLICY set, the application developer expects to see the horizontal scroll bar NEVER. Is this the only place where h.s.b. gets made visible?
@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).
@SaiPradeepDandem Can you enable GitHub actions runs for your personal fork? This will allow us to verify the status of new and existing tests more easily. To do this, go to the Actions tab of your personal fork and press the big green button to enable GHA workflows.
@andy-goryachev-oracle Below are my comments for the points you raised:
- I agree with this and upon inspecting the current code, I noticed that this was already considered for some other bounds checking within the same method
TableUtil.java->constrainedResize(...).
// Check for zero. This happens when the distribution of the delta
// finishes early due to a series of "fixed" entries at the end.
// In this case, lowerBound == upperBound, for all subsequent terms.
double newSize;
if (Math.abs(totalLowerBound - totalUpperBound) < .0000001) {
newSize = lowerBound;
} else {
double f = (target - totalLowerBound) / (totalUpperBound - totalLowerBound);
newSize = Math.round(lowerBound + f * (upperBound - lowerBound));
}
Will it be ok if I create a variable named double EPSILON=.0000001 and use this in both the places?
- I am not sure about setting TableView.CONSTRAINED_RESIZE_POLICY means to always hide the horizontal scroll bar. The most common use case with the CONSTRAINED_RESIZE_POLICY, is to auto stretch the columns if there is enough space, and set to the minimum widths and show horizontal scroll bar if there is not enough space. I included an example in this stackoverflow question and the gif show-casing the requirement (2nd gif). And the current code already handles this very well.
For reference, there is already an existing JDK ticket JDK-8089280 and a bit of discussion over this.
I am not sure about setting TableView.CONSTRAINED_RESIZE_POLICY means to always hide the horizontal scroll bar. The most common use case with the CONSTRAINED_RESIZE_POLICY, is to auto stretch the columns if there is enough space, and set to the minimum widths and show horizontal scroll bar if there is not enough space. I included an example in this stackoverflow question and the gif show-casing the requirement (2nd gif). And the current code already handles this very well.
Quite the opposite. As an app developer, I've used unrestrained policy (in swing) exactly once, all other cases used fit-to-width.
Even if you look at stackoverflow and even JDK-8089280 it is clear that the app developers do not want to see the horizontal scroll bar with the constrained policy set, ever. This was one of the pain points with javafx coming from swing.
In swing, JTable offers a rich set of possibilities:
` /** Do not adjust column widths automatically; use a horizontal scrollbar instead. */ public static final int AUTO_RESIZE_OFF = 0;
/** When a column is adjusted in the UI, adjust the next column the opposite way. */
public static final int AUTO_RESIZE_NEXT_COLUMN = 1;
/** During UI adjustment, change subsequent columns to preserve the total width;
* this is the default behavior. */
public static final int AUTO_RESIZE_SUBSEQUENT_COLUMNS = 2;
/** During all resize operations, apply adjustments to the last column only. */
public static final int AUTO_RESIZE_LAST_COLUMN = 3;
/** During all resize operations, proportionately resize all columns. */
public static final int AUTO_RESIZE_ALL_COLUMNS = 4;`
In javafx, behavior of TableView.CONSTRAINED_RESIZE_POLICY is, permit me to say, simply incorrect. The scope of this discussion may go beyond the scope of this PR, but the fact that we have a number of JDK bugs logged against it is an indication.
@andy-goryachev-oracle Thanks for providing the info about constrained resize policy in Swing. Unfornately I didn't get a chance to work with Swing, so I am not fully aware of this functionality that existed in Swing. At this stage in JavaFX,the need with scroll bar visibility comes into picture when working with resizable windows. May be as mentioned in one of the comments in JDK-8089280, when constrained resize policy is set, it needs to set/override the minWidth of the table view to total minWidths of all columns. And as you said, this might be beyond the scope of this PR.
With the EPSILON change the horizontal bar disappears for good! Tried with setSnapToPixel(false) on TableView, table columns, split pane.
@SaiPradeepDandem I might have accidentally reassigned JDK-8089009 to myself, you can grab it back if it was yours (for a full credit!).
This change might also fix JDK-8089280 (or JDK-8089280 should be closed as a duplicate).
:warning: @SaiPradeepDandem the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:
$ git checkout 8089009_tableview_constrained_scrollbar
$ git commit --author='Preferred Full Name <[email protected]>' --allow-empty -m 'Update full name'
$ git push
@SaiPradeepDandem This change now passes all automated pre-integration checks.
ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.
After integration, the commit message for the final commit will be:
8089009: TableView with CONSTRAINED_RESIZE_POLICY incorrectly displays a horizontal scroll bar.
Reviewed-by: kcr, angorya, aghaisas
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.
At the time when this comment was updated there had been 21 new commits pushed to the master branch:
- 88159811239b76399b8f90c6c648293b4a06528c: 8271395: Crash during printing when disposing textures
- cedc17cc26bb0ad3e2d0693bd174f118ad5f0b82: 8292549: GitHub actions: intermittent build failure on macOS while downloading ant
- 5c47295cf7ff9404b3bddaa2c30772fa2eff461c: 8290473: update Eclipse .classpath in apps, buildSrc
- 2618bf8aeef3c9d9d923576e8a610f5e9b2123f1: 8181084: JavaFX show big icons in system menu on macOS with Retina display
- eaddb0fbeeb99900636f9704758f6c004860ff9a: 8291908: VirtualFlow creates unneeded empty cells
- 33534cb9e0524e91ed717b22fab667b284c52252: 8291630: Update attribution in webkit.md file
- 38324a70c3054e75cb22865e7ffcc5375b62939d: 8291087: Wrong position of focus of screen reader on Windows with screen scale > 1
- b4d86bdffc2dd89e3473884b4079092e7e2843d1: 8218826: TableRowSkinBase: horizontal layout broken if row has padding
- 4c1f4c20b99dfc8c8016c305983dfea5e1b9d6f2: Merge
- dd30402a5c22daf79b88dd35b42f32d0a0e28867: 8291589: Update copyright header for files modified in 2022
- ... and 11 more: https://git.openjdk.org/jfx/compare/9f636da27576b69f797d923599bd4ae4b4d37be8...master
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.
As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kevinrushforth, @aghaisas) but any other Committer may sponsor as well.
➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).
@SaiPradeepDandem This is now ready to be integrated. The next step is for you to issue the /integrate command, and then Ajit or I will sponsor it.
@SaiPradeepDandem Your change (at version a9a689e07f1d43eebacbbbe8b7e23318946e225f) is now ready to be sponsored by a Committer.
Going to push as commit d4ff45af9fbde29f96d80681c7fc3af3ec580d1a.
Since your change was applied there have been 21 commits pushed to the master branch:
- 88159811239b76399b8f90c6c648293b4a06528c: 8271395: Crash during printing when disposing textures
- cedc17cc26bb0ad3e2d0693bd174f118ad5f0b82: 8292549: GitHub actions: intermittent build failure on macOS while downloading ant
- 5c47295cf7ff9404b3bddaa2c30772fa2eff461c: 8290473: update Eclipse .classpath in apps, buildSrc
- 2618bf8aeef3c9d9d923576e8a610f5e9b2123f1: 8181084: JavaFX show big icons in system menu on macOS with Retina display
- eaddb0fbeeb99900636f9704758f6c004860ff9a: 8291908: VirtualFlow creates unneeded empty cells
- 33534cb9e0524e91ed717b22fab667b284c52252: 8291630: Update attribution in webkit.md file
- 38324a70c3054e75cb22865e7ffcc5375b62939d: 8291087: Wrong position of focus of screen reader on Windows with screen scale > 1
- b4d86bdffc2dd89e3473884b4079092e7e2843d1: 8218826: TableRowSkinBase: horizontal layout broken if row has padding
- 4c1f4c20b99dfc8c8016c305983dfea5e1b9d6f2: Merge
- dd30402a5c22daf79b88dd35b42f32d0a0e28867: 8291589: Update copyright header for files modified in 2022
- ... and 11 more: https://git.openjdk.org/jfx/compare/9f636da27576b69f797d923599bd4ae4b4d37be8...master
Your commit was automatically rebased without conflicts.
@aghaisas @SaiPradeepDandem Pushed as commit d4ff45af9fbde29f96d80681c7fc3af3ec580d1a.
:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.