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