RFR: 5108458: JTable does not properly layout its content [v5]
Alexey Ivanov
aivanov at openjdk.org
Tue Nov 21 15:43:24 UTC 2023
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
More information about the client-libs-dev
mailing list