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

[Semyon Sadetsky]

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