Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling
On Tue, 19 Apr 2022 09:57:58 GMT, Marius Hanl wrote: >>> @effad Do you have time and interest to take a look at this for the >>> `TreeTableView` as well? Just asking as otherwise I will have a look :) >> >> Sorry for late response (extended easter holidays ...). Yes I could also >> take a look at this for TreeTableView. Is there a pre-existing issue for >> this? Otherwise I'll try and come up with an example and create a new >> issue... > >> Is there a pre-existing issue for this? Otherwise I'll try and come up with >> an example and create a new issue... > > I don't think so, you may need to create a new issue. I think it's pretty > much the same problem and solution as this ticket was, so you can probably > copy some information. @Maran23 https://bugs.openjdk.java.net/browse/JDK-8285197 created. - PR: https://git.openjdk.java.net/jfx/pull/757
Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling
On Tue, 19 Apr 2022 06:06:25 GMT, Robert Lichtenberger wrote: > Is there a pre-existing issue for this? Otherwise I'll try and come up with > an example and create a new issue... I don't think so, you may need to create a new issue. I think it's pretty much the same problem and solution as this ticket was, so you can probably copy some information. - PR: https://git.openjdk.java.net/jfx/pull/757
Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling
On Wed, 16 Mar 2022 13:01:45 GMT, Robert Lichtenberger wrote: >> Might make sense to also adjust the TreeTableView sizing implementation? > >> Might make sense to also adjust the TreeTableView sizing implementation? > > Yes I think that may be a good idea. True to the idea of specific, narrow > commits I've not integrated this into this PR but would be willing to open a > new issue / produce a separate PR for TreeTableView. > @effad Do you have time and interest to take a look at this for the > `TreeTableView` as well? Just asking as otherwise I will have a look :) Sorry for late response (extended easter holidays ...). Yes I could also take a look at this for TreeTableView. Is there a pre-existing issue for this? Otherwise I'll try and come up with an example and create a new issue... - PR: https://git.openjdk.java.net/jfx/pull/757
Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling
On Wed, 16 Mar 2022 13:01:45 GMT, Robert Lichtenberger wrote: >> Might make sense to also adjust the TreeTableView sizing implementation? > >> Might make sense to also adjust the TreeTableView sizing implementation? > > Yes I think that may be a good idea. True to the idea of specific, narrow > commits I've not integrated this into this PR but would be willing to open a > new issue / produce a separate PR for TreeTableView. @effad Do you have time and interest to take a look at this for the `TreeTableView` as well? Just asking as otherwise I will have a look :) - PR: https://git.openjdk.java.net/jfx/pull/757
Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling [v4]
On Wed, 30 Mar 2022 12:22:23 GMT, Robert Lichtenberger wrote: >> This fix respects a row factory, if present. >> It will put the cell that is used to measure the column width as child below >> the row. >> In that way the row's style will be used. > > Robert Lichtenberger has updated the pull request incrementally with one > additional commit since the last revision: > > 8251480: TableColumnHeader: calc of cell width must respect row styling > > Assertion removed. Looks good to me. - Marked as reviewed by aghaisas (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/757
Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling [v4]
On Wed, 30 Mar 2022 12:22:23 GMT, Robert Lichtenberger wrote: >> This fix respects a row factory, if present. >> It will put the cell that is used to measure the column width as child below >> the row. >> In that way the row's style will be used. > > Robert Lichtenberger has updated the pull request incrementally with one > additional commit since the last revision: > > 8251480: TableColumnHeader: calc of cell width must respect row styling > > Assertion removed. Looks good to me. - Marked as reviewed by mhanl (Author). PR: https://git.openjdk.java.net/jfx/pull/757
Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling [v4]
On Wed, 30 Mar 2022 12:22:23 GMT, Robert Lichtenberger wrote: >> This fix respects a row factory, if present. >> It will put the cell that is used to measure the column width as child below >> the row. >> In that way the row's style will be used. > > Robert Lichtenberger has updated the pull request incrementally with one > additional commit since the last revision: > > 8251480: TableColumnHeader: calc of cell width must respect row styling > > Assertion removed. In looking at the fix, I'd like two reviewers. - PR: https://git.openjdk.java.net/jfx/pull/757
Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling [v4]
> This fix respects a row factory, if present. > It will put the cell that is used to measure the column width as child below > the row. > In that way the row's style will be used. Robert Lichtenberger has updated the pull request incrementally with one additional commit since the last revision: 8251480: TableColumnHeader: calc of cell width must respect row styling Assertion removed. - Changes: - all: https://git.openjdk.java.net/jfx/pull/757/files - new: https://git.openjdk.java.net/jfx/pull/757/files/960e5012..f463ab09 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=757=03 - incr: https://webrevs.openjdk.java.net/?repo=jfx=757=02-03 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/757.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/757/head:pull/757 PR: https://git.openjdk.java.net/jfx/pull/757
Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling [v2]
On Wed, 30 Mar 2022 12:10:19 GMT, Kevin Rushforth wrote: >> I found assertions in various other parts of the code (e.g. >> `javafx.scene.control.skin.VirtualFlow`) so I assumed this is ok. > > Some old code has assertions, but library code should not use them. Please > delete them from any new / modified code. Deleted the assertion. - PR: https://git.openjdk.java.net/jfx/pull/757
Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling [v2]
On Wed, 30 Mar 2022 10:57:38 GMT, Robert Lichtenberger wrote: >> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableColumnHeader.java >> line 715: >> >>> 713: tableRow = createMeasureRow(tv, tableSkin, null); >>> 714: } >>> 715: assert tableRow.getSkin() instanceof SkinBase; >> >> Not sure if there is some convention about code asserts, maybe some else can >> tell. At least I never saw one in a PR here. > > I found assertions in various other parts of the code (e.g. > `javafx.scene.control.skin.VirtualFlow`) so I assumed this is ok. Some old code has assertions, but library code should not use them. Please delete them from any new / modified code. - PR: https://git.openjdk.java.net/jfx/pull/757
Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling [v3]
> This fix respects a row factory, if present. > It will put the cell that is used to measure the column width as child below > the row. > In that way the row's style will be used. Robert Lichtenberger has updated the pull request incrementally with one additional commit since the last revision: 8251480: TableColumnHeader: calc of cell width must respect row styling Test readability improved. - Changes: - all: https://git.openjdk.java.net/jfx/pull/757/files - new: https://git.openjdk.java.net/jfx/pull/757/files/af582570..960e5012 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=757=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx=757=01-02 Stats: 13 lines in 1 file changed: 6 ins; 6 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/757.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/757/head:pull/757 PR: https://git.openjdk.java.net/jfx/pull/757
Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling [v2]
On Tue, 29 Mar 2022 20:35:10 GMT, Marius Hanl wrote:
>> Robert Lichtenberger has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8251480: TableColumnHeader: calc of cell width must respect row styling
>>
>> Old behaviour restored for table rows with custom skins.
>> Unnecessary call to cell.applyCss() removed.
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableColumnHeader.java
> line 715:
>
>> 713: tableRow = createMeasureRow(tv, tableSkin, null);
>> 714: }
>> 715: assert tableRow.getSkin() instanceof SkinBase;
>
> Not sure if there is some convention about code asserts, maybe some else can
> tell. At least I never saw one in a PR here.
I found assertions in various other parts of the code (e.g.
`javafx.scene.control.skin.VirtualFlow`) so I assumed this is ok.
> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
> line 276:
>
>> 274: }
>> 275:
>> 276: private TableRow createSmallRow(TableView
>> tableView) {
>
> Minor: Move this method to the other row method below?
You're right. I'll move the method.
> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
> line 295:
>
>> 293: private TableRow createCustomRow(TableView
>> tableView) {
>> 294: TableRow row = new TableRow<>() {
>> 295: protected javafx.scene.control.Skin createDefaultSkin() {
>
> Very minor: `javafx.scene.control.` is not needed
You're right. I'll remove this.
-
PR: https://git.openjdk.java.net/jfx/pull/757
Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling [v2]
On Thu, 24 Mar 2022 06:14:42 GMT, Robert Lichtenberger
wrote:
>> This fix respects a row factory, if present.
>> It will put the cell that is used to measure the column width as child below
>> the row.
>> In that way the row's style will be used.
>
> Robert Lichtenberger has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8251480: TableColumnHeader: calc of cell width must respect row styling
>
> Old behaviour restored for table rows with custom skins.
> Unnecessary call to cell.applyCss() removed.
Just commented some minor things.
But overall it looks good.
I did some manual tests as well and can confirm that this works. :)
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableColumnHeader.java
line 715:
> 713: tableRow = createMeasureRow(tv, tableSkin, null);
> 714: }
> 715: assert tableRow.getSkin() instanceof SkinBase;
Not sure if there is some convention about code asserts, maybe some else can
tell. At least I never saw one in a PR here.
modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
line 276:
> 274: }
> 275:
> 276: private TableRow createSmallRow(TableView tableView)
> {
Minor: Move this method to the other row method below?
modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
line 295:
> 293: private TableRow createCustomRow(TableView
> tableView) {
> 294: TableRow row = new TableRow<>() {
> 295: protected javafx.scene.control.Skin createDefaultSkin() {
Very minor: `javafx.scene.control.` is not needed
-
Marked as reviewed by mhanl (Author).
PR: https://git.openjdk.java.net/jfx/pull/757
Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling [v2]
On Thu, 24 Mar 2022 04:57:27 GMT, Robert Lichtenberger wrote: >> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableColumnHeader.java >> line 650: >> >>> 648: } >>> 649: Callback, TableRow> rowFactory = >>> tv.getRowFactory(); >>> 650: TableRow tableRow = rowFactory != null ? >>> rowFactory.call(tv) : new TableRow<>(); >> >> When there is no row factory, we probably should just return like in line >> 632-633? > > Unlike cell factory, which always has to be present, the row factory is > optional and in fact most tables will not have a row factory. If we return in > that case, the algorithm will no longer work for these tables. > Compare with `javafx.scene.control.skin.TableViewSkin.createCell()`. You are right, thanks. - PR: https://git.openjdk.java.net/jfx/pull/757
Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling [v2]
> This fix respects a row factory, if present. > It will put the cell that is used to measure the column width as child below > the row. > In that way the row's style will be used. Robert Lichtenberger has updated the pull request incrementally with one additional commit since the last revision: 8251480: TableColumnHeader: calc of cell width must respect row styling Old behaviour restored for table rows with custom skins. Unnecessary call to cell.applyCss() removed. - Changes: - all: https://git.openjdk.java.net/jfx/pull/757/files - new: https://git.openjdk.java.net/jfx/pull/757/files/08851726..af582570 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=757=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=757=00-01 Stats: 74 lines in 2 files changed: 65 ins; 6 del; 3 mod Patch: https://git.openjdk.java.net/jfx/pull/757.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/757/head:pull/757 PR: https://git.openjdk.java.net/jfx/pull/757
Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling
On Wed, 23 Mar 2022 08:15:47 GMT, Marius Hanl wrote:
>> This fix respects a row factory, if present.
>> It will put the cell that is used to measure the column width as child below
>> the row.
>> In that way the row's style will be used.
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableColumnHeader.java
> line 667:
>
>> 665: if ((cell.getText() != null && !cell.getText().isEmpty())
>> || cell.getGraphic() != null) {
>> 666: tableRow.applyCss();
>> 667: cell.applyCss();
>
> Just wondering: Is `cell.applyCss();` still needed here?
Right, this is no longer needed. I'll remove it.
-
PR: https://git.openjdk.java.net/jfx/pull/757
Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling
On Wed, 23 Mar 2022 08:17:38 GMT, Marius Hanl wrote: >> This fix respects a row factory, if present. >> It will put the cell that is used to measure the column width as child below >> the row. >> In that way the row's style will be used. > > modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableColumnHeader.java > line 653: > >> 651: tableSkin.getChildren().add(tableRow); >> 652: tableRow.applyCss(); >> 653: ((SkinBase) tableRow.getSkin()).getChildren().add(cell); > > I don't think this is a safe cast. Instead, you probably should do an > `instanceof SkinBase` check before You're right. A tablerow may have been created that returns a custom skin which is not necessarily derived from SkinBase. However, since we do not have a getChildren() method in such a custom skin we will be unable to proceed. We could now abandon altogether (another return) but that mean that resizeColumnToFitContent no longer works at all in the presence of custom rows. So I think the best solution in that case would be to ignore the rowFactory and use a default `new TableRow<>` again to preserve at least the old behaviour in such cases. I'll try and improve the patch. - PR: https://git.openjdk.java.net/jfx/pull/757
Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling
On Wed, 23 Mar 2022 08:19:41 GMT, Marius Hanl wrote: >> This fix respects a row factory, if present. >> It will put the cell that is used to measure the column width as child below >> the row. >> In that way the row's style will be used. > > modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableColumnHeader.java > line 650: > >> 648: } >> 649: Callback, TableRow> rowFactory = >> tv.getRowFactory(); >> 650: TableRow tableRow = rowFactory != null ? rowFactory.call(tv) >> : new TableRow<>(); > > When there is no row factory, we probably should just return like in line > 632-633? Unlike cell factory, which always has to be present, the row factory is optional and in fact most tables will not have a row factory. If we return in that case, the algorithm will no longer work for these tables. Compare with `javafx.scene.control.skin.TableViewSkin.createCell()`. - PR: https://git.openjdk.java.net/jfx/pull/757
Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling
On Wed, 16 Mar 2022 08:20:59 GMT, Robert Lichtenberger
wrote:
> This fix respects a row factory, if present.
> It will put the cell that is used to measure the column width as child below
> the row.
> In that way the row's style will be used.
The approach looks good. I left some comments and questions
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableColumnHeader.java
line 650:
> 648: }
> 649: Callback, TableRow> rowFactory =
> tv.getRowFactory();
> 650: TableRow tableRow = rowFactory != null ? rowFactory.call(tv)
> : new TableRow<>();
When there is no row factory, we probably should just return like in line
632-633?
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableColumnHeader.java
line 653:
> 651: tableSkin.getChildren().add(tableRow);
> 652: tableRow.applyCss();
> 653: ((SkinBase) tableRow.getSkin()).getChildren().add(cell);
I don't think this is a safe cast. Instead, you probably should do an
`instanceof SkinBase` check before
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableColumnHeader.java
line 667:
> 665: if ((cell.getText() != null && !cell.getText().isEmpty()) ||
> cell.getGraphic() != null) {
> 666: tableRow.applyCss();
> 667: cell.applyCss();
Just wondering: Is `cell.applyCss();` still needed here?
modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
line 259:
> 257: /**
> 258: * @test
> 259: * @bug 8251480 Row style must affect the required column width
This annotations are not needed, although they do not hurt (just a note)
-
Changes requested by mhanl (Author).
PR: https://git.openjdk.java.net/jfx/pull/757
Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling
On Wed, 16 Mar 2022 11:46:07 GMT, Marius Hanl wrote: > Might make sense to also adjust the TreeTableView sizing implementation? Yes I think that may be a good idea. True to the idea of specific, narrow commits I've not integrated this into this PR but would be willing to open a new issue / produce a separate PR for TreeTableView. - PR: https://git.openjdk.java.net/jfx/pull/757
Re: RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling
On Wed, 16 Mar 2022 08:20:59 GMT, Robert Lichtenberger wrote: > This fix respects a row factory, if present. > It will put the cell that is used to measure the column width as child below > the row. > In that way the row's style will be used. Might make sense to also adjust the TreeTableView sizing implementation? - PR: https://git.openjdk.java.net/jfx/pull/757
RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling
This fix respects a row factory, if present. It will put the cell that is used to measure the column width as child below the row. In that way the row's style will be used. - Commit messages: - 8251480: TableColumnHeader: calc of cell width must respect row styling Changes: https://git.openjdk.java.net/jfx/pull/757/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=757=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8251480 Stats: 38 lines in 2 files changed: 34 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jfx/pull/757.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/757/head:pull/757 PR: https://git.openjdk.java.net/jfx/pull/757
