<Swing Dev> Review request for 4473075: JTable header rendering problem (after setting preferred size)

[Sergey Bylokhov]

On 04.03.2015 17:58, Semyon Sadetsky wrote:
> the updated webrev
> http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/4473075/webrev.03/
This version looks good.

>> At first do you know l&f that use anther than BasicTableHeaderUI 
>> getPreferredSize() definition?
>> And second changing the l&f affects nothing because the overridden 
>> JTableHeader.getPrefferedSize() always takes preferred size form the 
>> assigned UI.
>> In the previous code the UI preferred size could be masked completely 
>> but now only height can be masked and width is transparently passed 
>> from the UI. I don't see how the change can degrade the UI 
>> compatibility.
This is not only about a test coverage but about test stability. There 
is an assumtion that the test will complete successfully on all 
supported looks and feels, if the l&f was not set explicitly. Note that 
our sqe team sometimes run all tests using non-default look and feel(gtk 
for example), and the test can fail just because of the small difference 
in the components layout(especially if the test uses some constant)

>>
>> --Semyon
>>
>> On 3/4/2015 2:58 PM, Sergey Bylokhov wrote:
>>> Hi, Semyon.
>>>  A few notes:
>>>  - Please rephrase "Returns a preferred size of the TableHeader". 
>>> Use the class name JTableHeader or something like this: "Returns the 
>>> table header's...."
>>>  - The frame should be disposed at the end of the test.
>>>
>>> Small issues, not necessary to be fixed:
>>>  - You can add @override to the new method
>>>  - The test can iterate over all supported l&f.
>>>
>>> On 03.03.2015 16:53, Semyon Sadetsky wrote:
>>>> synchronization is not necessary because the invokeAndWait() method 
>>>> guarantee that the variable will never be accessed from EDT after it.
>>>>
>>>> On 3/3/2015 4:45 PM, Alexander Scherbatiy wrote:
>>>>> On 3/3/2015 3:30 PM, Semyon Sadetsky wrote:
>>>>>> http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/4473075/webrev.02/ 
>>>>>>
>>>>>>
>>>>>> null check is not needed because isPreferredSizeSet() is true 
>>>>>> only if preferred size is not null.
>>>>>> Others are accepted.
>>>>>    The point field is used both on the main and EDT thread in the 
>>>>> test. There should be proper synchronization to the fields access.
>>>>>
>>>>>    Otherwise  the fix looks good to me.
>>>>>
>>>>>    Thanks,
>>>>>    Alexandr.
>>>>>
>>>>>>
>>>>>>
>>>>>> On 3/3/2015 1:09 PM, Alexander Scherbatiy wrote:
>>>>>>>
>>>>>>>   The fix looks good to me.
>>>>>>>
>>>>>>>   Just few comments:
>>>>>>>   - Check that getPreferredSize() can't return null if prefferd 
>>>>>>> size is set
>>>>>>>   - extra empty lines 442 and 463 are unnecessary
>>>>>>>   - scpScroll.getHorizontalScrollBar() call (line 69) in the 
>>>>>>> test is called on the main thread. It should be called on EDT.
>>>>>>>
>>>>>>>  Thanks,
>>>>>>>  Alexandr.
>>>>>>>
>>>>>>> On 3/2/2015 4:11 PM, Semyon Sadetsky wrote:
>>>>>>>> Regression test scenario was added. Please review the fix.
>>>>>>>>
>>>>>>>> webrev: 
>>>>>>>> http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/4473075/webrev.01/ 
>>>>>>>>
>>>>>>>>
>>>>>>>> 2Sergey:
>>>>>>>> the garbage mentioned comments in the JBS is not reproducible 
>>>>>>>> anymore. In any case it is another problem than the described 
>>>>>>>> in the bug header. The fix solves the original issue.
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2/27/2015 4:47 PM, Semyon Sadetsky wrote:
>>>>>>>>> Hi Sergey,
>>>>>>>>>
>>>>>>>>> It really depends on a point of view.
>>>>>>>>> There is a comment in the ViewportLayout.java that states that 
>>>>>>>>> it is an expected behaviour:
>>>>>>>>> /* If the new viewport size would leave empty space to the
>>>>>>>>>   * right of the view, right justify the view or left justify
>>>>>>>>>   * the view when the width of the view is smaller than the
>>>>>>>>>   * container.
>>>>>>>>>   */
>>>>>>>>> ==
>>>>>>>>> Semyon
>>>>>>>>>
>>>>>>>>> On 2/27/2015 4:06 PM, Sergey Bylokhov wrote:
>>>>>>>>>> Hi, Semyon.
>>>>>>>>>> Can you clarify the comment below from the CR. Is the actual 
>>>>>>>>>> bug in JTableHeader and not in JScrollPane? Especially 
>>>>>>>>>> consider, that the behavior changes by different modes of a 
>>>>>>>>>> JScrollPane.
>>>>>>>>>>
>>>>>>>>>> "This problem isn't unique to JTableHeader. For example, 
>>>>>>>>>> consider the test case I've attached, Bug2.java. Run it the 
>>>>>>>>>> same way, resized so that the label header component shows 
>>>>>>>>>> "..." at the end, and then scroll.
>>>>>>>>>>
>>>>>>>>>> This appears to be a problem with how JScrollPane treats any 
>>>>>>>>>> header component with a fixed size like this. There are two 
>>>>>>>>>> things here:
>>>>>>>>>>
>>>>>>>>>> First, I don't understand why garbage is allowed to show up. 
>>>>>>>>>> We at least need to fix this. Second, perhaps JScrollPane 
>>>>>>>>>> needs to do better to ensure that the header is large enough 
>>>>>>>>>> to match the scrollable content."
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Also I think it is possible to write a test for this issue.
>>>>>>>>>>
>>>>>>>>>> -----semyon.sadetsky at oracle.com  wrote:
>>>>>>>>>>
>>>>>>>>>>> Hello,
>>>>>>>>>>>
>>>>>>>>>>> please review a fix for JDK 9.
>>>>>>>>>>>
>>>>>>>>>>> Bug:https://bugs.openjdk.java.net/browse/JDK-4473075
>>>>>>>>>>> Webrev:
>>>>>>>>>>> http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/4473075/webrev.00/ 
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> The thing is the general logic of the viewport layout 
>>>>>>>>>>> requires the
>>>>>>>>>>> right
>>>>>>>>>>> component size which is requested by the getPreferredSize 
>>>>>>>>>>> call to the
>>>>>>>>>>>
>>>>>>>>>>> child component.
>>>>>>>>>>> The right width of JTableHeader should be calculated in its 
>>>>>>>>>>> UI but if
>>>>>>>>>>>
>>>>>>>>>>> user sets the preferred size manually it makes the 
>>>>>>>>>>> calculated size
>>>>>>>>>>> invisible outside the component. User may need to set 
>>>>>>>>>>> preferred size
>>>>>>>>>>> in
>>>>>>>>>>> order to adjust header height but in 
>>>>>>>>>>> setPreferredSize(Dimension) the
>>>>>>>>>>> header width is adjusted as well.
>>>>>>>>>>> The fix solution overrides JComponent::getPreferredSize in
>>>>>>>>>>> JTableHeader
>>>>>>>>>>> with the logic returning the right width while preserving 
>>>>>>>>>>> the height
>>>>>>>>>>> adjustable for user.
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>>
>>
>


-- 
Best regards, Sergey.