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

[Alexey Ivanov]

On Tue, 21 Nov 2023 04:53:35 GMT, Tejesh R <tr at 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