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

[Semyon Sadetsky]

javadoc to the introduced JTableHeader.getPreferredSize() was changed 
according to Phil's corrections:
The last webrev: 
http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/4473075/webrev.04/

>
>
> On 3/4/2015 11:28 PM, Sergey Bylokhov wrote:
>> 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.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>>
>