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

[Semyon Sadetsky]

Thank you Sergey.

Please review the corresponding CCC request:

http://ccc.us.oracle.com/4473075?edit

--Semyon


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.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>
>