Re: [8u-backport] JDK 8145547: [AWT/Swing] Conditional support for GTK 3 on Linux
+1 --Semyon On 10/8/18 12:34 PM, Phil Race wrote: Hi, I've looked over the code and tested on Ubuntu 18.04 and can confirm it works as well as the JDK 11 code does. I reproduced the problems Semyon noted that are present on JDK 11 as well. The UI hang when switching themes is not specific to using GTK3 - it occurs with GTK2 as well, and not specific to Ubuntu 18.04, it occurs on Ubuntu 16.04 too. "AWT-EventQueue-0" #14 prio=6 os_prio=0 cpu=1537.25ms elapsed=16.86s tid=0x7f7d2c6dc000 nid=0x4a32 runnable [0x7f7cca817000] java.lang.Thread.State: RUNNABLE at com.sun.java.swing.plaf.gtk.GTKEngine.native_switch_theme(java.desktop/Native Method) at com.sun.java.swing.plaf.gtk.GTKEngine.themeChanged(java.desktop/GTKEngine.java:625) - locked <0x000706613b68> (a java.lang.Object) at com.sun.java.swing.plaf.gtk.GTKLookAndFeel$WeakPCL$1.run(java.desktop/GTKLookAndFeel.java:1496) It does NOT occur with current JDK 8u, so there is something in the GTK 3 work causing it. This is not a stopper for the backport since switching themes is unlikely / rare. But we should file bugs against that and the rendering issues and fix in JDK 12 and backport those too - as a follow on as at least some of these could be blockers for GTK3 on by default in 8u. "+1" to this main backport. -phil. On 10/08/2018 01:23 AM, Pankaj Bansal wrote: Hi, The patch in 02 version no longer applies cleanly as check-ins have been done since it was created. I have updated the webrev. Also fixed the whitespace in gtk2_interface.c Webrev: http://cr.openjdk.java.net/~pbansal/gtk3_backport/webrev.03/ Regards, Pankaj -Original Message- From: Kevin Rushforth Sent: Wednesday, September 12, 2018 8:19 PM To: Pankaj Bansal; awt-...@openjdk.java.net; swing-dev@openjdk.java.net Subject: Re: [8u-backport] JDK 8145547: [AWT/Swing] Conditional support for GTK 3 on Linux Looks fine. Btw, you have a trailing whitespace in gtk2_interface.c that you need to fix before you push (jcheck will tell you about that). -- Kevin On 9/12/2018 5:29 AM, Pankaj Bansal wrote: Hi, I have found a small mistake in gtk_interface.c. I had used wrong name of gtk3 lib at line 56. I have correct the same in webrev.02. It is just a typo and should not have any effect on functionality. All should work the same as 01 version. Webrev: http://cr.openjdk.java.net/~pbansal/gtk3_backport/webrev.02/ Regards, Pankaj -Original Message- From: Kevin Rushforth Sent: Saturday, September 8, 2018 1:53 AM To: Pankaj Bansal; Sergey Bylokhov; awt-...@openjdk.java.net; swing-dev@openjdk.java.net Subject: Re: [8u-backport] JDK 8145547: [AWT/Swing] Conditional support for GTK 3 on Linux I tested various combinations of Swing / FX interop and it all looks fine to me. I only glanced through the AWT code changes, though, but I didn't spot anything amiss. -- Kevin On 9/6/2018 4:35 AM, Pankaj Bansal wrote: Hello Sergey/Kevin, I have removed the backport for https://bugs.openjdk.java.net/browse/JDK-8154546 : Retire sun.misc.GThreadHelper. I did a clean build and tried few tests for Swing-FX interop. They all run fine. I have attached the link to tests if you would like to have a look. I did run the awt and swing jtreg tests also. Webrev: http://cr.openjdk.java.net/~pbansal/gtk3_backport/webrev.01/ SwingFXInteropTests: http://cr.openjdk.java.net/~pbansal/gtk3_backport/SwingFXInteropTests / Please let me know if you would like me to do some further testing for Swing-FX interop. Regards, Pankaj Bansal -Original Message- From: Kevin Rushforth Sent: Thursday, September 6, 2018 3:29 AM To: Sergey Bylokhov; Pankaj Bansal; awt-...@openjdk.java.net; swing-dev@openjdk.java.net Subject: Re: [8u-backport] JDK 8145547: [AWT/Swing] Conditional support for GTK 3 on Linux The simple testing that I did -- one each of a Swing app + JFXPanel and a JavaFX app + SwingNode -- worked for me on my local build after restoring that file. Some additional testing (necessarily limited to GTK 2 until the FX backport for GTK 3 is done) might be needed. -- Kevin On 9/5/2018 2:35 PM, Sergey Bylokhov wrote: HI, Pankaj. Can you please recheck that FX<-->Swing interop still works? Probably there are some other than GThreadHelper issues On 05/09/2018 11:44, Pankaj Bansal wrote: Hello Kevin, Thanks for pointing it out. I will remove this fix for now. I hope its ok, if I create new webrev when I get some more comments here. Regards, Pankaj Bansal *From:*Kevin Rushforth *Sent:* Wednesday, September 5, 2018 10:29 PM *To:* Pankaj Bansal; awt-...@openjdk.java.net; swing-dev@openjdk.java.net *Subject:* Re: [8u-backport] JDK 8145547: [AWT/Swing] Conditional support for GTK 3 on Linux The backport of the following fix, which removes sun.misc.GThreadHelper, will break all FX interop applications, even if GTK2 is used: https://bugs.openjdk.java.net/browse/JDK-8154546 : Retire sun.misc.GThreadHelper
[12] RFR JDK-8218473: JOptionPane display issue with GTKLookAndFeel
Hello, Please review a fix for JDK 12. JBS: https://bugs.openjdk.java.net/browse/JDK-8218473 webrev: http://cr.openjdk.java.net/~ssadetsky/8218473/webrev.00/ This is a JCK test failure fix. The test fails under the GTKLookAndFeel on Linux platforms with gtk+-3.20+ library installed. Starting gtklib- 3.20 the widget drawing styles were totally reworked so the text selection background cannot be obtained using TEXT_FIELD widget anymore. To fix that the corresponding request is redirected to the TEXT_AREA widget. The fix was tested on all major supported Linux platforms. --Semyon
[12] RFR JDK-8218479: JTextPane display issue with GTKLookAndFeel
Hello, Please review a fix for JDK 12. JBS: https://bugs.openjdk.java.net/browse/JDK-8218479 webrev: http://cr.openjdk.java.net/~ssadetsky/8218479/webrev.00/ This is a JCK test failure fix. The test fails under the GTKLookAndFeel on Linux platforms with gtk+-3.20+ library installed. Starting gtklib- 3.20 the widget drawing styles were totally reworked so the background layer of the TEXT_PANE widget should be painted explicitly. To fix the issue the corresponding area type is made opaque. The fix was tested on all major supported Linux platforms. --Semyon
[13] RFR 8153090 8223788 8224149: TAB key cannot change input focus after the radio button in the Color Selection dialog.+
bugs: https://bugs.openjdk.java.net/browse/JDK-8153090 https://bugs.openjdk.java.net/browse/JDK-8223788 https://bugs.openjdk.java.net/browse/JDK-8224149 webrev: http://cr.openjdk.java.net/~ssadetsky/8153090/webrev.00/ The fix eliminates issues in JColorChooser dialog making it more accessible by keyboard. See JBS for details. --Semyon
Re: [13] RFR 8153090 8223788 8224149: TAB key cannot change input focus after the radio button in the Color Selection dialog.+
On 5/20/19 11:04 AM, Philip Race wrote: Hi, I'm afraid I know next to nothing about the focus traversal code in Swing or AWT, but it smells very wrong to have such knowledge of a specific Swing component in the AWT focus code. I don't see anything wrong here. You can find a lot of Swing component specific code in java.awt.* implementations. This is not the first Swing aware AWT class. --Semyon -phil. On 5/20/19, 8:37 AM, semyon.sadet...@oracle.com wrote: bugs: https://bugs.openjdk.java.net/browse/JDK-8153090 https://bugs.openjdk.java.net/browse/JDK-8223788 https://bugs.openjdk.java.net/browse/JDK-8224149 webrev: http://cr.openjdk.java.net/~ssadetsky/8153090/webrev.00/ The fix eliminates issues in JColorChooser dialog making it more accessible by keyboard. See JBS for details. --Semyon
[13] RFR 8196096: javax/swing/JPopupMenu/6580930/bug6580930.java fails
JBS: https://bugs.openjdk.java.net/browse/JDK-8196096 webrev: http://cr.openjdk.java.net/~ssadetsky/8196096/webrev.00/ This is a fix for an unstable test: - inside EDT execution is ensured where necessary. - To pass on the mac platform the check for lightweight popup is excluded and the taskbar overlapping Swing property is enabled. --Semyon
Re: [13] RFR 8196096: javax/swing/JPopupMenu/6580930/bug6580930.java fails
On 5/22/19 2:29 PM, Sergey Bylokhov wrote: Hi, Semyon. Why it s necessary to change the adjustPopupLocationToFit value? why on macOS it does not work by default like on others platforms? If I read an initial bug 6580930 correctly there was an intention that HW popup should be able to overlap taskbar by default and I assume it works this way on win/lin(or it does not work)? Any Hi Sergey, It's hard to say now why this Swing property was introduced on Mac. But the current fix is intended to fix the test only and not the product. You can request the removal of the property in a separate bug. --Semyon On 22/05/2019 13:27, semyon.sadet...@oracle.com wrote: JBS: https://bugs.openjdk.java.net/browse/JDK-8196096 webrev: http://cr.openjdk.java.net/~ssadetsky/8196096/webrev.00/ This is a fix for an unstable test: - inside EDT execution is ensured where necessary. - To pass on the mac platform the check for lightweight popup is excluded and the taskbar overlapping Swing property is enabled. --Semyon
Re: [13] RFR 8153090 8223788 8224149: TAB key cannot change input focus after the radio button in the Color Selection dialog.+
On 5/20/19 11:48 PM, Prasanta Sadhukhan wrote: Probably we can move this traversal code to javax.swing.SortingFocusTraversalPolicy#SwingContainerOrderFocusTraversalPolicy for this JToggleButton swing component to avoid this scepticism. Hi Prasanta, We cannot move it there because it'd not be called when ContainerOrderFocusTraversalPolicy is used. --Semyon Regards Prasanta On 21-May-19 1:36 AM, semyon.sadet...@oracle.com wrote: On 5/20/19 11:04 AM, Philip Race wrote: Hi, I'm afraid I know next to nothing about the focus traversal code in Swing or AWT, but it smells very wrong to have such knowledge of a specific Swing component in the AWT focus code. I don't see anything wrong here. You can find a lot of Swing component specific code in java.awt.* implementations. This is not the first Swing aware AWT class. --Semyon -phil. On 5/20/19, 8:37 AM, semyon.sadet...@oracle.com wrote: bugs: https://bugs.openjdk.java.net/browse/JDK-8153090 https://bugs.openjdk.java.net/browse/JDK-8223788 https://bugs.openjdk.java.net/browse/JDK-8224149 webrev: http://cr.openjdk.java.net/~ssadetsky/8153090/webrev.00/ The fix eliminates issues in JColorChooser dialog making it more accessible by keyboard. See JBS for details. --Semyon
Re: [13] RFR 8196096: javax/swing/JPopupMenu/6580930/bug6580930.java fails
On 5/22/19 3:15 PM, Sergey Bylokhov wrote: On 22/05/2019 14:59, semyon.sadet...@oracle.com wrote: On 5/22/19 2:29 PM, Sergey Bylokhov wrote: Why it s necessary to change the adjustPopupLocationToFit value? why on macOS it does not work by default like on others platforms? If I read an initial bug 6580930 correctly there was an intention that HW popup should be able to overlap taskbar by default and I assume it works this way on win/lin(or it does not work)? Any It's hard to say now why this Swing property was introduced on Mac. But the current fix is intended to fix the test only and not the product. You can request the removal of the property in a separate bug. This property was introduced long before the macosx-port via JDK-4425878. If the test does not work properly because of the product bug, then you should not work around it, but instead, you should create a new product bug and add this test and a new bugid to the problem list for macOS. I don't agree. Java specification doesn't say that the popup menu shall overlap the OS taskbar. The test scenario shouldn't expect that. Permission to overlap taskbar is determined by the security settings and the platform default settings. By adding adjustPopupLocationToFit=false the test is abstracted from those settings which makes the overlap testing sensible. Otherwise the test may fail in some environments and platforms. As for the mac platform where the taskbar overlapping is disabled by default in the toolkit. It was intentionally disabled by fixing of a bug reported by NetBeans team. There is JDK-7124313 reported internally which requests the opposite, but it requires investigation at first, it is not clear that we can do this. --Semyon On 22/05/2019 13:27, semyon.sadet...@oracle.com wrote: JBS: https://bugs.openjdk.java.net/browse/JDK-8196096 webrev: http://cr.openjdk.java.net/~ssadetsky/8196096/webrev.00/ This is a fix for an unstable test: - inside EDT execution is ensured where necessary. - To pass on the mac platform the check for lightweight popup is excluded and the taskbar overlapping Swing property is enabled. --Semyon
Re: [13] RFR 8196096: javax/swing/JPopupMenu/6580930/bug6580930.java fails
On 5/23/19 9:12 AM, Sergey Bylokhov wrote: On 23/05/2019 08:58, semyon.sadet...@oracle.com wrote: This property was introduced long before the macosx-port via JDK-4425878. If the test does not work properly because of the product bug, then you should not work around it, but instead, you should create a new product bug and add this test and a new bugid to the problem list for macOS. I don't agree. Java specification doesn't say that the popup menu shall overlap the OS taskbar. The test scenario shouldn't expect that. Permission to overlap taskbar is determined by the security settings and the platform default settings. By adding adjustPopupLocationToFit=false the test is abstracted from those settings which makes the overlap testing sensible. Otherwise the test may fail in some environments and platforms. As for the mac platform where the taskbar overlapping is disabled by default in the toolkit. It was intentionally disabled by fixing of a bug reported by NetBeans team. There is JDK-7124313 reported internally which requests the opposite, but it requires investigation at first, it is not clear that we can do this. You just workaround the case for which the test was created, this is not a test bug, and it should not be fixed by the change in the test during test_sprint. What product issue do you suggest to fix?
Re: [13] RFR 8196096: javax/swing/JPopupMenu/6580930/bug6580930.java fails
On 5/23/19 9:37 AM, Sergey Bylokhov wrote: On 23/05/2019 09:28, semyon.sadet...@oracle.com wrote: You just workaround the case for which the test was created, this is not a test bug, and it should not be fixed by the change in the test during test_sprint. What product issue do you suggest to fix? I do not suggest to fix a product bug, I suggest to fix the usage of EDT and any other issues of instability of the test. And add this test and a new bugid to the problem list on macOS. http://cr.openjdk.java.net/~ssadetsky/8196096/webrev.01/
[13] RFR 7184956: [macosx] JPopupMenu.setDefaultLightPopupEneble(true) doesn't work correctly
webrev: http://cr.openjdk.java.net/~ssadetsky/7184956/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-7184956 Fix for a test: Mac light weigh popup testing excluded. --Semyon
Re: [13] RFR 7184956: [macosx] JPopupMenu.setDefaultLightPopupEneble(true) doesn't work correctly
On 5/24/19 2:07 PM, Sergey Bylokhov wrote: Hi, Semyon. I am not sure that this is a test bug, the Aqua L&F uses HW popup by default, but the case when the user requests LW popup for some reason should work, it is just not implemented yet. Hi Sergey, I think your understanding of the lightWeightPopupEnabled property purpose is not fully correct. The specification of the property is very clear on that: /** * Sets the value of the lightWeightPopupEnabled property, * which by default is true. * By default, when a look and feel displays a popup, * it can choose to * use a lightweight (all-Java) popup. * Lightweight popup windows are more efficient than heavyweight * (native peer) windows, * but lightweight and heavyweight components do not mix well in a GUI. * If your application mixes lightweight and heavyweight components, * you should disable lightweight popups. * Some look and feels might always use heavyweight popups, * no matter what the value of this property. */ So, there is nothing to implement in Aqua L&F about it. --Semyon BTW: note that the current two weeks sprint is about the cleanup of our internal CI, which has some failing tests which are not in the problem list. It is not necessary to change the tests which are already in the problem list. On 24/05/2019 13:26, semyon.sadet...@oracle.com wrote: webrev: http://cr.openjdk.java.net/~ssadetsky/7184956/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-7184956 Fix for a test: Mac light weigh popup testing excluded. --Semyon
Re: [13] RFR 7184956: [macosx] JPopupMenu.setDefaultLightPopupEneble(true) doesn't work correctly
On 5/28/19 1:55 PM, Sergey Bylokhov wrote:
On 28/05/2019 09:19, semyon.sadet...@oracle.com wrote:
Hi Sergey,
I think your understanding of the lightWeightPopupEnabled property
purpose is not fully correct. The specification of the property is
very clear on that:
/**
* Sets the value of the lightWeightPopupEnabled
property,
* which by default is true.
* By default, when a look and feel displays a popup,
* it can choose to
* use a lightweight (all-Java) popup.
* Lightweight popup windows are more efficient than heavyweight
* (native peer) windows,
* but lightweight and heavyweight components do not mix well in
a GUI.
* If your application mixes lightweight and heavyweight
components,
* you should disable lightweight popups.
* Some look and feels might always use heavyweight popups,
* no matter what the value of this property.
*/
So, there is nothing to implement in Aqua L&F about it.
I guess the spec above is from the different method, which is not used
in the test? But as of the spec for
setDefaultLightWeightPopupEnabled()/getDefaultLightWeightPopupEnabled()
we of course work according to the specification, but it does not mean
that we should not implement/change the optional part.
Did you mean that *the default* value of some property has more priority
then the value of the property it-self? This sounds very strange to me.
This'd mean that if we setDefaultLightWeightPopupEnabled() to some value
than we cannot use setLightWeightPopupEnabled() to change the value.
This contradicts to the current implementation which uses the default as
initial property value only:
public JPopupMenu(String label) {
this.label = label;
lightWeightPopup = getDefaultLightWeightPopupEnabled();
...
I see no reason to change that behavior.
Re: [13] RFR 7184956: [macosx] JPopupMenu.setDefaultLightPopupEneble(true) doesn't work correctly
On 5/28/19 3:56 PM, Sergey Bylokhov wrote: On 28/05/2019 15:19, semyon.sadet...@oracle.com wrote: I guess the spec above is from the different method, which is not used in the test? But as of the spec for setDefaultLightWeightPopupEnabled()/getDefaultLightWeightPopupEnabled() we of course work according to the specification, but it does not mean that we should not implement/change the optional part. Did you mean that *the default* value of some property has more priority then the value of the property it-self? This sounds very strange to me. I said nothing about the property itself, I just point that the method which is called in the test setDefaultLightWeightPopupEnabled() is eventually ignored, but it should not. And why it shouldn't when the spec states the opposite? : * Some look and feels might always use heavyweight popups, * no matter what the value of this property. Maybe the thing is that you just overlooked the specification? If you haven't enough expertise in the area you should insist on your superficial suggestions to implement something and I shouldn't explain why they are not feasible many times here.
Re: [13] RFR 7184956: [macosx] JPopupMenu.setDefaultLightPopupEneble(true) doesn't work correctly
On 5/29/19 5:41 PM, Sergey Bylokhov wrote: On 29/05/2019 08:58, semyon.sadet...@oracle.com wrote: And why it shouldn't when the spec states the opposite? : * Some look and feels might always use heavyweight popups, * no matter what the value of this property. Maybe the thing is that you just overlooked the specification? The spec does not say the opposite, if something is specified as optional and may be ignored, does not mean that it should be ignored. OK, let's implement all possible features that is not disabled... Sounds just not sensible, isn't it? It should be somehow possible to switch from one type of popups to another regardless which type of popups used by default, since it is known that the applications use both types for a different reasons. Again, it shouldn't. The spec is very clear about that point. Aqua L&F disables lightweight popups because they are alien. If you haven't enough expertise in the area you should insist on your superficial suggestions to implement something and I shouldn't explain why they are not feasible many times here. Thank you for the suggestion.
Re: [13] RFR 7184956: [macosx] JPopupMenu.setDefaultLightPopupEneble(true) doesn't work correctly
On 6/3/19 1:07 PM, Phil Race wrote: The test update tests the OS but if I understand correctly, it is the Aqua L&F that does not support lightweight popups. Can we check if it is Aqua rather than by OS ? If you don't want to hard code Aqua then check if it is macos and system L&F. Ideally there'd be some way to query whether a L&F supports this case but there isn't, so I'd like to see the test include a comment that says // No API exists to check this, but Aqua currently does not support lightweight popups. OK: http://cr.openjdk.java.net/~ssadetsky/7184956/webrev.01 Lightweight popups seem to me to be principally a performance optimisation. I agree, it is clear from the spec that the property is mostly aimed to enforce lightweight popups disabled rather than enabling them. And this optimization is not very effective since even if the lightweight popups are enabled they still cannot be used beyond the borders of the window and above other windows which are the areas where popups are usually showed up. Also the behavior and appearance of lightweight popups often don't correspond to the native apps. For example, on Mac if a Swing widow is overlapped by the top window remaining visible some area of the Swing window which got the right mouse click then the appeared popup menu should overlap the top window without bringing the Swing window to top. This is cannot be achieve using the optimized lightweight version of the popup that will be selected when the popup size fits in the Swing window despite it intersects the top window. The thing you have to support is heavyweights and lightweights are optional, and since I don't think we are likely to investigate whether this can be done for Aqua anytime soon then the test update seems like an OK step. That would require to introduce an API change and the implementation which addresses all those behavioral and appearance issues. Just unreasonable. --Semyon -phil On 5/24/19 1:26 PM, semyon.sadet...@oracle.com wrote: webrev: http://cr.openjdk.java.net/~ssadetsky/7184956/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-7184956 Fix for a test: Mac light weigh popup testing excluded. --Semyon
Re: [13] Review Request: 8225144 [macos] In Aqua L&F backspace key does not delete when Shift is pressed
Hi Sergey, Please add also Ctrl+BackSpace (delete previous char) and Command+BackSpace (delete previous word) keystrokes which are ignored in Aqua as well. --Semyon On 6/3/19 5:16 PM, Sergey Bylokhov wrote: Hello. Please review the fix for JDK 13. Bug: https://bugs.openjdk.java.net/browse/JDK-8225144 Fix: http://cr.openjdk.java.net/~serb/8225144/webrev.00 This bug is a version of JDK-6361367[1] but this time for Aqua L&F. In JTextFields and JTextAreas when pressing with backspace and shift nothing happens, but the native programs delete under these circumstances. In the fix in the Aqua L&F, the key binding for "shift+backspace" is added, actually, it means that shift in this shortcut will be ignored and simple backspace action will be executed. [1] https://bugs.openjdk.java.net/browse/JDK-6361367
Re: [13] RFR 7184956: [macosx] JPopupMenu.setDefaultLightPopupEneble(true) doesn't work correctly
On 6/3/19 5:19 PM, Philip Race wrote: I don't know how much I agree with the earlier email but this much is true, i.e. it doesn't make any practical difference to avoid running the test in a different way. The practical difference is the coverage. We avoid running only one specific scenario of the test and only for one L&F other test scenarios and L&Fs will not be avoided. --Semyon
Re: [13] Review Request: 8225144 [macos] In Aqua L&F backspace key does not delete when Shift is pressed
On 6/4/19 7:24 PM, Sergey Bylokhov wrote: Hi, Semyon. I would like to investigate other macOS specific shortcuts in a separate CR What specifically you need to investigate? Those keystrokes work for all other L&Fs.and I see no other actions mapped to them. Please add also Ctrl+BackSpace (delete previous char) I made a quick look and it seems ctrl+backspace does not work in some application like Thunderbird. That is not a valid argument. Thunderbird is base on Mozilla XUL framework which UI components behavior can be different from the standard one. and Command+BackSpace (delete previous word) keystrokes which are ignored in Aqua as well. On macOS to remove the previous word the "Opt+BackSpace" is used and it is implemented. That's correct, but Command modifier works the same way. Also "Ctrl+BackSpace" and "Command+BackSpace", unlike "Opt+BackSpace" are not mentioned in the list of "standard" shortcuts: https://support.apple.com/en-us/HT201236 Anyway it works in macOS native apps and costs nothing to be added to Aqua L&F. OK, this one is up to you... --Semyon On 04/06/2019 09:18, semyon.sadet...@oracle.com wrote: --Semyon On 6/3/19 5:16 PM, Sergey Bylokhov wrote: Hello. Please review the fix for JDK 13. Bug: https://bugs.openjdk.java.net/browse/JDK-8225144 Fix: http://cr.openjdk.java.net/~serb/8225144/webrev.00 This bug is a version of JDK-6361367[1] but this time for Aqua L&F. In JTextFields and JTextAreas when pressing with backspace and shift nothing happens, but the native programs delete under these circumstances. In the fix in the Aqua L&F, the key binding for "shift+backspace" is added, actually, it means that shift in this shortcut will be ignored and simple backspace action will be executed. [1] https://bugs.openjdk.java.net/browse/JDK-6361367
Re: [13] RFR 8153090 8223788 8224149: TAB key cannot change input focus after the radio button in the Color Selection dialog.+
On 6/6/19 12:29 AM, Prasanta Sadhukhan wrote: Hi Semyon, I do not have much idea of focus traversal code but it seems when your regression testcase is run, SwingContainerOrderFocusTraversalPolicy does get called as I can see, so I see no harm in putting the code there, given Phil's objection. You see it called in the color panel or for other container? --Semyon Regards Prasanta On 23-May-19 3:46 AM, semyon.sadet...@oracle.com wrote: On 5/20/19 11:48 PM, Prasanta Sadhukhan wrote: Probably we can move this traversal code to javax.swing.SortingFocusTraversalPolicy#SwingContainerOrderFocusTraversalPolicy for this JToggleButton swing component to avoid this scepticism. Hi Prasanta, We cannot move it there because it'd not be called when ContainerOrderFocusTraversalPolicy is used. --Semyon Regards Prasanta On 21-May-19 1:36 AM, semyon.sadet...@oracle.com wrote: On 5/20/19 11:04 AM, Philip Race wrote: Hi, I'm afraid I know next to nothing about the focus traversal code in Swing or AWT, but it smells very wrong to have such knowledge of a specific Swing component in the AWT focus code. I don't see anything wrong here. You can find a lot of Swing component specific code in java.awt.* implementations. This is not the first Swing aware AWT class. --Semyon -phil. On 5/20/19, 8:37 AM, semyon.sadet...@oracle.com wrote: bugs: https://bugs.openjdk.java.net/browse/JDK-8153090 https://bugs.openjdk.java.net/browse/JDK-8223788 https://bugs.openjdk.java.net/browse/JDK-8224149 webrev: http://cr.openjdk.java.net/~ssadetsky/8153090/webrev.00/ The fix eliminates issues in JColorChooser dialog making it more accessible by keyboard. See JBS for details. --Semyon
[14] RFR 8226513:, JEditorPane is shown with incorrect size
bug: https://bugs.openjdk.java.net/browse/JDK-8226513 webrev: http://cr.openjdk.java.net/~ssadetsky/8226513/webrev.00/ The fix adds resetting the root view initialization flag when the text component underling document is changed and also removes the check for the zero component size for the root view initialization to correct the resulting preferred component size in some scenarios when the root view need to be initially layouted. --Semyon
Review Request for 8073454: When de-selecting the "Opaque" checkbox, the pink background is not shown through the togglebuttons that are not selected under the special options "-client -Xm
Hello, Please review the fix for JDK 9. The component.isOpaque() is now taken into account in the XP System LAF if it is available to do not render component background in case of FALSE. Bug: https://bugs.openjdk.java.net/browse/JDK-8073454 Webrev: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8073454/webrev.01/
Re: Review Request for 8073454: When de-selecting the "Opaque" checkbox, the pink background is not shown through the togglebuttons that are not selected under the special options "-client
fix updated: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8073454/webrev.02/ On 2/25/2015 2:14 PM, Semyon Sadetsky wrote: Hello, Please review the fix for JDK 9. The component.isOpaque() is now taken into account in the XP System LAF if it is available to do not render component background in case of FALSE. Bug: https://bugs.openjdk.java.net/browse/JDK-8073454 Webrev: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8073454/webrev.01/
Re: Review Request for 8073454: When de-selecting the "Opaque" checkbox, the pink background is not shown through the togglebuttons that are not selected under the special options "-client
Finally determined as not an issue. The ticket will be closed. On 2/26/2015 10:41 AM, Semyon Sadetsky wrote: fix updated: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8073454/webrev.02/ On 2/25/2015 2:14 PM, Semyon Sadetsky wrote: Hello, Please review the fix for JDK 9. The component.isOpaque() is now taken into account in the XP System LAF if it is available to do not render component background in case of FALSE. Bug: https://bugs.openjdk.java.net/browse/JDK-8073454 Webrev: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8073454/webrev.01/
Review request for 4473075: JTable header rendering problem (after setting preferred size)
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.
Re: Review request for 4473075: JTable header rendering problem (after setting preferred size)
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.
Re: Review request for 4473075: JTable header rendering problem (after setting preferred size)
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.
Review request for 6894632: Removing rows from a DefaultTableModel with a RowSorter deselectes last row
Hello, Please review the fix: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/6894632/webrev.01/ for bug: https://bugs.openjdk.java.net/browse/JDK-6894632 The thing is the sorter logic uses its internal data structures to restore selection when the table model has been changed. For performance reason this internal data is created only upon the table sort call. If sortable JTable is initialized with no sort order specified the table sort is not executed and table remains unsorted as JTable without sorting capability. When the model of table in such state is changed the corresponding event handler logic still called the sorter to transform selection. But since the sorter internal cache was empty the transformation failed in cases when the selection index exceeds the new table rows count. Now with the fix the sorter is updated with table model change event only if the table is really sorted.
Re: Review request for 4473075: JTable header rendering problem (after setting preferred size)
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. 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.
JDK9 client libs bugs CLOSED as not an issue
Hello, the next JBS issue were closed as not an issue: https://bugs.openjdk.java.net/browse/JDK-8073454 https://bugs.openjdk.java.net/browse/JDK-8065359 https://bugs.openjdk.java.net/browse/JDK-8055750 https://bugs.openjdk.java.net/browse/JDK-8050248 https://bugs.openjdk.java.net/browse/JDK-8042114 https://bugs.openjdk.java.net/browse/JDK-7182821
Re: Review request for 4473075: JTable header rendering problem (after setting preferred size)
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.
Re: Review request for 6894632: Removing rows from a DefaultTableModel with a RowSorter deselectes last row
No, because it wouldn't be a valid state. The primary sort column is always the first. If primary is unsorted then there are no more sorted columns. On 3/4/2015 11:54 AM, Alexander Scherbatiy wrote: Is it possible that getSortKeys() methods returns some sort keys where the first has unsorted sort order but others do not. Thanks, Alexandr. On 3/2/2015 5:47 PM, Semyon Sadetsky wrote: Hello, Please review the fix: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/6894632/webrev.01/ for bug: https://bugs.openjdk.java.net/browse/JDK-6894632 The thing is the sorter logic uses its internal data structures to restore selection when the table model has been changed. For performance reason this internal data is created only upon the table sort call. If sortable JTable is initialized with no sort order specified the table sort is not executed and table remains unsorted as JTable without sorting capability. When the model of table in such state is changed the corresponding event handler logic still called the sorter to transform selection. But since the sorter internal cache was empty the transformation failed in cases when the selection index exceeds the new table rows count. Now with the fix the sorter is updated with table model change event only if the table is really sorted.
Re: Review request for 4473075: JTable header rendering problem (after setting preferred size)
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.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.
Re: Review request for 4473075: JTable header rendering problem (after setting preferred size)
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.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.
Re: Review request for 4473075: JTable header rendering problem (after setting preferred size)
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.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
Re: Review request for 6894632: Removing rows from a DefaultTableModel with a RowSorter deselectes last row
Hi Sergey, Good catch! Yes it should be inverted. I fix that and added more extensive test scenarios to double check different table states. Please review. It's here: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/6894632/webrev.02/ Thanks! Semyon On 3/6/2015 1:26 AM, Sergey Bylokhov wrote: Hi, Semyon . Are you sure that this new if-statement is correct? it seems that it should be opposite. No? On 04.03.2015 13:45, Alexander Scherbatiy wrote: The fix looks good to me. Thanks, Alexandr. On 3/4/2015 12:29 PM, Semyon Sadetsky wrote: No, because it wouldn't be a valid state. The primary sort column is always the first. If primary is unsorted then there are no more sorted columns. On 3/4/2015 11:54 AM, Alexander Scherbatiy wrote: Is it possible that getSortKeys() methods returns some sort keys where the first has unsorted sort order but others do not. Thanks, Alexandr. On 3/2/2015 5:47 PM, Semyon Sadetsky wrote: Hello, Please review the fix: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/6894632/webrev.01/ for bug: https://bugs.openjdk.java.net/browse/JDK-6894632 The thing is the sorter logic uses its internal data structures to restore selection when the table model has been changed. For performance reason this internal data is created only upon the table sort call. If sortable JTable is initialized with no sort order specified the table sort is not executed and table remains unsorted as JTable without sorting capability. When the model of table in such state is changed the corresponding event handler logic still called the sorter to transform selection. But since the sorter internal cache was empty the transformation failed in cases when the selection index exceeds the new table rows count. Now with the fix the sorter is updated with table model change event only if the table is really sorted.
Review request for 8013566: Failure of GroupLayout in combination of addPreferredGap and addGroup's
Hi, please review a fix for JDK9. bug: https://bugs.openjdk.java.net/browse/JDK-8013566 webrev: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8013566/webrev.00/ User got here an infinite loop inside GroupLayout's auto-gap-insertion mechanism that was triggered by getPreferredSize() call upon the JFrame.pack(). The GroupLayout has not trivial algorithm and its code is really nontransparent. The aim of its auto-preferred-gap-insertion algorithm is inserting gaps between the container components. This allows user to not specify gaps manually for convenience. In the considered scenario user relays on the auto-preferred-gap-insertion except for one place where he specifies the preferred gap manually. Moreover this manual preferred gap is inserted not at the leading position but at the trail. That is specifically the case when the algorithm fails. The thing is that the auto-preferred-gap-insertion routine insertAutopadding() cannot distinguish its auto inserted preferred gaps from manual preferred gaps because it uses the "instance of" check but all those gaps are instances of the same AutoPreferredGapSpring class. Also the insertAutopadding() does not insert gaps at trailing positions but only at leading. More precisely the trailing AutoPreferredGapSpring object left from the child level group (which insertAutopadding() is called recursively) is ignored on the parent group level and parent's insertAutopadding() inserts a new leading AutoPreferredGapSpring object connected to the last component of the child level group, but this in its turn makes a spring counter value (which serves as a loop exit flag) incorrect. As result preferred gaps are added infinitely on this position. The fix introduces a check that tests if the AutoPreferredGapSpring related the current leading component is already created as a trailing gap on the child group level, and if it is this trailing AutoPreferredGapSpring is used to setup the source and new AutoPreferredGapSpring object is not created. --Semyon
Re: Review request for 8013566: Failure of GroupLayout in combination of addPreferredGap and addGroup's
On 3/10/2015 2:09 PM, Alexander Scherbatiy wrote: What is the reason that the class of the manual inserted gap is AutoPreferredGapSpring and not GapSpring? According to comments in the code *AutoPreferredGapSpring* Spring reprensenting the distance between any number of sources and targets. The targets and sources are computed during layout. An instance of this can either be dynamically created when autocreatePadding is true, or explicitly created by the developer. *GapSpring* Spring represented a certain amount of space. so they have really different functionality.
Re: Review request for 8013566: Failure of GroupLayout in combination of addPreferredGap and addGroup's
On 3/10/2015 2:38 PM, Alexander Scherbatiy wrote: *AutoPreferredGapSpring* Spring reprensenting the distance between any number of sources and targets. The targets and sources are computed during layout. An instance of this can either be dynamically created when autocreatePadding is true, or explicitly created by the developer. What about just add a flag to the AutoPreferredGapSpring that allows to distinguish a manually added gap from an auto-inserted? Thanks, Alexandr. Functionality of AutoPreferredGapSpring should be the same regardless how it was added automatically or manually. The problem is connected to the auto insertion algorithm only. It is sensitive to manual AutoPreferredGapSpring added at the leading position and do not add extra gaps in this case, but ignores manual gaps on the trailing position in the child group and add extra leading gap on the parent group level.
Re: Review request for 8013566: Failure of GroupLayout in combination of addPreferredGap and addGroup's
Hi Alexander, your remarks are taken into account : http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8013566/webrev.01/ jtreg result: Execution successful * javax/swing/GroupLayout/6613904/bug6613904.java <../../JTwork/javax/swing/GroupLayout/6613904/bug6613904.jtr>: javax.swing.GroupLayout.createParallelGroup(..) doesn't throw IllegalArgumentException for null arg * javax/swing/GroupLayout/7071166/bug7071166.java <../../JTwork/javax/swing/GroupLayout/7071166/bug7071166.jtr>: LayoutStyle.getPreferredGap() - IAE is expected but not thrown * javax/swing/GroupLayout/8013566/bug8013566.java <../../JTwork/javax/swing/GroupLayout/8013566/bug8013566.jtr>: Failure of GroupLayout in combination of addPreferredGap and addGroup's last row --Semyon On 3/10/2015 3:46 PM, Alexander Scherbatiy wrote: On 3/6/2015 5:42 PM, Semyon Sadetsky wrote: Hi, please review a fix for JDK9. bug: https://bugs.openjdk.java.net/browse/JDK-8013566 webrev: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8013566/webrev.00/ Just minor comments: - 'newLeadingPadding.size() == 0' can be changed to newLeadingPadding.isEmpty() - //frame.dispose() - may be it should not be commented? Could you run regression and JCK GroupLayout tests to check that there are no known regressions. Thanks, Alexandr. User got here an infinite loop inside GroupLayout's auto-gap-insertion mechanism that was triggered by getPreferredSize() call upon the JFrame.pack(). The GroupLayout has not trivial algorithm and its code is really nontransparent. The aim of its auto-preferred-gap-insertion algorithm is inserting gaps between the container components. This allows user to not specify gaps manually for convenience. In the considered scenario user relays on the auto-preferred-gap-insertion except for one place where he specifies the preferred gap manually. Moreover this manual preferred gap is inserted not at the leading position but at the trail. That is specifically the case when the algorithm fails. The thing is that the auto-preferred-gap-insertion routine insertAutopadding() cannot distinguish its auto inserted preferred gaps from manual preferred gaps because it uses the "instance of" check but all those gaps are instances of the same AutoPreferredGapSpring class. Also the insertAutopadding() does not insert gaps at trailing positions but only at leading. More precisely the trailing AutoPreferredGapSpring object left from the child level group (which insertAutopadding() is called recursively) is ignored on the parent group level and parent's insertAutopadding() inserts a new leading AutoPreferredGapSpring object connected to the last component of the child level group, but this in its turn makes a spring counter value (which serves as a loop exit flag) incorrect. As result preferred gaps are added infinitely on this position. The fix introduces a check that tests if the AutoPreferredGapSpring related the current leading component is already created as a trailing gap on the child group level, and if it is this trailing AutoPreferredGapSpring is used to setup the source and new AutoPreferredGapSpring object is not created. --Semyon
Re: Review request for 8013566: Failure of GroupLayout in combination of addPreferredGap and addGroup's
JCK run is OK: Mar 11, 2015 1:58:55 PM Finished executing all tests, wait for cleanup... Mar 11, 2015 1:58:55 PM Harness done with cleanup from test run. Total time = 33s Setup time = 0s Cleanup time = 0s Test results: passed: 8 Hi Alexander, your remarks are taken into account : http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8013566/webrev.01/ jtreg result: Execution successful * javax/swing/GroupLayout/6613904/bug6613904.java <../../JTwork/javax/swing/GroupLayout/6613904/bug6613904.jtr>: javax.swing.GroupLayout.createParallelGroup(..) doesn't throw IllegalArgumentException for null arg * javax/swing/GroupLayout/7071166/bug7071166.java <../../JTwork/javax/swing/GroupLayout/7071166/bug7071166.jtr>: LayoutStyle.getPreferredGap() - IAE is expected but not thrown * javax/swing/GroupLayout/8013566/bug8013566.java <../../JTwork/javax/swing/GroupLayout/8013566/bug8013566.jtr>: Failure of GroupLayout in combination of addPreferredGap and addGroup's last row --Semyon On 3/10/2015 3:46 PM, Alexander Scherbatiy wrote: On 3/6/2015 5:42 PM, Semyon Sadetsky wrote: Hi, please review a fix for JDK9. bug: https://bugs.openjdk.java.net/browse/JDK-8013566 webrev: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8013566/webrev.00/ Just minor comments: - 'newLeadingPadding.size() == 0' can be changed to newLeadingPadding.isEmpty() - //frame.dispose() - may be it should not be commented? Could you run regression and JCK GroupLayout tests to check that there are no known regressions. Thanks, Alexandr. User got here an infinite loop inside GroupLayout's auto-gap-insertion mechanism that was triggered by getPreferredSize() call upon the JFrame.pack(). The GroupLayout has not trivial algorithm and its code is really nontransparent. The aim of its auto-preferred-gap-insertion algorithm is inserting gaps between the container components. This allows user to not specify gaps manually for convenience. In the considered scenario user relays on the auto-preferred-gap-insertion except for one place where he specifies the preferred gap manually. Moreover this manual preferred gap is inserted not at the leading position but at the trail. That is specifically the case when the algorithm fails. The thing is that the auto-preferred-gap-insertion routine insertAutopadding() cannot distinguish its auto inserted preferred gaps from manual preferred gaps because it uses the "instance of" check but all those gaps are instances of the same AutoPreferredGapSpring class. Also the insertAutopadding() does not insert gaps at trailing positions but only at leading. More precisely the trailing AutoPreferredGapSpring object left from the child level group (which insertAutopadding() is called recursively) is ignored on the parent group level and parent's insertAutopadding() inserts a new leading AutoPreferredGapSpring object connected to the last component of the child level group, but this in its turn makes a spring counter value (which serves as a loop exit flag) incorrect. As result preferred gaps are added infinitely on this position. The fix introduces a check that tests if the AutoPreferredGapSpring related the current leading component is already created as a trailing gap on the child group level, and if it is this trailing AutoPreferredGapSpring is used to setup the source and new AutoPreferredGapSpring object is not created. --Semyon
[9] Review Request for 8040328: JSlider has wrong preferred size with Synth LAF
Hello, please review fix for jdk9 webrev: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8040328/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-8040328 Thanks, --Semyon
Re: Review request for 8013566: Failure of GroupLayout in combination of addPreferredGap and addGroup's
Hi Sergey, I couldn't find this 6494656 in jdk and jck test repositories. How can I find it? Thanks, --Semyon On 3/11/2015 8:05 PM, Sergey Bylokhov wrote: Hi, Semyon. Please confirm that the test for 6494656 completed successfully also. 11.03.15 4:01, Semyon Sadetsky wrote: JCK run is OK: Mar 11, 2015 1:58:55 PM Finished executing all tests, wait for cleanup... Mar 11, 2015 1:58:55 PM Harness done with cleanup from test run. Total time = 33s Setup time = 0s Cleanup time = 0s Test results: passed: 8 Hi Alexander, your remarks are taken into account : http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8013566/webrev.01/ jtreg result: Execution successful * javax/swing/GroupLayout/6613904/bug6613904.java <../../JTwork/javax/swing/GroupLayout/6613904/bug6613904.jtr>: javax.swing.GroupLayout.createParallelGroup(..) doesn't throw IllegalArgumentException for null arg * javax/swing/GroupLayout/7071166/bug7071166.java <../../JTwork/javax/swing/GroupLayout/7071166/bug7071166.jtr>: LayoutStyle.getPreferredGap() - IAE is expected but not thrown * javax/swing/GroupLayout/8013566/bug8013566.java <../../JTwork/javax/swing/GroupLayout/8013566/bug8013566.jtr>: Failure of GroupLayout in combination of addPreferredGap and addGroup's last row --Semyon On 3/10/2015 3:46 PM, Alexander Scherbatiy wrote: On 3/6/2015 5:42 PM, Semyon Sadetsky wrote: Hi, please review a fix for JDK9. bug: https://bugs.openjdk.java.net/browse/JDK-8013566 webrev: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8013566/webrev.00/ Just minor comments: - 'newLeadingPadding.size() == 0' can be changed to newLeadingPadding.isEmpty() - //frame.dispose() - may be it should not be commented? Could you run regression and JCK GroupLayout tests to check that there are no known regressions. Thanks, Alexandr. User got here an infinite loop inside GroupLayout's auto-gap-insertion mechanism that was triggered by getPreferredSize() call upon the JFrame.pack(). The GroupLayout has not trivial algorithm and its code is really nontransparent. The aim of its auto-preferred-gap-insertion algorithm is inserting gaps between the container components. This allows user to not specify gaps manually for convenience. In the considered scenario user relays on the auto-preferred-gap-insertion except for one place where he specifies the preferred gap manually. Moreover this manual preferred gap is inserted not at the leading position but at the trail. That is specifically the case when the algorithm fails. The thing is that the auto-preferred-gap-insertion routine insertAutopadding() cannot distinguish its auto inserted preferred gaps from manual preferred gaps because it uses the "instance of" check but all those gaps are instances of the same AutoPreferredGapSpring class. Also the insertAutopadding() does not insert gaps at trailing positions but only at leading. More precisely the trailing AutoPreferredGapSpring object left from the child level group (which insertAutopadding() is called recursively) is ignored on the parent group level and parent's insertAutopadding() inserts a new leading AutoPreferredGapSpring object connected to the last component of the child level group, but this in its turn makes a spring counter value (which serves as a loop exit flag) incorrect. As result preferred gaps are added infinitely on this position. The fix introduces a check that tests if the AutoPreferredGapSpring related the current leading component is already created as a trailing gap on the child group level, and if it is this trailing AutoPreferredGapSpring is used to setup the source and new AutoPreferredGapSpring object is not created. --Semyon -- Best regards, Sergey.
Re: Review request for 8013566: Failure of GroupLayout in combination of addPreferredGap and addGroup's
I've found it. Thanks! $ ~/jtreg/win32/bin/jtreg -jdk:../build/windows-x86_64-normal-server-fastdebug/jdk/ test/closed/javax/swing/GroupLayout/UnitTest Test results: passed: 1 --Semyon On 3/12/2015 9:35 AM, Semyon Sadetsky wrote: Hi Sergey, I couldn't find this 6494656 in jdk and jck test repositories. How can I find it? Thanks, --Semyon On 3/11/2015 8:05 PM, Sergey Bylokhov wrote: Hi, Semyon. Please confirm that the test for 6494656 completed successfully also. 11.03.15 4:01, Semyon Sadetsky wrote: JCK run is OK: Mar 11, 2015 1:58:55 PM Finished executing all tests, wait for cleanup... Mar 11, 2015 1:58:55 PM Harness done with cleanup from test run. Total time = 33s Setup time = 0s Cleanup time = 0s Test results: passed: 8 Hi Alexander, your remarks are taken into account : http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8013566/webrev.01/ jtreg result: Execution successful * javax/swing/GroupLayout/6613904/bug6613904.java <../../JTwork/javax/swing/GroupLayout/6613904/bug6613904.jtr>: javax.swing.GroupLayout.createParallelGroup(..) doesn't throw IllegalArgumentException for null arg * javax/swing/GroupLayout/7071166/bug7071166.java <../../JTwork/javax/swing/GroupLayout/7071166/bug7071166.jtr>: LayoutStyle.getPreferredGap() - IAE is expected but not thrown * javax/swing/GroupLayout/8013566/bug8013566.java <../../JTwork/javax/swing/GroupLayout/8013566/bug8013566.jtr>: Failure of GroupLayout in combination of addPreferredGap and addGroup's last row --Semyon On 3/10/2015 3:46 PM, Alexander Scherbatiy wrote: On 3/6/2015 5:42 PM, Semyon Sadetsky wrote: Hi, please review a fix for JDK9. bug: https://bugs.openjdk.java.net/browse/JDK-8013566 webrev: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8013566/webrev.00/ Just minor comments: - 'newLeadingPadding.size() == 0' can be changed to newLeadingPadding.isEmpty() - //frame.dispose() - may be it should not be commented? Could you run regression and JCK GroupLayout tests to check that there are no known regressions. Thanks, Alexandr. User got here an infinite loop inside GroupLayout's auto-gap-insertion mechanism that was triggered by getPreferredSize() call upon the JFrame.pack(). The GroupLayout has not trivial algorithm and its code is really nontransparent. The aim of its auto-preferred-gap-insertion algorithm is inserting gaps between the container components. This allows user to not specify gaps manually for convenience. In the considered scenario user relays on the auto-preferred-gap-insertion except for one place where he specifies the preferred gap manually. Moreover this manual preferred gap is inserted not at the leading position but at the trail. That is specifically the case when the algorithm fails. The thing is that the auto-preferred-gap-insertion routine insertAutopadding() cannot distinguish its auto inserted preferred gaps from manual preferred gaps because it uses the "instance of" check but all those gaps are instances of the same AutoPreferredGapSpring class. Also the insertAutopadding() does not insert gaps at trailing positions but only at leading. More precisely the trailing AutoPreferredGapSpring object left from the child level group (which insertAutopadding() is called recursively) is ignored on the parent group level and parent's insertAutopadding() inserts a new leading AutoPreferredGapSpring object connected to the last component of the child level group, but this in its turn makes a spring counter value (which serves as a loop exit flag) incorrect. As result preferred gaps are added infinitely on this position. The fix introduces a check that tests if the AutoPreferredGapSpring related the current leading component is already created as a trailing gap on the child group level, and if it is this trailing AutoPreferredGapSpring is used to setup the source and new AutoPreferredGapSpring object is not created. --Semyon -- Best regards, Sergey.
Re: Review request for 4473075: JTable header rendering problem (after setting preferred size)
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 it
[9] Review Request for 8041642: Incorrect paint of JProgressBar in Nimbus LF
Hello, please review JDK9 fix: webrev: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8041642/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-8041642 Nimbus uses hard-coded images of 27x19 size for progress bar which is stretched using 9-squares caching algorithm. The image fixed insets are set to (5,5,5,5) so when width of the progress bar value is less then 10 pixels the image cannot be stretched correctly and garbage is painted. Fix solution: do not paint values less then summarized insets width along the bar direction. The same approach is used in the System LnF. Thanks, --Semyon
Re: [9] Review Request for 8040328: JSlider has wrong preferred size with Synth LAF
Reg test is updated because it failed on OSX platform: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8040328/webrev.01/ On 3/12/2015 12:43 PM, Alexander Scherbatiy wrote: The fix looks good to me. Thanks, Alexandr. On 3/12/2015 8:59 AM, Semyon Sadetsky wrote: Hello, please review fix for jdk9 webrev: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8040328/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-8040328 Thanks, --Semyon
Re: [9] Review Request for 8041642: Incorrect paint of JProgressBar in Nimbus LF
That's correct. Because with the insets left+right=5+5px those values are <10px and cannot be painted. The bar style image contains a gradient pattern along with transparent shadows for the bar margins. The squeezing will distort it and user will see a different pattern. That was the point of bug. --Semyon It seems that for the small progress bar values (1, 2, 3, 4, 5, 6) in the test the progress is not shown after the fix. Is it possible to draw something correctly for these small values? Thanks, Alexandr. Thanks, --Semyon
[9] Review Request for 8075314: All the InternalFrames will be maximized after maximizing only one of the InternalFrame with the special options "-client -Xmixed -Dswing.defaultlaf=com.sun
Hello, please review JDK9 fix webrev: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8075314/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-8075314 Thanks! --Semyon
[9] Review Request for 6921687: Mnemonic disappears after repeated attempts to open menu items using mnemonics
Hello, Please review JDK9 fix. webrev: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/6921687/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-6921687 Thanks! --Semyon
[9] Review Request for 6866751: J2SE_Swing_Reg: the caret disappears when moving to the end of the line.
Hello, Please review JDK9 fix. webrev: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/6866751/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-6866751 Thanks! --Semyon
Re: [9] Review Request for 6921687: Mnemonic disappears after repeated attempts to open menu items using mnemonics
+description When ALT PRESSED event comes for the second time it can be the case to clear menu focus and srokes or the case when ALT+key combination is about to be pressed. The last one was missed in the ALT key handler. On 3/23/2015 2:22 PM, Semyon Sadetsky wrote: Hello, Please review JDK9 fix. webrev: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/6921687/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-6921687 Thanks! --Semyon
Re: [9] Review Request for 8075314: All the InternalFrames will be maximized after maximizing only one of the InternalFrame with the special options "-client -Xmixed -Dswing.defaultlaf=com
+description A check for the closed state of the previous topmost window is added for the top window selection procedure. On 3/23/2015 1:58 PM, Semyon Sadetsky wrote: Hello, please review JDK9 fix webrev: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8075314/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-8075314 Thanks! --Semyon
Re: [9] Review Request for 6866751: J2SE_Swing_Reg: the caret disappears when moving to the end of the line.
+description Cursor size is added to any text control preferred width. On 3/23/2015 2:33 PM, Semyon Sadetsky wrote: Hello, Please review JDK9 fix. webrev: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/6866751/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-6866751 Thanks! --Semyon
Re: [9] Review Request for 6866751: J2SE_Swing_Reg: the caret disappears when moving to the end of the line.
Hi Sergey, On 3/23/2015 5:41 PM, Sergey Bylokhov wrote: Hi, Semyon. It seems that "Caret.width" is platform specific property, right? (looks like it is initialized on windows only). It can be set on any platform. If it is not set we take 1px which is the default cursor width. What happens if the user will set the size/aspect of the caret via "putClientProperty" after the fix? It depends on specific Cursor implementation. It seems that correct width of the caret can be obtained via DefaultCaret.getCaretWidth (It takes into account both properties: aspectRatio/caretWidth). Probably we can push implementation of this method to the Caret class itself(in jdk9) or at least share the code somehow? Note that we can initialize caretWidth for each L&F by default to 1(or to Caret.width) if this value is correct, so the user will be able take current value which is used by Swing. DefaultCaret.getCaretWidth(height) has package visibility. DefaultCaret implementation can be retired by API JTextComponet.setCaret(Caret). So what you've proposed is not a complete solution as well. An introduction a new method in the Caret interface is too drastic change. And I'm not sure that font height can be the only parameter for all possible Caret implementations. 1px seems a good solution for the issue because caret width=1px in 99.9% cases. Currently under the System LnF we don't see this 1px caret at all on platforms. That solution will be a great improvment. For specific situations like custom caret aspect ratio and custom caret shape I'm not sure. If we add the full right text padding for such situations users who already use custom carets may not expect such behavior. It can be too noticeable change when all text components will receive large right padding. --Semyon 23.03.15 14:57, Semyon Sadetsky wrote: +description Cursor size is added to any text control preferred width. On 3/23/2015 2:33 PM, Semyon Sadetsky wrote: Hello, Please review JDK9 fix. webrev: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/6866751/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-6866751 Thanks! --Semyon
Re: [9] Review Request for 6921687: Mnemonic disappears after repeated attempts to open menu items using mnemonics
On 3/23/2015 5:36 PM, Alexander Scherbatiy wrote: On 3/23/2015 2:22 PM, Semyon Sadetsky wrote: Hello, Please review JDK9 fix. webrev: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/6921687/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-6921687 - What was the reason that mnemonics were hidden between alt press and release key events? - Could you format 'if' condition to have proper space indentation? The reason is that in Windows if ALT is pressed for the second time mnemonics are hidden. But if one press a char key then or keep ALT pressed for a while the mnemonics are shown again. --Semyon Thanks, Alexandr. Thanks! --Semyon
Re: [9] Review Request for 8075314: All the InternalFrames will be maximized after maximizing only one of the InternalFrame with the special options "-client -Xmixed -Dswing.defaultlaf=com
Hi Sergey, I extended the test to iterate through all available LnFs. The updated webrev: http://cr.openjdk.java.net/~ssadetsky/8075314/webrev.01/ --Semyon On 3/24/2015 2:35 PM, Sergey Bylokhov wrote: Hi, Semyon. The fix looks good. Since this is a jck issue I assume that this functionality was tested on all supported look and feels and the new test always passed. So I suggest to change the test and cover all l&f, to be sure that this bug will not be integrated back in some other/new looks and feels. 23.03.15 13:58, Semyon Sadetsky wrote: Hello, please review JDK9 fix webrev: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/8075314/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-8075314 Thanks! --Semyon
Re: [9] Review Request for 6866751: J2SE_Swing_Reg: the caret disappears when moving to the end of the line.
Hi Sergey, I have added reading of the client property. It will also allow user to adjust the right margin for caret. The test scenario is also extended. the updated webrev: http://cr.openjdk.java.net/~ssadetsky/6866751/webrev.01/ --Semyon On 3/24/2015 9:10 AM, Semyon Sadetsky wrote: Hi Sergey, On 3/23/2015 5:41 PM, Sergey Bylokhov wrote: Hi, Semyon. It seems that "Caret.width" is platform specific property, right? (looks like it is initialized on windows only). It can be set on any platform. If it is not set we take 1px which is the default cursor width. What happens if the user will set the size/aspect of the caret via "putClientProperty" after the fix? It depends on specific Cursor implementation. It seems that correct width of the caret can be obtained via DefaultCaret.getCaretWidth (It takes into account both properties: aspectRatio/caretWidth). Probably we can push implementation of this method to the Caret class itself(in jdk9) or at least share the code somehow? Note that we can initialize caretWidth for each L&F by default to 1(or to Caret.width) if this value is correct, so the user will be able take current value which is used by Swing. DefaultCaret.getCaretWidth(height) has package visibility. DefaultCaret implementation can be retired by API JTextComponet.setCaret(Caret). So what you've proposed is not a complete solution as well. An introduction a new method in the Caret interface is too drastic change. And I'm not sure that font height can be the only parameter for all possible Caret implementations. 1px seems a good solution for the issue because caret width=1px in 99.9% cases. Currently under the System LnF we don't see this 1px caret at all on platforms. That solution will be a great improvment. For specific situations like custom caret aspect ratio and custom caret shape I'm not sure. If we add the full right text padding for such situations users who already use custom carets may not expect such behavior. It can be too noticeable change when all text components will receive large right padding. --Semyon 23.03.15 14:57, Semyon Sadetsky wrote: +description Cursor size is added to any text control preferred width. On 3/23/2015 2:33 PM, Semyon Sadetsky wrote: Hello, Please review JDK9 fix. webrev: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/6866751/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-6866751 Thanks! --Semyon
Re: Review request for 6894632: Removing rows from a DefaultTableModel with a RowSorter deselectes last row
Hi Sergey, the ticket to correct the spec is created https://bugs.openjdk.java.net/browse/JDK-8076474 --Semyon On 3/26/2015 2:35 AM, Sergey Bylokhov wrote: Hi, Semyon. There is a problem which we discussed offline already. This change fix the problem described in the bug report, but it contradicts the specification of this method, which state that tableChanged should clears the selection if any. So the specification should be updated also via ccc: in this CR if you have no plan to backport it to jdk8, or in separate CR if you plan to backport it to jdk8. 06.03.15 15:30, Semyon Sadetsky wrote: Hi Sergey, Good catch! Yes it should be inverted. I fix that and added more extensive test scenarios to double check different table states. Please review. It's here: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/6894632/webrev.02/ Thanks! Semyon On 3/6/2015 1:26 AM, Sergey Bylokhov wrote: Hi, Semyon . Are you sure that this new if-statement is correct? it seems that it should be opposite. No? On 04.03.2015 13:45, Alexander Scherbatiy wrote: The fix looks good to me. Thanks, Alexandr. On 3/4/2015 12:29 PM, Semyon Sadetsky wrote: No, because it wouldn't be a valid state. The primary sort column is always the first. If primary is unsorted then there are no more sorted columns. On 3/4/2015 11:54 AM, Alexander Scherbatiy wrote: Is it possible that getSortKeys() methods returns some sort keys where the first has unsorted sort order but others do not. Thanks, Alexandr. On 3/2/2015 5:47 PM, Semyon Sadetsky wrote: Hello, Please review the fix: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/6894632/webrev.01/ for bug: https://bugs.openjdk.java.net/browse/JDK-6894632 The thing is the sorter logic uses its internal data structures to restore selection when the table model has been changed. For performance reason this internal data is created only upon the table sort call. If sortable JTable is initialized with no sort order specified the table sort is not executed and table remains unsorted as JTable without sorting capability. When the model of table in such state is changed the corresponding event handler logic still called the sorter to transform selection. But since the sorter internal cache was empty the transformation failed in cases when the selection index exceeds the new table rows count. Now with the fix the sorter is updated with table model change event only if the table is really sorted.
Re: [9] Review Request for 6866751: J2SE_Swing_Reg: the caret disappears when moving to the end of the line.
Hi Alexander,
The case you've described means user overrides the default behavior but
sets an incorrect value. In my opinion it is ok if the value fallbacks
to the default in this scenario.
Imagine the situation when user wants to have the same default right
margin in all platforms (currently caret.width is set only in Windows)
he can achieve that by setting client property to negative value and
then the internal default value will be used everywhere. Without that
possibility he would need to set the specific value explicitly.
--Semyon
On 4/9/2015 11:44 AM, Alexander Scherbatiy wrote:
On 4/1/2015 5:45 PM, Semyon Sadetsky wrote:
Hi Sergey,
I have added reading of the client property. It will also allow user
to adjust the right margin for caret. The test scenario is also
extended.
the updated webrev:
http://cr.openjdk.java.net/~ssadetsky/6866751/webrev.01/
If c.getClientProperty("caretWidth") is negative should
UIManager.get("Caret.width") or DEFAULT_CARET_MARGIN be used?
Thanks,
Alexandr.
--Semyon
On 3/24/2015 9:10 AM, Semyon Sadetsky wrote:
Hi Sergey,
On 3/23/2015 5:41 PM, Sergey Bylokhov wrote:
Hi, Semyon.
It seems that "Caret.width" is platform specific property, right?
(looks like it is initialized on windows only).
It can be set on any platform. If it is not set we take 1px which is
the default cursor width.
What happens if the user will set the size/aspect of the caret via
"putClientProperty" after the fix?
It depends on specific Cursor implementation.
It seems that correct width of the caret can be obtained via
DefaultCaret.getCaretWidth (It takes into account both properties:
aspectRatio/caretWidth). Probably we can push implementation of
this method to the Caret class itself(in jdk9) or at least share
the code somehow? Note that we can initialize caretWidth for each
L&F by default to 1(or to Caret.width) if this value is correct, so
the user will be able take current value which is used by Swing.
DefaultCaret.getCaretWidth(height) has package visibility.
DefaultCaret implementation can be retired by API
JTextComponet.setCaret(Caret). So what you've proposed is not a
complete solution as well.
An introduction a new method in the Caret interface is too drastic
change. And I'm not sure that font height can be the only parameter
for all possible Caret implementations.
1px seems a good solution for the issue because caret width=1px in
99.9% cases. Currently under the System LnF we don't see this 1px
caret at all on platforms. That solution will be a great improvment.
For specific situations like custom caret aspect ratio and custom
caret shape I'm not sure. If we add the full right text padding for
such situations users who already use custom carets may not expect
such behavior. It can be too noticeable change when all text
components will receive large right padding.
--Semyon
23.03.15 14:57, Semyon Sadetsky wrote:
+description
Cursor size is added to any text control preferred width.
On 3/23/2015 2:33 PM, Semyon Sadetsky wrote:
Hello,
Please review JDK9 fix.
webrev:
http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/6866751/webrev.00/
bug: https://bugs.openjdk.java.net/browse/JDK-6866751
Thanks!
--Semyon
[9] Review Request for 6866751:
Hello, Please review fix for JDK9: bug: https://bugs.openjdk.java.net/browse/JDK-7072653 webrev: http://cr.openjdk.java.net/~ssadetsky/7072653/webrev.00/ *THE ROOT CAUSE Incorrect bounds calculation of heavy weight popup window in the BasicComboPopup when the requested popup menu height exceeds screen height. Additional issue found: popup border thickness is not taken into account when popup direction switched to up as the result popup overlaps its combo-box by 2 pixels. Also in multi-monitor desktop if screen are arranged vertically popup can be shown on another monitor. *SOLUTION The popup location algorithm is corrected to take into account the current screen height and border insets. *TESTING Test is added to ensure popup window height when combo-box contains 1000 items. --Semyon
Re: [9] Review Request for 7072653: JComboBox popup mispositioned if its height exceeds the screen height
Subject corrected. On 4/16/2015 3:55 PM, Semyon Sadetsky wrote: Hello, Please review fix for JDK9: bug: https://bugs.openjdk.java.net/browse/JDK-7072653 webrev: http://cr.openjdk.java.net/~ssadetsky/7072653/webrev.00/ *THE ROOT CAUSE Incorrect bounds calculation of heavy weight popup window in the BasicComboPopup when the requested popup menu height exceeds screen height. Additional issue found: popup border thickness is not taken into account when popup direction switched to up as the result popup overlaps its combo-box by 2 pixels. Also in multi-monitor desktop if screen are arranged vertically popup can be shown on another monitor. *SOLUTION The popup location algorithm is corrected to take into account the current screen height and border insets. *TESTING Test is added to ensure popup window height when combo-box contains 1000 items. --Semyon
[9] Review Request for 7172652: With JDK 1.7 text field does not obtain focus when using mnemonic Alt/Key combin
Hello, please review a fix for JDK9: webrev: http://cr.openjdk.java.net/~ssadetsky/7172652/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-7172652 ***ROOT CAUSE This is a bug in Label UI's key release event processing routine for key mnemonics: only one release event is captured instead of two (Alt release and the mnemonic key release). The Alt release event goes up on hierarchy and is captured by the parent internal frame's menu bar for which Alt key release means selection change event under the Windows system LnF. ***SOLUTION Change key release event handling logic to capture events from both Alt modifier and the key. The logic takes into account that when the first release key event come it transfers focus back to the field so the second key release event should be captured from any window component. ***TESTING A simple scenario is written to exclusively cover the situation. --Semyon
[9] Review Request for 7190544: Nimbus LaF: regression UnitTest failure
Hello, Please review JDK9 fix: webrev: http://cr.openjdk.java.net/~ssadetsky/7190544/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-7190544 ***ROOT CAUSE The divider size field is not set for the split pane UI during Synth style initialization. ***SOLUTION Set the field. ***TESTING Not need. This is a reg test failure. --Semyon
Re: [9] Review Request for 7072653: JComboBox popup mispositioned if its height exceeds the screen height
Hi Alexander, Thanks for the review. The updated webrev: http://cr.openjdk.java.net/~ssadetsky/7072653/webrev.01/ --Semyon On 4/21/2015 5:17 PM, Alexander Zvegintsev wrote: Hello Semyon, it looks like that second call to getBorder() is unnecessary, we already have its result in popupBorder variable. borderHeight looks strange to me, it is initialized with left and right insets(related to width) and then used as height. The test uses full screen bounds and does not subtracts screen insets. Thanks, Alexander. On 04/16/2015 04:14 PM, Semyon Sadetsky wrote: Subject corrected. On 4/16/2015 3:55 PM, Semyon Sadetsky wrote: Hello, Please review fix for JDK9: bug: https://bugs.openjdk.java.net/browse/JDK-7072653 webrev: http://cr.openjdk.java.net/~ssadetsky/7072653/webrev.00/ *THE ROOT CAUSE Incorrect bounds calculation of heavy weight popup window in the BasicComboPopup when the requested popup menu height exceeds screen height. Additional issue found: popup border thickness is not taken into account when popup direction switched to up as the result popup overlaps its combo-box by 2 pixels. Also in multi-monitor desktop if screen are arranged vertically popup can be shown on another monitor. *SOLUTION The popup location algorithm is corrected to take into account the current screen height and border insets. *TESTING Test is added to ensure popup window height when combo-box contains 1000 items. --Semyon
Re: [9] Review Request for 7072653: JComboBox popup mispositioned if its height exceeds the screen height
Updated fix: http://cr.openjdk.java.net/~ssadetsky/7072653/webrev.02/
(Aqua LnF had own logic need to be fixed as well as it was noticed by
Alexander Z.)
Alexander,
This is because the screenBounds are in relative coordinates and we need
to check if there are enough space to show popup above the combo.
--Semyon
On 4/22/2015 2:33 PM, Alexander Scherbatiy wrote:
On 4/21/2015 5:31 PM, Semyon Sadetsky wrote:
Hi Alexander,
Thanks for the review.
The updated webrev:
http://cr.openjdk.java.net/~ssadetsky/7072653/webrev.01/
1299 if (py + ph > screenBounds.y + screenBounds.height) {
1300 if (ph <= -screenBounds.y - borderHeight) {
Is it correct to compare the popup height with the screenBounds.y on
the line 1300?
Thanks,
Alexandr.
--Semyon
On 4/21/2015 5:17 PM, Alexander Zvegintsev wrote:
Hello Semyon,
it looks like that second call to getBorder() is unnecessary, we
already have its result in popupBorder variable.
borderHeight looks strange to me, it is initialized with left and
right insets(related to width) and then used as height.
The test uses full screen bounds and does not subtracts screen insets.
Thanks,
Alexander.
On 04/16/2015 04:14 PM, Semyon Sadetsky wrote:
Subject corrected.
On 4/16/2015 3:55 PM, Semyon Sadetsky wrote:
Hello,
Please review fix for JDK9:
bug: https://bugs.openjdk.java.net/browse/JDK-7072653
webrev: http://cr.openjdk.java.net/~ssadetsky/7072653/webrev.00/
*THE ROOT CAUSE
Incorrect bounds calculation of heavy weight popup window in the
BasicComboPopup when the requested popup menu height exceeds
screen height.
Additional issue found: popup border thickness is not taken into
account when popup direction switched to up as the result popup
overlaps its combo-box by 2 pixels.
Also in multi-monitor desktop if screen are arranged vertically
popup can be shown on another monitor.
*SOLUTION
The popup location algorithm is corrected to take into account the
current screen height and border insets.
*TESTING
Test is added to ensure popup window height when combo-box
contains 1000 items.
--Semyon
[9] Review Request for 6260348: GTK+ L&F JTextComponent not respecting desktop caret blink rate
Hello, Please review fix for JDK9: web rev: http://cr.openjdk.java.net/~ssadetsky/6260348/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-6260348 ***ROOT CAUSE GTK L&F never took care about native GTK cursor blink settings. ***SOLUTION Implement this feature. ***TESTING Two test scenarios added. 1st sets GTK cursor blink enabled and blink time 200 ms. 2nd disables cursor blinking. Then Java text caret is checked to respect these settings. --Semyon
[9] Review Request for 8078483: Apparent endless loop running JEditorPanePaintTest
Hello, Please review fix for JDK9: webrev: http://cr.openjdk.java.net/~ssadetsky/8078483/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-8078483 ***ROOT CASE Text component root view should receive the component dimension without margins. The caret margin introduced in https://bugs.openjdk.java.net/browse/JDK-6866751 fix was not taken into account. ***SOLUTION Extract caret margin from the dimension width. ***TESTING No extra test is needed. This is a reg test failure issue. --Semyon
[9] Review Request for 8001470: JTextField's size is computed incorrectly when it contains Indic or Thai characters
Hello, Please review JDK9 fix. webrev: http://cr.openjdk.java.net/~ssadetsky/8001470/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-8001470 ***ROOT CAUSE The setVisible() call without pack() or setBounds() means set the frame size to the initial window size. The initial window size usually is the size to fit the window frame and title only, so the content pane receives some width and zero height. Since GridLayout does not limit minimum component size when doing layout under fixed size, the components receive zero height. This situation we have right after the setVisible(true) call. Now we do call our pack() call to do layout of the fame which is already visible. The pack() triggers getPreferedSize() calls for components. Calculation of TextFileld preferred size is performed in its View which setSize() method should be called. For an i18n text filed the view is a complex data structure which requires the right initialization. According to line 942 of the BasicTextUI class the view initialization happens only when both component's width and height equal to 0, which is an expected situation for the initial layout call. But in our specific pack() call this never happens because while the height is actually equals 0 the width is not, because component received such size as the result of the previous frame.setVisible() call. So the view.setSize() method is not called at all and preferred size is calculated incorrectly, the zero height is obtained particularly. ***SOLUTION As a fix it is proposed to change condition in line 942 of the BasicTextUI to OR to cover the scenario. ***TESTING A test case for the scenario is added. --Semyon
Re: [9] Review Request for 7072653: JComboBox popup mispositioned if its height exceeds the screen height
On 4/27/2015 4:58 PM, Alexander Scherbatiy wrote:
On 4/23/2015 2:02 PM, Semyon Sadetsky wrote:
Updated fix: http://cr.openjdk.java.net/~ssadetsky/7072653/webrev.02/
(Aqua LnF had own logic need to be fixed as well as it was noticed by
Alexander Z.)
Alexander,
This is because the screenBounds are in relative coordinates and we
need to check if there are enough space to show popup above the combo.
The screenBoundsare created in two ways: gc.getBounds() and new
Rectangle(p, toolkit.getScreenSize()).
The javadoc for the getBounds() claims: "In a multi-screen
environment with a virtual device, the bounds can have negative X or Y
origins."
Does your solution work both for the positive and negative Y value?
Thanks,
Alexandr.
Yes, it now works for muli-monitor env. I specially pointed that in the
description.
Did you find any issues?
--Semyon
--Semyon
On 4/22/2015 2:33 PM, Alexander Scherbatiy wrote:
On 4/21/2015 5:31 PM, Semyon Sadetsky wrote:
Hi Alexander,
Thanks for the review.
The updated webrev:
http://cr.openjdk.java.net/~ssadetsky/7072653/webrev.01/
1299 if (py + ph > screenBounds.y + screenBounds.height) {
1300 if (ph <= -screenBounds.y - borderHeight) {
Is it correct to compare the popup height with the screenBounds.y
on the line 1300?
Thanks,
Alexandr.
--Semyon
On 4/21/2015 5:17 PM, Alexander Zvegintsev wrote:
Hello Semyon,
it looks like that second call to getBorder() is unnecessary, we
already have its result in popupBorder variable.
borderHeight looks strange to me, it is initialized with left and
right insets(related to width) and then used as height.
The test uses full screen bounds and does not subtracts screen
insets.
Thanks,
Alexander.
On 04/16/2015 04:14 PM, Semyon Sadetsky wrote:
Subject corrected.
On 4/16/2015 3:55 PM, Semyon Sadetsky wrote:
Hello,
Please review fix for JDK9:
bug: https://bugs.openjdk.java.net/browse/JDK-7072653
webrev: http://cr.openjdk.java.net/~ssadetsky/7072653/webrev.00/
*THE ROOT CAUSE
Incorrect bounds calculation of heavy weight popup window in the
BasicComboPopup when the requested popup menu height exceeds
screen height.
Additional issue found: popup border thickness is not taken into
account when popup direction switched to up as the result popup
overlaps its combo-box by 2 pixels.
Also in multi-monitor desktop if screen are arranged vertically
popup can be shown on another monitor.
*SOLUTION
The popup location algorithm is corrected to take into account
the current screen height and border insets.
*TESTING
Test is added to ensure popup window height when combo-box
contains 1000 items.
--Semyon
[9] Review Request for 8079640: GroupLayout incorrect layout with large JTextArea
Hello, Please review fix for JDK9: bug: https://bugs.openjdk.java.net/browse/JDK-8079640 webrev: http://cr.openjdk.java.net/~ssadetsky/8079640/webrev.00/ *THE ROOT CAUSE Component minimum size is limited to Short.MAX_VALUE in GroupLayout. JDK turtorial https://docs.oracle.com/javase/tutorial/uiswing/layout/group.html tells that Short.MAX_VALUE is treated as infinite. But I did not find any reasons in JDK specs why a bigger number cannot be used. *SOLUTION Remove the limitation *TESTING Test is added to cover the large component scenario. Existing GroupLayout tests are passed. --Semyon
Re: [9] Review Request for 6260348: GTK+ L&F JTextComponent not respecting desktop caret blink rate
Hi Alexander, gboolean and gint are equivalent. --Semyon On 4/27/2015 5:31 PM, Alexander Scherbatiy wrote: On 4/23/2015 5:23 PM, Semyon Sadetsky wrote: Hello, Please review fix for JDK9: web rev: http://cr.openjdk.java.net/~ssadetsky/6260348/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-6260348 gtk2_interface.c 2510 gint intval = NULL; 2511 (*fp_g_object_get)(settings, key, &intval, NULL); 2512 return create_Boolean(env, intval); Probably the type of the value should be gboolean. Thanks, Alexandr. ***ROOT CAUSE GTK L&F never took care about native GTK cursor blink settings. ***SOLUTION Implement this feature. ***TESTING Two test scenarios added. 1st sets GTK cursor blink enabled and blink time 200 ms. 2nd disables cursor blinking. Then Java text caret is checked to respect these settings. --Semyon
Re: [9] Review Request for 7172652: With JDK 1.7 text field does not obtain focus when using mnemonic Alt/Key combin
updated: http://cr.openjdk.java.net/~ssadetsky/7172652/webrev.01/ On 4/28/2015 4:31 PM, Alexander Scherbatiy wrote: Is it possible to make code shorter by adding methods like: putOnRelease(InputMap inputMap, int keyCode, int modifiers) removeOnRelease(InputMap inputMap, int keyCode, int modifiers) Thanks, Alexandr. On 4/20/2015 5:53 PM, Semyon Sadetsky wrote: Hello, please review a fix for JDK9: webrev: http://cr.openjdk.java.net/~ssadetsky/7172652/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-7172652 ***ROOT CAUSE This is a bug in Label UI's key release event processing routine for key mnemonics: only one release event is captured instead of two (Alt release and the mnemonic key release). The Alt release event goes up on hierarchy and is captured by the parent internal frame's menu bar for which Alt key release means selection change event under the Windows system LnF. ***SOLUTION Change key release event handling logic to capture events from both Alt modifier and the key. The logic takes into account that when the first release key event come it transfers focus back to the field so the second key release event should be captured from any window component. ***TESTING A simple scenario is written to exclusively cover the situation. --Semyon
Re: [9] Review Request for 7172652: With JDK 1.7 text field does not obtain focus when using mnemonic Alt/Key combin
Alexander, thanks. all is relevant. http://cr.openjdk.java.net/~ssadetsky/7172652/webrev.02/ --Semyon On 5/13/2015 6:06 PM, Alexander Scherbatiy wrote: On 5/8/2015 12:13 PM, Semyon Sadetsky wrote: updated: http://cr.openjdk.java.net/~ssadetsky/7172652/webrev.01/ - Should the labelFor request focus in the method BasicLabelUI.doRelease(...) as it was before the fix - There are some formatting problems (space after a method argument on line 507, bracket on line 534) - It seems that inputMap.put() in doPress() method also can be changed to putOnRelease() - frame.dispose() also should be called on EDT in the test Thanks, Alexandr. On 4/28/2015 4:31 PM, Alexander Scherbatiy wrote: Is it possible to make code shorter by adding methods like: putOnRelease(InputMap inputMap, int keyCode, int modifiers) removeOnRelease(InputMap inputMap, int keyCode, int modifiers) Thanks, Alexandr. On 4/20/2015 5:53 PM, Semyon Sadetsky wrote: Hello, please review a fix for JDK9: webrev: http://cr.openjdk.java.net/~ssadetsky/7172652/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-7172652 ***ROOT CAUSE This is a bug in Label UI's key release event processing routine for key mnemonics: only one release event is captured instead of two (Alt release and the mnemonic key release). The Alt release event goes up on hierarchy and is captured by the parent internal frame's menu bar for which Alt key release means selection change event under the Windows system LnF. ***SOLUTION Change key release event handling logic to capture events from both Alt modifier and the key. The logic takes into account that when the first release key event come it transfers focus back to the field so the second key release event should be captured from any window component. ***TESTING A simple scenario is written to exclusively cover the situation. --Semyon
[9] Review Request for 8078269: JTabbedPane UI Property TabbedPane.tabAreaBackground no longer works
Hello, Please review fix for JDK9: bug: https://bugs.openjdk.java.net/browse/JDK-8078269 webrev: http://cr.openjdk.java.net/~ssadetsky/8078269/webrev.00/ This is regression of the 8007563 which was incorrect fix of the 4690946 regression. Actually the 4690946 was a test bug because tab area's background can be controlled separately using "TabbedPane.tabAreaBackground" property. Presence of "TabbedPane.tabAreaBackground" depends on L&F. Test of the 4690946 is fixed to take this into account. Also inappropriate design chosen for the 8007563 reg test code affected test results stability. It also has been fixed. --Semyon
Re: [9] Review Request for 8078269: JTabbedPane UI Property TabbedPane.tabAreaBackground no longer works
Sergey, Why the component background priority looks reasonable for you? A component has only one background color which is set by L&F to the default value. But it is not enough for the tabbed pane component which has more then one background surfaces and L&F can define more background colors to paint tabbed pane more precisely. In this case the common component background should have second priority. --Semyon On 5/14/2015 5:39 PM, Sergey Bylokhov wrote: Hi, Semyon. Usage of background(if it was set) instead of property looks reasonable. Please clarify who sets default color of component explicitly to the value other than from UI property? On 14.05.15 17:12, Semyon Sadetsky wrote: Hello, Please review fix for JDK9: bug: https://bugs.openjdk.java.net/browse/JDK-8078269 webrev: http://cr.openjdk.java.net/~ssadetsky/8078269/webrev.00/ This is regression of the 8007563 which was incorrect fix of the 4690946 regression. Actually the 4690946 was a test bug because tab area's background can be controlled separately using "TabbedPane.tabAreaBackground" property. Presence of "TabbedPane.tabAreaBackground" depends on L&F. Test of the 4690946 is fixed to take this into account. Also inappropriate design chosen for the 8007563 reg test code affected test results stability. It also has been fixed. --Semyon
Re: [9] Review Request for 8078269: JTabbedPane UI Property TabbedPane.tabAreaBackground no longer works
On 5/15/2015 2:06 PM, Sergey Bylokhov wrote: On 14.05.15 18:00, Semyon Sadetsky wrote: Sergey, Why the component background priority looks reasonable for you? Because otherwise there is no way to change the color of one particular component. A component has only one background color which is set by L&F to the default value. But it is not enough for the tabbed pane component which has more then one background surfaces and L&F can define more background colors to paint tabbed pane more precisely. In this case the common component background should have second priority. What is the difference between "TabbedPane.background" and "TabbedPane.tabAreaBackground" in the metal l&f? "TabbedPane.background" is the default background. --Semyon On 5/14/2015 5:39 PM, Sergey Bylokhov wrote: Hi, Semyon. Usage of background(if it was set) instead of property looks reasonable. Please clarify who sets default color of component explicitly to the value other than from UI property? On 14.05.15 17:12, Semyon Sadetsky wrote: Hello, Please review fix for JDK9: bug: https://bugs.openjdk.java.net/browse/JDK-8078269 webrev: http://cr.openjdk.java.net/~ssadetsky/8078269/webrev.00/ This is regression of the 8007563 which was incorrect fix of the 4690946 regression. Actually the 4690946 was a test bug because tab area's background can be controlled separately using "TabbedPane.tabAreaBackground" property. Presence of "TabbedPane.tabAreaBackground" depends on L&F. Test of the 4690946 is fixed to take this into account. Also inappropriate design chosen for the 8007563 reg test code affected test results stability. It also has been fixed. --Semyon -- Best regards, Sergey.
[9] Review Request for 8078514: Nightly: api/javax_swing/DefaultRowSorter/index_ModelStructChanged failure
Hello, Please review fix for JDK9: bug: https://bugs.openjdk.java.net/browse/JDK-8078514 webrev: http://cr.openjdk.java.net/~ssadetsky/8078514/webrev.00/ The 6894632 fix violated a contract between the table and its row sorter: the sorter should receive TableChanged events even if table is not sorted actually. Another way to fix 6894632 is to initialize sorter internal structures instantly. The last is applied in the fix. --Semyon
Re: [9] Review Request for 8079640: GroupLayout incorrect layout with large JTextArea
Sergey, I have dug into the SCCS history. This was set initially and was not related to any issues. --Semyon On 5/8/2015 4:07 PM, Sergey Bylokhov wrote: Hi, Semyon. It will be good to dig into the history of GroupLayout and understand why this was constrained, note that tutorial should be updated also. On 08.05.15 11:47, Semyon Sadetsky wrote: Hello, Please review fix for JDK9: bug: https://bugs.openjdk.java.net/browse/JDK-8079640 webrev: http://cr.openjdk.java.net/~ssadetsky/8079640/webrev.00/ *THE ROOT CAUSE Component minimum size is limited to Short.MAX_VALUE in GroupLayout. JDK turtorial https://docs.oracle.com/javase/tutorial/uiswing/layout/group.html tells that Short.MAX_VALUE is treated as infinite. But I did not find any reasons in JDK specs why a bigger number cannot be used. *SOLUTION Remove the limitation *TESTING Test is added to cover the large component scenario. Existing GroupLayout tests are passed. --Semyon
Re: [9] Review Request for 8079640: GroupLayout incorrect layout with large JTextArea
JDK-8080604 created. On 5/18/2015 4:20 PM, Sergey Bylokhov wrote: Looks fine then. Please file a separate issue to update tutorial. On 18.05.15 15:19, Semyon Sadetsky wrote: Sergey, I have dug into the SCCS history. This was set initially and was not related to any issues. --Semyon On 5/8/2015 4:07 PM, Sergey Bylokhov wrote: Hi, Semyon. It will be good to dig into the history of GroupLayout and understand why this was constrained, note that tutorial should be updated also. On 08.05.15 11:47, Semyon Sadetsky wrote: Hello, Please review fix for JDK9: bug: https://bugs.openjdk.java.net/browse/JDK-8079640 webrev: http://cr.openjdk.java.net/~ssadetsky/8079640/webrev.00/ *THE ROOT CAUSE Component minimum size is limited to Short.MAX_VALUE in GroupLayout. JDK turtorial https://docs.oracle.com/javase/tutorial/uiswing/layout/group.html tells that Short.MAX_VALUE is treated as infinite. But I did not find any reasons in JDK specs why a bigger number cannot be used. *SOLUTION Remove the limitation *TESTING Test is added to cover the large component scenario. Existing GroupLayout tests are passed. --Semyon
Re: [9] Review Request for 8079640: GroupLayout incorrect layout with large JTextArea
Alexander, thanks that's true. http://cr.openjdk.java.net/~ssadetsky/8079640/webrev.01/ On 5/19/2015 2:29 PM, Alexander Scherbatiy wrote: The SequentialGroup.operator(int a, int b) method returns constrain(a) + constrain(b). If there are no any restriction to the constrain() return value it can leads to the integer overflow. May be Integer.MAX_VALUE / 2 can be used instead of Short.MAX_VALUE? Thanks, Alexandr. On 5/18/2015 3:19 PM, Semyon Sadetsky wrote: Sergey, I have dug into the SCCS history. This was set initially and was not related to any issues. --Semyon On 5/8/2015 4:07 PM, Sergey Bylokhov wrote: Hi, Semyon. It will be good to dig into the history of GroupLayout and understand why this was constrained, note that tutorial should be updated also. On 08.05.15 11:47, Semyon Sadetsky wrote: Hello, Please review fix for JDK9: bug: https://bugs.openjdk.java.net/browse/JDK-8079640 webrev: http://cr.openjdk.java.net/~ssadetsky/8079640/webrev.00/ *THE ROOT CAUSE Component minimum size is limited to Short.MAX_VALUE in GroupLayout. JDK turtorial https://docs.oracle.com/javase/tutorial/uiswing/layout/group.html tells that Short.MAX_VALUE is treated as infinite. But I did not find any reasons in JDK specs why a bigger number cannot be used. *SOLUTION Remove the limitation *TESTING Test is added to cover the large component scenario. Existing GroupLayout tests are passed. --Semyon
Re: [9] Review Request for 6260348: GTK+ L&F JTextComponent not respecting desktop caret blink rate
Hi Alexander, I agree. http://cr.openjdk.java.net/~ssadetsky/6260348/webrev.01/ --Semyon On 5/8/2015 5:04 PM, Alexander Zvegintsev wrote: Hi Semyon, the fix itself looks good to me, but the test doesn't work. 1. FileWriter is replacing the .gtkrc-2.0 file (FileWriter(file, true /*append*/) should be used) ; 2. settings from .gtkrc-2.0 doesn't taken into account on my system. there are at least two system tools which allows to modify such values: gconftool-2 and gsettings. One of them may lack on different systems. this will make test complicated. So I suggest just remove the test and add noreg-hard label to the issue. -- Thanks, Alexander. On 23.04.2015 17:23, Semyon Sadetsky wrote: Hello, Please review fix for JDK9: web rev: http://cr.openjdk.java.net/~ssadetsky/6260348/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-6260348 ***ROOT CAUSE GTK L&F never took care about native GTK cursor blink settings. ***SOLUTION Implement this feature. ***TESTING Two test scenarios added. 1st sets GTK cursor blink enabled and blink time 200 ms. 2nd disables cursor blinking. Then Java text caret is checked to respect these settings. --Semyon
Re: [9] Review Request for 8079640: GroupLayout incorrect layout with large JTextArea
Hi Alexander, This definition is mentioned in the tutorial. I think it would be good to make this constant public eventually because it is often used in various the GroupLayout calls as a parameter. --Semyon On 5/20/2015 3:34 PM, Alexander Zvegintsev wrote: Hi Semyon, I think that INFINITE is a bit inaccurate name. Personally, I think that there is no need in extra variable here. Why just not inline MAX_VALUE / 2 (with comment about integer overflow) into constrain()? Thanks, Alexander. On 05/19/2015 05:38 PM, Semyon Sadetsky wrote: Alexander, thanks that's true. http://cr.openjdk.java.net/~ssadetsky/8079640/webrev.01/ On 5/19/2015 2:29 PM, Alexander Scherbatiy wrote: The SequentialGroup.operator(int a, int b) method returns constrain(a) + constrain(b). If there are no any restriction to the constrain() return value it can leads to the integer overflow. May be Integer.MAX_VALUE / 2 can be used instead of Short.MAX_VALUE? Thanks, Alexandr. On 5/18/2015 3:19 PM, Semyon Sadetsky wrote: Sergey, I have dug into the SCCS history. This was set initially and was not related to any issues. --Semyon On 5/8/2015 4:07 PM, Sergey Bylokhov wrote: Hi, Semyon. It will be good to dig into the history of GroupLayout and understand why this was constrained, note that tutorial should be updated also. On 08.05.15 11:47, Semyon Sadetsky wrote: Hello, Please review fix for JDK9: bug: https://bugs.openjdk.java.net/browse/JDK-8079640 webrev: http://cr.openjdk.java.net/~ssadetsky/8079640/webrev.00/ *THE ROOT CAUSE Component minimum size is limited to Short.MAX_VALUE in GroupLayout. JDK turtorial https://docs.oracle.com/javase/tutorial/uiswing/layout/group.html tells that Short.MAX_VALUE is treated as infinite. But I did not find any reasons in JDK specs why a bigger number cannot be used. *SOLUTION Remove the limitation *TESTING Test is added to cover the large component scenario. Existing GroupLayout tests are passed. --Semyon
Re: [9] Review Request for 8078514: Nightly: api/javax_swing/DefaultRowSorter/index_ModelStructChanged failure
Hello, I have decided to remake the fix. The reason for that is sun.swing.FilePane class. One of its inner classes extends DefaultRowSorter and relays on lazy initialization of the DefaultRowSorter in unsorted state. After removing the lazy init the FilePane crashes with AOB exception. This can be fixed, but I think it will be too big change for the issue and users can be already using the DefaultRowSorter in the similar way. Here is the new approach: http://cr.openjdk.java.net/~ssadetsky/8078514/webrev.01/ It looks a little bit risky ,but all related tests have been passed. --Semyon On 5/19/2015 2:03 PM, Alexander Scherbatiy wrote: On 5/15/2015 5:49 PM, Semyon Sadetsky wrote: Hello, Please review fix for JDK9: bug: https://bugs.openjdk.java.net/browse/JDK-8078514 webrev: http://cr.openjdk.java.net/~ssadetsky/8078514/webrev.00/ DefaultRowSorter 221 allChanged(); 222 modelRowCount = getModelWrapper().getRowCount(); - This can be moved to a private method that will be used both in the public modelStructureChanged() and setModelWrapper() methods. 532 public void sort() - Could the rawFilter be null in case viewToModel != null an !isUnsorted() ? - isUnsorted() method is called twice. Is it possible to store its value to a variable and use it? Thanks, Alexandr. The 6894632 fix violated a contract between the table and its row sorter: the sorter should receive TableChanged events even if table is not sorted actually. Another way to fix 6894632 is to initialize sorter internal structures instantly. The last is applied in the fix. --Semyon
[9] Review Request for 8075785: The regression-swing case failed as colored text is not shown on disabled checkbox and radio button with the special options "-client -Dswing.defaultlaf=com
Hello, Please review fix for JDK9: bug: https://bugs.openjdk.java.net/browse/JDK-8075785 webrev: http://cr.openjdk.java.net/~ssadetsky/8075785/webrev.00/ *.disabledText properties are not suported in in Windows Classic L&F & Windows XP L&F. In the fix the properties remain optional but are taken into account if set by user. --Semyon
Re: [9] Review Request for 8075785: The regression-swing case failed as colored text is not shown on disabled checkbox and radio button with the special options "-client -Dswing.defaultlaf
Hi Sergey, As per our offline discussion: we cannot avoid the "instance-of" chain even if we add new properties to the default table. So the advantage of the proposed change: it is minimalistic and does not change the initial design. --Semyon On 5/29/2015 1:24 PM, Sergey Bylokhov wrote: Hi, Semyon. Why not set this properties in the look and feel to the same color as "Button.shadow" and use it WindowsGraphicsUtils? On 28.05.15 18:29, Semyon Sadetsky wrote: Hello, Please review fix for JDK9: bug: https://bugs.openjdk.java.net/browse/JDK-8075785 webrev: http://cr.openjdk.java.net/~ssadetsky/8075785/webrev.00/ *.disabledText properties are not suported in in Windows Classic L&F & Windows XP L&F. In the fix the properties remain optional but are taken into account if set by user. --Semyon
Re: [9] Review Request for 8078269: JTabbedPane UI Property TabbedPane.tabAreaBackground no longer works
Hi Sergey, as agreed I've added UIResource check: http://cr.openjdk.java.net/~ssadetsky/8078269/webrev.01/ --Semyon On 5/15/2015 3:10 PM, Semyon Sadetsky wrote: On 5/15/2015 2:06 PM, Sergey Bylokhov wrote: On 14.05.15 18:00, Semyon Sadetsky wrote: Sergey, Why the component background priority looks reasonable for you? Because otherwise there is no way to change the color of one particular component. A component has only one background color which is set by L&F to the default value. But it is not enough for the tabbed pane component which has more then one background surfaces and L&F can define more background colors to paint tabbed pane more precisely. In this case the common component background should have second priority. What is the difference between "TabbedPane.background" and "TabbedPane.tabAreaBackground" in the metal l&f? "TabbedPane.background" is the default background. --Semyon On 5/14/2015 5:39 PM, Sergey Bylokhov wrote: Hi, Semyon. Usage of background(if it was set) instead of property looks reasonable. Please clarify who sets default color of component explicitly to the value other than from UI property? On 14.05.15 17:12, Semyon Sadetsky wrote: Hello, Please review fix for JDK9: bug: https://bugs.openjdk.java.net/browse/JDK-8078269 webrev: http://cr.openjdk.java.net/~ssadetsky/8078269/webrev.00/ This is regression of the 8007563 which was incorrect fix of the 4690946 regression. Actually the 4690946 was a test bug because tab area's background can be controlled separately using "TabbedPane.tabAreaBackground" property. Presence of "TabbedPane.tabAreaBackground" depends on L&F. Test of the 4690946 is fixed to take this into account. Also inappropriate design chosen for the 8007563 reg test code affected test results stability. It also has been fixed. --Semyon -- Best regards, Sergey.
Re: [9] Review Request for 8078269: JTabbedPane UI Property TabbedPane.tabAreaBackground no longer works
On 6/8/2015 8:33 PM, Sergey Bylokhov wrote: On 08.06.15 20:29, Sergey Bylokhov wrote: On 08.06.15 16:33, Semyon Sadetsky wrote: Hi Sergey, as agreed I've added UIResource check: http://cr.openjdk.java.net/~ssadetsky/8078269/webrev.01/ Note that null value is not a UIResource, but tabAreaBackground should be used in this case. Or it should not? Will our components survive null background color? I guess we should check how UIResource usually check a null value. Not 100% sure that component background is allowed to be null, but if it is then it was set by user and we should use user's preference as we discuses offline. --Semyon --Semyon On 5/15/2015 3:10 PM, Semyon Sadetsky wrote: On 5/15/2015 2:06 PM, Sergey Bylokhov wrote: On 14.05.15 18:00, Semyon Sadetsky wrote: Sergey, Why the component background priority looks reasonable for you? Because otherwise there is no way to change the color of one particular component. A component has only one background color which is set by L&F to the default value. But it is not enough for the tabbed pane component which has more then one background surfaces and L&F can define more background colors to paint tabbed pane more precisely. In this case the common component background should have second priority. What is the difference between "TabbedPane.background" and "TabbedPane.tabAreaBackground" in the metal l&f? "TabbedPane.background" is the default background. --Semyon On 5/14/2015 5:39 PM, Sergey Bylokhov wrote: Hi, Semyon. Usage of background(if it was set) instead of property looks reasonable. Please clarify who sets default color of component explicitly to the value other than from UI property? On 14.05.15 17:12, Semyon Sadetsky wrote: Hello, Please review fix for JDK9: bug: https://bugs.openjdk.java.net/browse/JDK-8078269 webrev: http://cr.openjdk.java.net/~ssadetsky/8078269/webrev.00/ This is regression of the 8007563 which was incorrect fix of the 4690946 regression. Actually the 4690946 was a test bug because tab area's background can be controlled separately using "TabbedPane.tabAreaBackground" property. Presence of "TabbedPane.tabAreaBackground" depends on L&F. Test of the 4690946 is fixed to take this into account. Also inappropriate design chosen for the 8007563 reg test code affected test results stability. It also has been fixed. --Semyon -- Best regards, Sergey. -- Best regards, Sergey. -- Best regards, Sergey.
[9] Review Request for 8098835: [PIT] Endless loop in JEditorPane
Hello, Please review fix for JDK9: bug: https://bugs.openjdk.java.net/browse/JDK-8098835 webrev: http://cr.openjdk.java.net/~ssadetsky/8098835/webrev.00/ It is regression of https://bugs.openjdk.java.net/browse/JDK-6866751. Some important palaces in the BasicTextUI where width should be increased on the caret width have been missed. --Semyon
[9] Review Request for 8076164: [JTextField] When input too long Thai character, cursor's behavior is odd
Hello, Please review fix for JDK9: bug: https://bugs.openjdk.java.net/browse/JDK-8076164 webrev: http://cr.openjdk.java.net/~ssadetsky/8076164/webrev.00/ It is a regression from 6973777. In 6973777 GlyphView minimum span with calculation was changed to use text wrapping into several lines. That was necessary to fix several JTextPane scenarios. But this change did not preserve the generic rule: if resizing is disabled in getResizeWeight() then the preferred size should be used as the minimum. Since originally the GlyphView was not resizable the fix broke JTextFiled's I18N view because text wrapping is not applicable to it by design. The solution restores the logic for JTextFiled's I18N content view. --Semyon
[9] Review Request for 8129830: JTree drag/drop on lower half of last child of container incorrect
Hello, Please review fix for JDK9: bug: https://bugs.openjdk.java.net/browse/JDK-8129830 webrev: http://cr.openjdk.java.net/~ssadetsky/8129830/webrev.00/ It is not possible to insert a tree node as a last child of any parent using Drag&Drop because the node is inserted as a sibling after the parent node. To distinct those cases a solution is proposed: if drop point location is closer to the previous row then to insert into the last child position and insert into the sibling position otherwise. --Semyon
[9] Review Request for 8129940: JRadioButton does not honor non-standard FocusTraversalKeys
Hello, Please review fix for JDK9: bug: https://bugs.openjdk.java.net/browse/JDK-8129940 webrev: http://cr.openjdk.java.net/~ssadetsky/8129940/webrev.00/ It is a regression from 8033699. In this fix focus traversal keys were hard-coded in JRadioButton to be TAB and SHIFT+TAB only. But those keys can be amended by user as well. Solution : use the generic logic to identify focus traversal keys. --Semyon
[9] Review Request for 8081484: [TEST_BUG]Test javax/swing/plaf/basic/6866751/bug6866751.java fails
Hello, Please review fix for JDK9: bug: https://bugs.openjdk.java.net/browse/JDK-8081484 webrev: http://cr.openjdk.java.net/~ssadetsky/8081484/webrev.00/ Frame need to be re-layouted after the caret with changed. --Semyon
Re: [9] Review Request for 8129940: JRadioButton does not honor non-standard FocusTraversalKeys
Hi Alexander, 1. It seems bug8075609.java is wrong . Tab key should not move focus to the next option in the group. 2. I have changed the if-statement to the shortcut you've proposed: --Semyon On 7/2/2015 3:13 PM, Alexander Scherbatiy wrote: - Could you look at the test javax/swing/JRadioButton/8075609/bug8075609.java It fails on my Windows system even without your fix (RuntimeException: Focus is not on textField as Expected). If it does not relate to the fix area we can create a separate issue to it. - The 'if' statement on the line 609 can be simplified to return keys != null && keys.contains(stroke) Thanks, Alexandr. On 6/26/2015 4:22 PM, Semyon Sadetsky wrote: Hello, Please review fix for JDK9: bug: https://bugs.openjdk.java.net/browse/JDK-8129940 webrev: http://cr.openjdk.java.net/~ssadetsky/8129940/webrev.00/ It is a regression from 8033699. In this fix focus traversal keys were hard-coded in JRadioButton to be TAB and SHIFT+TAB only. But those keys can be amended by user as well. Solution : use the generic logic to identify focus traversal keys. --Semyon
Re: [9] Review Request for 8129940: JRadioButton does not honor non-standard FocusTraversalKeys
Sorry, I forget to add the link: http://cr.openjdk.java.net/~ssadetsky/8129940/webrev.01/ On 7/2/2015 3:53 PM, Semyon Sadetsky wrote: Hi Alexander, 1. It seems bug8075609.java is wrong . Tab key should not move focus to the next option in the group. 2. I have changed the if-statement to the shortcut you've proposed: --Semyon On 7/2/2015 3:13 PM, Alexander Scherbatiy wrote: - Could you look at the test javax/swing/JRadioButton/8075609/bug8075609.java It fails on my Windows system even without your fix (RuntimeException: Focus is not on textField as Expected). If it does not relate to the fix area we can create a separate issue to it. - The 'if' statement on the line 609 can be simplified to return keys != null && keys.contains(stroke) Thanks, Alexandr. On 6/26/2015 4:22 PM, Semyon Sadetsky wrote: Hello, Please review fix for JDK9: bug: https://bugs.openjdk.java.net/browse/JDK-8129940 webrev: http://cr.openjdk.java.net/~ssadetsky/8129940/webrev.00/ It is a regression from 8033699. In this fix focus traversal keys were hard-coded in JRadioButton to be TAB and SHIFT+TAB only. But those keys can be amended by user as well. Solution : use the generic logic to identify focus traversal keys. --Semyon
Re: [9] Review Request for 8076164: [JTextField] When input too long Thai character, cursor's behavior is odd
Hi Alexander, Since the component is initialized only and is not added to any window the EDT is not really necessary. Anyway I added it and also took into account your other comments here: http://cr.openjdk.java.net/~ssadetsky/8076164/webrev.01/ --Semyon On 6/30/2015 4:24 PM, Alexander Scherbatiy wrote: The fix looks good to me. Just update the copyright in the test, use Swing component on EDT and format the code. Thanks, Alexandr. On 6/25/2015 5:27 PM, Semyon Sadetsky wrote: Hello, Please review fix for JDK9: bug: https://bugs.openjdk.java.net/browse/JDK-8076164 webrev: http://cr.openjdk.java.net/~ssadetsky/8076164/webrev.00/ It is a regression from 6973777. In 6973777 GlyphView minimum span with calculation was changed to use text wrapping into several lines. That was necessary to fix several JTextPane scenarios. But this change did not preserve the generic rule: if resizing is disabled in getResizeWeight() then the preferred size should be used as the minimum. Since originally the GlyphView was not resizable the fix broke JTextFiled's I18N view because text wrapping is not applicable to it by design. The solution restores the logic for JTextFiled's I18N content view. --Semyon
[9] Review Request for 8130735: javax.swing.TimerQueue: timer fires late when another timer starts
Hello, Please review fix for JDK9: bug: https://bugs.openjdk.java.net/browse/JDK-8130735 webrev: http://cr.openjdk.java.net/~ssadetsky/8130735/webrev.00/ The root cause is the setting larger expiration time for the timer which is already inserted into the delay queue. So all timers behind the timer cannot be executed earlier than its expiration time. This happens very rare only for repeated timers and only if user uses the Swing timer API inaccurately (call start() without stop()). The fix eliminates this possibility by introducing a check if the timer was already restarted concurrently. It is difficult to write test because I could not reliably reproduce the issue for a reasonable time. --Semyon
Re: [9] Review Request for 8098835: [PIT] Endless loop in JEditorPane
pushed. --Semyon On 7/10/2015 8:57 PM, Phil Race wrote: Looks like this has been approved. Can it be pushed now ? It needs some kind of noreg- keyword on the bug. -phil. On 07/08/2015 12:29 PM, Sergey Bylokhov wrote: Looks fine. On 17.06.15 15:21, Alexander Scherbatiy wrote: The fix looks good to me. Thanks, Alexandr. On 6/16/2015 8:27 PM, Semyon Sadetsky wrote: Hello, Please review fix for JDK9: bug: https://bugs.openjdk.java.net/browse/JDK-8098835 webrev: http://cr.openjdk.java.net/~ssadetsky/8098835/webrev.00/ It is regression of https://bugs.openjdk.java.net/browse/JDK-6866751. Some important palaces in the BasicTextUI where width should be increased on the caret width have been missed. --Semyon
Re: [9] Review Request for 8130735: javax.swing.TimerQueue: timer fires late when another timer starts
Hi Alexander,
I added the double check
:http://cr.openjdk.java.net/~ssadetsky/8130735/webrev.01/
--Semyon
On 7/13/2015 1:24 PM, Alexander Zvegintsev wrote:
Hello Semyon,
the fix looks good to me.
P.S. Just a side note, as I can see we could possibly start two
threads instead of one in startIfNeeded():
96 void startIfNeeded() {
97 if (! running) {
98 runningLock.lock();
99 try {
100 final ThreadGroup threadGroup =
AppContext.getAppContext().getThreadGroup();
101 AccessController.doPrivileged((PrivilegedAction) () -> {
102 String name = "TimerQueue";
103 Thread timerThread = new
ManagedLocalsThread(threadGroup,
104 this, name);
!running check is missing after try. It is not the case with current
code base, but it may be changed in future.
Thanks,
Alexander.
On 07/09/2015 08:08 PM, Semyon Sadetsky wrote:
Hello,
Please review fix for JDK9:
bug: https://bugs.openjdk.java.net/browse/JDK-8130735
webrev: http://cr.openjdk.java.net/~ssadetsky/8130735/webrev.00/
The root cause is the setting larger expiration time for the timer
which is already inserted into the delay queue. So all timers behind
the timer cannot be executed earlier than its expiration time. This
happens very rare only for repeated timers and only if user uses the
Swing timer API inaccurately (call start() without stop()).
The fix eliminates this possibility by introducing a check if the
timer was already restarted concurrently.
It is difficult to write test because I could not reliably reproduce
the issue for a reasonable time.
--Semyon
