On Mon, 20 Nov 2023 05:24:51 GMT, Tejesh R <t...@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
