On Tue, 21 Nov 2023 04:53:35 GMT, Tejesh R <t...@openjdk.org> wrote:
>> 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.
>
> The methods can be both moved to Utility class for sure, but not sure of
> re-using the code from `JTableHeader` coz `getColumnModel()` and
> `getWidthInRightToLeft()` are specific to it.
How is `getColumnModel()` specific?
I didn't dive into comparing rendering… I expect the gist of it is the same,
you mentioned that it was similar. It doesn't necessarily mean the header and
table cells can re-use the algorithm, yet I feel it will be beneficial — one
algorithm for the two similar cases, less code duplication.
>> 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.
>
> <img width="719" alt="JTableRTLOrientationFix"
> src="https://github.com/openjdk/jdk/assets/94159358/3de732e6-cab6-4bb0-8c25-724fac22eced";>
> As far as I understood from the description, it's about
> **Orientation**—left-to-right vs. right-to-left—rather than alignment.
My comment referred to `-Alignment` in the test name which should be replaced
with `-Orientation` because you're dealing with *orientation* here, not with
alignment.
>> 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.
>
> Any particular reason for this suggestion ?
It is much more efficient this way: `getPixelColor` captures 1 pixel of the
screen but what you really want is to capture the entire table one way or
another. Capturing the entire table with `createScreenCapture` once means you
read pixels from the screen only once.
Painting to a `BufferedImage` is even more efficient, it avoids accessing
screen pixels altogether, and the test could be made headless.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16374#discussion_r1400772062
PR Review Comment: https://git.openjdk.org/jdk/pull/16374#discussion_r1400749864
PR Review Comment: https://git.openjdk.org/jdk/pull/16374#discussion_r1400765296
