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

[Semyon Sadetsky]

the updated webrev
http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/4473075/webrev.03/

On 3/4/2015 3:49 PM, Semyon Sadetsky wrote:
> Hi Sergey,
>
> Thank you for another fast review.
> all comments are accepted exept for this:
>
> > - The test can iterate over all supported l&f.
>
> 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.
>
> --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.
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>>
>