RFR: 5108458: JTable does not properly layout its content [v5]

[Alexey Ivanov]

On Mon, 20 Nov 2023 05:24:51 GMT, Tejesh R <tr at openjdk.org> wrote:

>> Table contents does not follow right-left Orientation when Max width of columns are set. This is due to not considering the offset in `x position` while painting table grid and table cell. The fix handles the offset and adjust the x position for each paint, similar to how header is painted. The fix is applied to both Basic and Synth Look and Feel. 
>> The fix is verified for all Look and Feel manually and test verifies on Metal L&F since automatic test cannot be generalized throughout other Look and Feel. 
>> CI tested is green for regression check and test check.
>
> Tejesh R has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Review fix

src/java.desktop/share/classes/javax/swing/plaf/synth/SynthTableUI.java line 855:

> 853:         }
> 854:     }
> 855:     private int getXPosition(int column) {

This method is exactly the same as in `BasicTableUI`, if I don't miss anything.

You should rather move this to a utility class which is accessible to both `-UI` classes. Even more: if you're following the same code path that's implemented for `JTableHeader`, you should consider re-using the code from the header painting too… by extracting the relevant parts to a utility class if appropriate.

If another bug is found, it will need to be fixed in *one place* instead of several places which repeat the same code.

test/jdk/javax/swing/JTable/JTableRightAlignmentTest.java line 57:

> 55:  */
> 56: 
> 57: public class JTableRightAlignmentTest {

As far as I understood from the description, it's about **Orientation**—left-to-right vs. right-to-left—rather than alignment.

test/jdk/javax/swing/JTable/JTableRightAlignmentTest.java line 87:

> 85:             SwingUtilities.invokeAndWait(() -> {
> 86:                 int maxHeight = (int) (((double) table.table.getTableHeader().getHeight())
> 87:                         + ((double) table.table.getHeight()));

Why do you need to convert the height to `double`?

test/jdk/javax/swing/JTable/JTableRightAlignmentTest.java line 93:

> 91:                                 .getColumn(2)
> 92:                                 .getWidth() - 1;
> 93:                 Color expectedRGB = robot.getPixelColor(xPos, yPos);

I suggest capturing the screen rectangle and then sample the colours of the captured buffered image, or alternatively even paint into a buffered image.

test/jdk/javax/swing/JTable/JTableRightAlignmentTest.java line 100:

> 98:                         saveImage(failImage, "failureImage.png");
> 99:                         passed = false;
> 100:                         failureString = "Test Failed at <" + xPos + ", " + y + ">";

`failureString == null` implies `passed` has the value `true` — one variable / field is enough.

test/jdk/javax/swing/JTable/JTableRightAlignmentTest.java line 144:

> 142:             "Salary",
> 143:     };
> 144:     List data = new ArrayList();

You should use generic `List<CustomTable.Data>` rather than raw one.

test/jdk/javax/swing/JTable/JTableRightAlignmentTest.java line 157:

> 155:     }
> 156: 
> 157:     class Data {

It should be static. You can use a `record` for simplicity.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16374#discussion_r1399424912
PR Review Comment: https://git.openjdk.org/jdk/pull/16374#discussion_r1399427167
PR Review Comment: https://git.openjdk.org/jdk/pull/16374#discussion_r1399431006
PR Review Comment: https://git.openjdk.org/jdk/pull/16374#discussion_r1399435563
PR Review Comment: https://git.openjdk.org/jdk/pull/16374#discussion_r1399441176
PR Review Comment: https://git.openjdk.org/jdk/pull/16374#discussion_r1399446461
PR Review Comment: https://git.openjdk.org/jdk/pull/16374#discussion_r1399449675