RFR: 8311031: JTable header border vertical lines are not aligned with data grid lines [v10]

[Alexey Ivanov]

On Mon, 31 Jul 2023 16:31:21 GMT, Tejesh R <tr at openjdk.org> wrote:

>> The header border uses `g.drawLine` whereas the JTable data grid lines uses `SwingUtilities2.drawVLine` and `SwingUtilities2.drawHLine` to draw horizontal and vertical lines. The SwingUtilities2 uses `Graphics.fillRect` which contributes to the difference between the position of these two lines which happens/visible at higher ui scaling (difference in alignment between vertical lines of these two). The fix propose to use the same methods for metal L&F of JTable header border paint. 
>> CI testing shows green.
>> 
>> ![image](https://github.com/openjdk/jdk/assets/94159358/f6d1d822-55ba-4ad3-9914-d3f68b67a6c5)
>
> Tejesh R has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Review fix

Changes requested by aivanov (Reviewer).

test/jdk/javax/swing/JTableHeader/TableHeaderBorderPositionTest.java line 68:

> 66: 
> 67:         BufferedImage bufferedImage = new BufferedImage(WIDTH, HEIGHT,
> 68:                 BufferedImage.TYPE_INT_RGB);

Hm, you have to create the image large enough to contain the table after scaling it by `SCALE`, so you still need `w` and `h` or you could inline `WIDTH * SCALE` and `HEIGHT * SCALE` correspondingly.

test/jdk/javax/swing/JTableHeader/TableHeaderBorderPositionTest.java line 76:

> 74:             g2d.translate(0, header.getHeight());
> 75:             table.paint(g2d);
> 76:         } finally {

Awesome! I didn't think about it. If it works, it makes verification easier. When you read the code, you clearly see the borders of the header and grid align.

I expected the table would paint both its header and its contents, but it may require validation which can't happen until the table is added to a heavyweight container such as a frame.

test/jdk/javax/swing/JTableHeader/TableHeaderBorderPositionTest.java line 83:

> 81:                                           .getColumnModel()
> 82:                                           .getColumn(0)
> 83:                                           .getWidth() * SCALE) - 2;

Suggestion:

        int verticalLineCol = (int)(table.getTableHeader()
                                         .getColumnModel()
                                         .getColumn(0)
                                         .getWidth() * SCALE) - 2;

If you add a space after the cast operator, add it everywhere; if you don't, add it nowhere.

I could've given you a suggestion based on my formatting, and I have the space after the cast operator because Java Coding Style recommends adding it. Yet there's no consensus about it in client-libs.

test/jdk/javax/swing/JTableHeader/TableHeaderBorderPositionTest.java line 85:

> 83:                                           .getWidth() * SCALE) - 2;
> 84:         int expectedRGB = bufferedImage.getRGB(verticalLineCol, 0);
> 85:         int maxHeight = (int)(((double)table.getTableHeader().getHeight() * SCALE )

Suggestion:

        int maxHeight = (int)(((double)table.getTableHeader().getHeight() * SCALE)

-------------

PR Review: https://git.openjdk.org/jdk/pull/14766#pullrequestreview-1555731207
PR Review Comment: https://git.openjdk.org/jdk/pull/14766#discussion_r1279855301
PR Review Comment: https://git.openjdk.org/jdk/pull/14766#discussion_r1279853146
PR Review Comment: https://git.openjdk.org/jdk/pull/14766#discussion_r1279858473
PR Review Comment: https://git.openjdk.org/jdk/pull/14766#discussion_r1279850115