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

Mon Mar 16 22:32:01 UTC 2015

Hi, Semyon.
The fix looks fine.

16.03.15 3:09, Semyon Sadetsky wrote:
> 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.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>>
>>
>

-- 
Best regards, Sergey.