Re: [9] Review request for 8154431: Allow source and target based validation for the focus transfer between two JComponents.

[Semyon Sadetsky]

On 4/20/2016 12:11 AM, Phil Race wrote:

PS I see the class doc talks about shouldYieldFocus() being called,
so I don't understand the inter-relationship of that and verify(), but
it makes me no more sure that deprecating that method is right.
I deprecated shouldYieldFocus(JCopmponent) and added 
shouldYieldFocus(JCopmponent, JComponent). I cannot simply remove the 
first because it's public and can be called from user code. But it's not 
used in JDK code outside this class anymore only internally for 
compatibility.
I can remove the @Depricated annotation but still cannot get why do we 
need to keep two overloaded shouldYieldFocus methods if only one is 
supposed to be used? That may confuse user which one of them to use, so 
@Depricated is a hint.


I think perhaps verify() is all that the app is supposed to have 
over-ridden

and the comments are written on the assumption it would not over-ride
anything else.

Right. But in fact shouldYieldFocus is public and not final.
So perhaps the design mistake could be that shouldYieldFocus() was not 
final.

Maybe your new one should be - if it is public at all ...
Probably you are correct. Before this method had the same signature as 
verify() and there was no reason for user to change its default 
implementation because it would not provide anything extra . I think it 
should be package private then.
But now it has 2 arguments and can be taken in consideration as some 
intermediate step in which the results obtained from the verify() and 
the new verifyTarget() are combined, and user may want to change the 
behavior how they are combined. Does it make sense?


BTW you removed text that says

Before focus is transfered to another Swing component
The key word here is SWING - I looked at the JComponent code
and it returns true if either source or target is not a JComponent,
without ever getting near the call to shouldYieldFocus().
I know. That is why I changed the description of the class and its first 
statement now:


" This class provides the validation mechanism for Swing components."

I think we can emphasize this additionally. What do you think about the 
next:


  39  * Before focus is transferred from the source Swing component to 
the target

  40  * Swing component

?

--Semyon



-phil.


On 04/19/2016 12:41 PM, Phil Race wrote:

On 04/19/2016 11:05 AM, Semyon Sadetsky wrote:

On 4/19/2016 7:47 PM, Phil Race wrote:

Hi,

You are deprecating shouldYieldFocus(JComponent) and yet this class 
directly uses it.

Is this deprecation really the right thing to do ?
Why is this not correct? There are plenty examples in JDK: 
Component#setVisible() & Component#show(), Component#transferFocus() 
& Component#nextFocus(), etc...

This is necessary for backward compatibility.


My question is why deprecate it ?

So far as I can see unless some one wants to over-ride verifyTarget() 
they are

fine to continue over-riding this method and ignore the new one.

Leaving aside the merits of those previous changes, and at least one 
of those I think was dubious,
all you have done is at an @Deprecated annotation. There is no 
@deprecated javadoc tag, and you
have left doc which says when this method is called. In fact that doc 
is now very misleading.

It is not called. You call the new one.


BTW I see you mis-spell over-ridden as overriden in verifyTarget(..)



The new over-loaded shouldYieldFocus() is perhaps not much more 
than a utility.

And the doc says "calls verify(input)" which seems odd since you do not
directly call it. And you are just describing what the default 
implementation

does aren't you ?
This is also necessary for compatibility. There may be 
implementations of the InputVerify where the shouldYieldFocus() is 
overloaded since it is public (that was initial design mistake, I 
suppose). At the same time the shouldYieldFocus() is the entry point 
that plugs InputVerify into the JComponent.
But you are correct "calls verify(input)" is not precisely describes 
what happens in this method. Maybe just make it more indirect, like 
"validate the source and the target components..."?


Now I am rather confused. You make it sound like verify() not 
shouldYieldFocus() is all the public

API should have contained, but you are adding a public over-load of it.
So why does this new method need to be public ? All you need is 
verifyTarget(), dont you ?


-phil.




--Semyon


-phil.

On 04/19/2016 04:40 AM, Semyon Sadetsky wrote:

Hello,

Please review fix for JDK9:
bug: https://bugs.openjdk.java.net/browse/JDK-8154431
webrev: http://cr.openjdk.java.net/~ssadetsky/8154431/webrev.00/

The thing is the Swing validation doesn't allow to validate state 
of the target component of input focus transfer operation.
To support that the fix adds two new methods 
verifyTarget(JComponent target) and shouldYieldFocus(JComponent 
source, JComponent target) to the javax.swing.InputVerifier class, 
and its old shouldYieldFocus(JComponent input) is marked as 
deprecated.

The 

Re: RfR JDK-8076554, [macosx] Custom Swing text components need to allow standard accessibility

[Pete Brunet]

p.s. JPRT ran OK. 

Still need one more +1.

On 4/19/16 2:16 PM, Phil Race wrote:
> +1
>
> -phil.
>
> On 04/19/2016 12:05 PM, Pete Brunet wrote:
>> New patch: http://cr.openjdk.java.net/~ptbrunet/JDK-8076554/webrev.03/
>>
>> Changes:
>> - removed _AccessibleState
>> - removed wildcard imports.
>>
>> Is this ready to push?
>>
>> Pete
>>
>> On 4/19/16 12:26 PM, Phil Race wrote:
>>> oh one nit picky thing - can we get rid of the wild card imports ?
>>>
>>> -phil.
>>>
>>> On 04/19/2016 10:21 AM, Phil Race wrote:
 Which seems to imply the _AccessibleState subclass has been
 obsolete since 1.5
 Although when these tools were unbundled it perhaps could not
 assume 1.5.

 The rest of the changes look fine and it is a lot simpler than the
 v0 ..

 -phil.

 On 04/19/2016 08:54 AM, Pete Brunet wrote:
> Thanks for noticing that Alexandr.  I see this state was added in
> 1.5 and apparently the code from which I borrowed this from was
> never updated.  I will remove that and open a bug to remove that
> from the original.  -Pete
>
> On 4/19/16 7:39 AM, Alexander Scherbatiy wrote:
>> On 19/04/16 05:50, Pete Brunet wrote:
>>> Please review the following patch.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8076554
>>> Patch: http://cr.openjdk.java.net/~ptbrunet/JDK-8076554/webrev.02/
>>>
>>> The problem is that the code is currently hardcoded to only support text
>>> accessibility for JTextComponent using the DocumentListener,
>>> CaretListener interfaces.  This eliminates the ability to provide
>>> accessibility for a custom text components.  The resolution is to remove
>>> the JTextComponent gate and use the PropertyChangeListener listener
>>> interface with its ACCESSIBLE_CARET_PROPERTY and
>>> ACCESSIBLE_TEXT_PROPERTY properties.
>>   The AccessibleState class already has the MANAGES_DESCENDANTS
>> constant. What is purpose to add a constant with the same name to
>> the _AccessibleState?
>>
>>   Thanks,
>>   Alexandr.
>>> I tested this with the two test cases which can be found at
>>> http://cr.openjdk.java.net/~ptbrunet/JDK-8076554/test/
>>> running them side by side with VoiceOver.
>>>
>>> Note that I will fix the similar JProgressBar and JSlider restrictions
>>> in JDK-8154507 and JDK-8154508 respectively.
>>>
>>> Pete
>>>
>>>
>>
>

>>>
>>
>

Re: [9] Review request for 8154431: Allow source and target based validation for the focus transfer between two JComponents.

[Phil Race]

PS I see the class doc talks about shouldYieldFocus() being called,
so I don't understand the inter-relationship of that and verify(), but
it makes me no more sure that deprecating that method is right.

I think perhaps verify() is all that the app is supposed to have over-ridden
and the comments are written on the assumption it would not over-ride
anything else.
So perhaps the design mistake could be that shouldYieldFocus() was not 
final.

Maybe your new one should be - if it is public at all ...

BTW you removed text that says

 Before focus is transfered to another Swing component

The key word here is SWING - I looked at the JComponent code
and it returns true if either source or target is not a JComponent,
without ever getting near the call to shouldYieldFocus().

-phil.


On 04/19/2016 12:41 PM, Phil Race wrote:

On 04/19/2016 11:05 AM, Semyon Sadetsky wrote:

On 4/19/2016 7:47 PM, Phil Race wrote:

Hi,

You are deprecating shouldYieldFocus(JComponent) and yet this class 
directly uses it.

Is this deprecation really the right thing to do ?
Why is this not correct? There are plenty examples in JDK: 
Component#setVisible() & Component#show(), Component#transferFocus() 
& Component#nextFocus(), etc...

This is necessary for backward compatibility.


My question is why deprecate it ?

So far as I can see unless some one wants to over-ride verifyTarget() 
they are

fine to continue over-riding this method and ignore the new one.

Leaving aside the merits of those previous changes, and at least one 
of those I think was dubious,
all you have done is at an @Deprecated annotation. There is no 
@deprecated javadoc tag, and you
have left doc which says when this method is called. In fact that doc 
is now very misleading.

It is not called. You call the new one.


BTW I see you mis-spell over-ridden as overriden in verifyTarget(..)



The new over-loaded shouldYieldFocus() is perhaps not much more than 
a utility.

And the doc says "calls verify(input)" which seems odd since you do not
directly call it. And you are just describing what the default 
implementation

does aren't you ?
This is also necessary for compatibility. There may be 
implementations of the InputVerify where the shouldYieldFocus() is 
overloaded since it is public (that was initial design mistake, I 
suppose). At the same time the shouldYieldFocus() is the entry point 
that plugs InputVerify into the JComponent.
But you are correct "calls verify(input)" is not precisely describes 
what happens in this method. Maybe just make it more indirect, like 
"validate the source and the target components..."?


Now I am rather confused. You make it sound like verify() not 
shouldYieldFocus() is all the public

API should have contained, but you are adding a public over-load of it.
So why does this new method need to be public ? All you need is 
verifyTarget(), dont you ?


-phil.




--Semyon


-phil.

On 04/19/2016 04:40 AM, Semyon Sadetsky wrote:

Hello,

Please review fix for JDK9:
bug: https://bugs.openjdk.java.net/browse/JDK-8154431
webrev: http://cr.openjdk.java.net/~ssadetsky/8154431/webrev.00/

The thing is the Swing validation doesn't allow to validate state 
of the target component of input focus transfer operation.
To support that the fix adds two new methods 
verifyTarget(JComponent target) and shouldYieldFocus(JComponent 
source, JComponent target) to the javax.swing.InputVerifier class, 
and its old shouldYieldFocus(JComponent input) is marked as 
deprecated.

The solution guaranties full compatibility with previous JDK versions.

--Semyon

Re: RfR JDK-8076554, [macosx] Custom Swing text components need to allow standard accessibility

[Phil Race]

+1

-phil.

On 04/19/2016 12:05 PM, Pete Brunet wrote:

New patch: http://cr.openjdk.java.net/~ptbrunet/JDK-8076554/webrev.03/

Changes:
- removed _AccessibleState
- removed wildcard imports.

Is this ready to push?

Pete

On 4/19/16 12:26 PM, Phil Race wrote:

oh one nit picky thing - can we get rid of the wild card imports ?

-phil.

On 04/19/2016 10:21 AM, Phil Race wrote:
Which seems to imply the _AccessibleState subclass has been obsolete 
since 1.5
Although when these tools were unbundled it perhaps could not assume 
1.5.


The rest of the changes look fine and it is a lot simpler than the v0 ..

-phil.

On 04/19/2016 08:54 AM, Pete Brunet wrote:
Thanks for noticing that Alexandr.  I see this state was added in 
1.5 and apparently the code from which I borrowed this from was 
never updated.  I will remove that and open a bug to remove that 
from the original.  -Pete


On 4/19/16 7:39 AM, Alexander Scherbatiy wrote:

On 19/04/16 05:50, Pete Brunet wrote:

Please review the following patch.

Bug:https://bugs.openjdk.java.net/browse/JDK-8076554
Patch:http://cr.openjdk.java.net/~ptbrunet/JDK-8076554/webrev.02/

The problem is that the code is currently hardcoded to only support text
accessibility for JTextComponent using the DocumentListener,
CaretListener interfaces.  This eliminates the ability to provide
accessibility for a custom text components.  The resolution is to remove
the JTextComponent gate and use the PropertyChangeListener listener
interface with its ACCESSIBLE_CARET_PROPERTY and
ACCESSIBLE_TEXT_PROPERTY properties.
  The AccessibleState class already has the MANAGES_DESCENDANTS 
constant. What is purpose to add a constant with the same name to 
the _AccessibleState?


  Thanks,
  Alexandr.

I tested this with the two test cases which can be found at
http://cr.openjdk.java.net/~ptbrunet/JDK-8076554/test/
running them side by side with VoiceOver.

Note that I will fix the similar JProgressBar and JSlider restrictions
in JDK-8154507 and JDK-8154508 respectively.

Pete

Re: RfR JDK-8076554, [macosx] Custom Swing text components need to allow standard accessibility

[Pete Brunet]

New patch: http://cr.openjdk.java.net/~ptbrunet/JDK-8076554/webrev.03/

Changes:
- removed _AccessibleState
- removed wildcard imports.

Is this ready to push?

Pete

On 4/19/16 12:26 PM, Phil Race wrote:
> oh one nit picky thing - can we get rid of the wild card imports ?
>
> -phil.
>
> On 04/19/2016 10:21 AM, Phil Race wrote:
>> Which seems to imply the _AccessibleState subclass has been obsolete
>> since 1.5
>> Although when these tools were unbundled it perhaps could not assume 1.5.
>>
>> The rest of the changes look fine and it is a lot simpler than the v0 ..
>>
>> -phil.
>>
>> On 04/19/2016 08:54 AM, Pete Brunet wrote:
>>> Thanks for noticing that Alexandr.  I see this state was added in
>>> 1.5 and apparently the code from which I borrowed this from was
>>> never updated.  I will remove that and open a bug to remove that
>>> from the original.  -Pete
>>>
>>> On 4/19/16 7:39 AM, Alexander Scherbatiy wrote:
 On 19/04/16 05:50, Pete Brunet wrote:
> Please review the following patch.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8076554
> Patch: http://cr.openjdk.java.net/~ptbrunet/JDK-8076554/webrev.02/
>
> The problem is that the code is currently hardcoded to only support text
> accessibility for JTextComponent using the DocumentListener,
> CaretListener interfaces.  This eliminates the ability to provide
> accessibility for a custom text components.  The resolution is to remove
> the JTextComponent gate and use the PropertyChangeListener listener
> interface with its ACCESSIBLE_CARET_PROPERTY and
> ACCESSIBLE_TEXT_PROPERTY properties.
   The AccessibleState class already has the MANAGES_DESCENDANTS
 constant. What is purpose to add a constant with the same name to
 the _AccessibleState?

   Thanks,
   Alexandr.
> I tested this with the two test cases which can be found at
> http://cr.openjdk.java.net/~ptbrunet/JDK-8076554/test/
> running them side by side with VoiceOver.
>
> Note that I will fix the similar JProgressBar and JSlider restrictions
> in JDK-8154507 and JDK-8154508 respectively.
>
> Pete
>
>

>>>
>>
>

Re: [9] Review request for 8154431: Allow source and target based validation for the focus transfer between two JComponents.

[Semyon Sadetsky]

On 4/19/2016 7:47 PM, Phil Race wrote:

Hi,

You are deprecating shouldYieldFocus(JComponent) and yet this class 
directly uses it.

Is this deprecation really the right thing to do ?
Why is this not correct? There are plenty examples in JDK: 
Component#setVisible() & Component#show(), Component#transferFocus() & 
Component#nextFocus(), etc...

This is necessary for backward compatibility.


The new over-loaded shouldYieldFocus() is perhaps not much more than a 
utility.

And the doc says "calls verify(input)" which seems odd since you do not
directly call it. And you are just describing what the default 
implementation

does aren't you ?
This is also necessary for compatibility. There may be implementations 
of the InputVerify where the shouldYieldFocus() is overloaded since it 
is public (that was initial design mistake, I suppose). At the same time 
the shouldYieldFocus() is the entry point that plugs InputVerify into 
the JComponent.
But you are correct "calls verify(input)" is not precisely describes 
what happens in this method. Maybe just make it more indirect, like 
"validate the source and the target components..."?


--Semyon


-phil.

On 04/19/2016 04:40 AM, Semyon Sadetsky wrote:

Hello,

Please review fix for JDK9:
bug: https://bugs.openjdk.java.net/browse/JDK-8154431
webrev: http://cr.openjdk.java.net/~ssadetsky/8154431/webrev.00/

The thing is the Swing validation doesn't allow to validate state of 
the target component of input focus transfer operation.
To support that the fix adds two new methods verifyTarget(JComponent 
target) and shouldYieldFocus(JComponent source, JComponent target) to 
the javax.swing.InputVerifier class, and its old 
shouldYieldFocus(JComponent input) is marked as deprecated.

The solution guaranties full compatibility with previous JDK versions.

--Semyon

Re: RfR JDK-8076554, [macosx] Custom Swing text components need to allow standard accessibility

[Phil Race]

oh one nit picky thing - can we get rid of the wild card imports ?

-phil.

On 04/19/2016 10:21 AM, Phil Race wrote:
Which seems to imply the _AccessibleState subclass has been obsolete 
since 1.5

Although when these tools were unbundled it perhaps could not assume 1.5.

The rest of the changes look fine and it is a lot simpler than the v0 ..

-phil.

On 04/19/2016 08:54 AM, Pete Brunet wrote:
Thanks for noticing that Alexandr.  I see this state was added in 1.5 
and apparently the code from which I borrowed this from was never 
updated.  I will remove that and open a bug to remove that from the 
original.  -Pete


On 4/19/16 7:39 AM, Alexander Scherbatiy wrote:

On 19/04/16 05:50, Pete Brunet wrote:

Please review the following patch.

Bug:https://bugs.openjdk.java.net/browse/JDK-8076554
Patch:http://cr.openjdk.java.net/~ptbrunet/JDK-8076554/webrev.02/

The problem is that the code is currently hardcoded to only support text
accessibility for JTextComponent using the DocumentListener,
CaretListener interfaces.  This eliminates the ability to provide
accessibility for a custom text components.  The resolution is to remove
the JTextComponent gate and use the PropertyChangeListener listener
interface with its ACCESSIBLE_CARET_PROPERTY and
ACCESSIBLE_TEXT_PROPERTY properties.
  The AccessibleState class already has the MANAGES_DESCENDANTS 
constant. What is purpose to add a constant with the same name to 
the _AccessibleState?


  Thanks,
  Alexandr.

I tested this with the two test cases which can be found at
http://cr.openjdk.java.net/~ptbrunet/JDK-8076554/test/
running them side by side with VoiceOver.

Note that I will fix the similar JProgressBar and JSlider restrictions
in JDK-8154507 and JDK-8154508 respectively.

Pete

Re: [9] Review request for 8154431: Allow source and target based validation for the focus transfer between two JComponents.

[Phil Race]

Hi,

You are deprecating shouldYieldFocus(JComponent) and yet this class 
directly uses it.

Is this deprecation really the right thing to do ?

The new over-loaded shouldYieldFocus() is perhaps not much more than a 
utility.

And the doc says "calls verify(input)" which seems odd since you do not
directly call it. And you are just describing what the default 
implementation

does aren't you ?

-phil.

On 04/19/2016 04:40 AM, Semyon Sadetsky wrote:

Hello,

Please review fix for JDK9:
bug: https://bugs.openjdk.java.net/browse/JDK-8154431
webrev: http://cr.openjdk.java.net/~ssadetsky/8154431/webrev.00/

The thing is the Swing validation doesn't allow to validate state of 
the target component of input focus transfer operation.
To support that the fix adds two new methods verifyTarget(JComponent 
target) and shouldYieldFocus(JComponent source, JComponent target) to 
the javax.swing.InputVerifier class, and its old 
shouldYieldFocus(JComponent input) is marked as deprecated.

The solution guaranties full compatibility with previous JDK versions.

--Semyon

Re: RfR JDK-8076554, [macosx] Custom Swing text components need to allow standard accessibility

[Phil Race]

Which seems to imply the _AccessibleState subclass has been obsolete 
since 1.5

Although when these tools were unbundled it perhaps could not assume 1.5.

The rest of the changes look fine and it is a lot simpler than the v0 ..

-phil.

On 04/19/2016 08:54 AM, Pete Brunet wrote:
Thanks for noticing that Alexandr.  I see this state was added in 1.5 
and apparently the code from which I borrowed this from was never 
updated.  I will remove that and open a bug to remove that from the 
original.  -Pete


On 4/19/16 7:39 AM, Alexander Scherbatiy wrote:

On 19/04/16 05:50, Pete Brunet wrote:

Please review the following patch.

Bug:https://bugs.openjdk.java.net/browse/JDK-8076554
Patch:http://cr.openjdk.java.net/~ptbrunet/JDK-8076554/webrev.02/

The problem is that the code is currently hardcoded to only support text
accessibility for JTextComponent using the DocumentListener,
CaretListener interfaces.  This eliminates the ability to provide
accessibility for a custom text components.  The resolution is to remove
the JTextComponent gate and use the PropertyChangeListener listener
interface with its ACCESSIBLE_CARET_PROPERTY and
ACCESSIBLE_TEXT_PROPERTY properties.
  The AccessibleState class already has the MANAGES_DESCENDANTS 
constant. What is purpose to add a constant with the same name to the 
_AccessibleState?


  Thanks,
  Alexandr.

I tested this with the two test cases which can be found at
http://cr.openjdk.java.net/~ptbrunet/JDK-8076554/test/
running them side by side with VoiceOver.

Note that I will fix the similar JProgressBar and JSlider restrictions
in JDK-8154507 and JDK-8154508 respectively.

Pete

Re: Review Request of 8152492: [macosx swing] double key event actions when using Mac menubar

[Sergey Bylokhov]

On 19.04.16 11:46, Avik Niyogi wrote:

Hi All,
Please review my code changes with inputs received:


Can you please add some description of your changes, it seems you 
removed all code which was added by you previously.



http://cr.openjdk.java.net/~aniyogi/8152492/webrev.01/

With Regards,
Avik Niyogi

On 19-Apr-2016, at 11:47 am, Avik Niyogi > wrote:

Hi Sergey,
I have tried all these Input sources:

I am unable to reproduce the test failure as mentioned by you with
these inputs. Please provide some screenshots and/or details of the
procedure used for this test failure.
Thank you in advance.

With Regards,
Avik Niyogi


On 08-Apr-2016, at 8:15 pm, Sergey Bylokhov
> wrote:

Hi, Avik.
It seems there is one more bug in this "if".
Change the input source to "Hindi Dev.."/"Russian.." then this test
fails anyway(it will produce the double action).
Moreover if I change the source input to Hindi Transliteration, the
test sometimes crashed.
Please double check.

On 06.04.16 13:42, Avik Niyogi wrote:

Hi All,

Kindly review the bug fix for JDK 9.

*Bug:*
https://bugs.openjdk.java.net/browse/JDK-8152492

*Webrev:*
http://cr.openjdk.java.net/~aniyogi/8152492/webrev.00/

*Issue:*
For OSX, having there is provision of non-integrated menubar. for this
case, triggering the menu action by key press triggered double action in
some cases

*Cause:*
Due to the use of Shift modifier being a soft modifier in case of mac.
For example “Shift + m” is actually “M” and “Shift + ,” is “<“ .
So such cases can not be used as shortcuts. This needs to be
accounted for.
Also, cases where “Cmd + Shift + c” is used as a shortcut, it should not
morph that to a “Cmd + C” command and should detect c itself.
Also, “Shift + ↑” has no equivalent soft modification and can be taken
as a shortcut. This has to be accounted for.

*Fix:*
All required edge case scenarios for the soft modification of Shift and
cases of valid modifiers were taken into account.

With Regards,
Avik Niyogi



--
Best regards, Sergey.







--
Best regards, Sergey.

Re: [9] Review request for 8152677 [macosx] All files filter can't be selected in JFileChooser

[Alexander Scherbatiy]

  Hello,

  Could you review the updated fix:
http://cr.openjdk.java.net/~alexsch/8152677/webrev.01

  - file chooser file filter is used as an initial value for the old 
file filter in the AquaFileChooserUI.FilterComboBoxModel

  - AcceptAllFileFilter class and filed are made static


 The current regression appears after fixing issue JDK-8031696 [macosx] 
TwentyThousandTest test failed with OOM.
 This was because for the each file filter setting (this includes file 
chooser creating and UI updating) new Aqua L file loading thread was 
started.
 The current fix makes the acceptAllFileFilter field static which 
allows to eliminate the tread starting when the file chooser UI is 
updated. However, it is still not enough and the TwentyThousandTest 
still fails. It requires additional investigation what else leads to the 
OOME.  I have created a separate issue on it: JDK-8154548 [macosx] 
Reconsider TwentyThousandTest test failed fix:

   https://bugs.openjdk.java.net/browse/JDK-8154548

  Thanks,
  Alexandr.


On 15/04/16 20:26, Sergey Bylokhov wrote:

On 15.04.16 19:22, Alexander Scherbatiy wrote:

On 15/04/16 18:27, Sergey Bylokhov wrote:
  When a JFileChooser is created it always adds Accept All file filter
in the constructor because useAcceptAllFileFilter filed is true by 
default.

  Because of this the AquaFileChooserUI.FilterComboBoxModel initially
has the Accept All file filter as the first item.

  After that it is possible to unset the Accept All file filter by
calling JFileChooser.setAcceptAllFileFilterUsed(false).


But what will happen if the user will update the filter before the 
Aqua will be set as default l(for example if Metal is set as default 
via commandline)?



On 15.04.16 16:46, Alexander Scherbatiy wrote:


Hello,

Could you review the fix:
   bug: https://bugs.openjdk.java.net/browse/JDK-8152677
   webrev: http://cr.openjdk.java.net/~alexsch/8152677/webrev.00

   FilterComboBoxModel from AquaFileChooserUI overrides
getSelectedItem() method to always return the selected filter from the
file chooser.

   JFileChooser.setFileFilter(FileFilter) first assign the passed 
filter

to fileFilter field and then fire "fileFilterChanged" property change
event. This leads that FilterComboBoxModel compares new file filter 
with

the selected one (which has the same value because it is obtained from
JFileChooser.fileFilter field) and do not updated JComboBox internal
structure so JComboBox.selectedItemReminder field still points to the
old one.

   When a user selects “All Files” filter which is the first item 
from a

JFileChooser the JComboBox does not fire the action event because
selectedItemReminder points to the same first item.

   The proposed solution is to remember the previous selected item in
the AquaFileChooserUI.FilterComboBoxModel.

   Thanks,
   Alexandr.

[9] Review request for 8154431: Allow source and target based validation for the focus transfer between two JComponents.

[Semyon Sadetsky]

Hello,

Please review fix for JDK9:
bug: https://bugs.openjdk.java.net/browse/JDK-8154431
webrev: http://cr.openjdk.java.net/~ssadetsky/8154431/webrev.00/

The thing is the Swing validation doesn't allow to validate state of the 
target component of input focus transfer operation.
To support that the fix adds two new methods verifyTarget(JComponent 
target) and shouldYieldFocus(JComponent source, JComponent target) to 
the javax.swing.InputVerifier class, and its old 
shouldYieldFocus(JComponent input) is marked as deprecated.

The solution guaranties full compatibility with previous JDK versions.

--Semyon

Re: [9] Review request for 8132119 Provide public API for text related methods in SwingUtilities2

[Alexandr Scherbatiy]

On 4/11/2016 4:29 PM, Philip Race wrote:



On 4/6/16, 1:23 PM, Alexander Scherbatiy wrote:


Hello,

Could you review the updated fix:
  http://cr.openjdk.java.net/~alexsch/8132119/webrev.09

 - TextUIDrawing interface and its default implementaion 
BasicTextUIDrawing class are added

 - font metrics argument description is updated

On 31/03/16 23:23, Phil Race wrote:
Another webrev where you have to slip past 40_ files to get to the 
two that really matter :-)
I would have put SwingUtilities2.java and TextUIDrawing.java as the 
first files.

   Updated.



Some of what I have to say here is more along the lines of things to 
think
about rather than things that are wrong .. but there are also maybe 
some things

that need to be fixed.

Is javax.swing.plaf really the right package for the new class ?
I suppose it is for the use by the UI classes so maybe its right.

Should the methods be taking "double" instead of "int" for location ?
This means the measurement APIs too.
None of the JDK 1.2 text APIs use ints. That is all 1.0 legacy.
So if Swing internally wants to use ints that is OK but maybe the API
should be floating point (double).
  The provided methods use Graphics as argument which only has 
drawString(String, int, int) method.
  If it is possible it is better to add the methods with float 
arguments and Graphics2D later.



Would that  help hi-dpi at all ?
  The hi-dpi support mostly does not require changes in Swing. What 
it does just scales graphics using default transform from graphics 
configuration.


Yes, but in another bug you are dealing with a problem positioning
the caret because of (somewhat) similar issues where coordinates have 
been

rounded to an integral. A floating point value allows you to say that
this is 25.5 in user-space, even it if is 51.0 in device space.


   It needs some more investigation.

   What I have now is Swing uses font metrics to calculate a string 
width (FontMetrics.charsWidth(...)) which sums up float char values.
   The difference between font metrics used by Swing and font metrics 
from graphics passed to paint method is that the fist has null frc.tx 
matrix and the second one has a matrix with scales 2 on HiDPI display.
   The returned char width by the font metrics with null transform has 
value 7 for char 'a' (linear advance is 6.67 and xAdvance is 7).
   The char width for the font metrics with scaled transform is 6.5 for 
the same font and char.  FileFontStrike requests glyph metrics and gets 
linear advance  13.35 (dev transform is taken into account) xAdvance  13 
-  and apply the reverse transform. The result is 13 / 2  = 6.5.


  And this bothers me because a result for applying the tx transform 
and inverting it is different than just use the identity transform. 
There are definitely problems with advance rounding but it seems they 
are placed out of the Swing area.







I suppose it would add over-head since all the existing code uses int
and we are no worse off and can add double methods later if we want to.


Regarding FontMetrics we need to add a caution that is must be a 
FontMetrics

*obtained from the correct font and graphics*.

   Updated.



 i.e what about attributes on the font such as "tracking" ?
or on the graphics such as FRACTIONALMETRICS
It looks like Swing might already fail if that were used.

Look at this code :-

   public static int stringWidth(JComponent c, FontMetrics fm, 
String string){

if (string == null || string.equals("")) {
return 0;
}
boolean needsTextLayout = ((c != null) &&
(c.getClientProperty(TextAttribute.NUMERIC_SHAPING) != null));
if (needsTextLayout) {
synchronized(charsBufferLock) {
int length = syncCharsBuffer(string);
needsTextLayout = isComplexLayout(charsBuffer, 0, 
length);

}
}
if (needsTextLayout) {
TextLayout layout = createTextLayout(c, string,
fm.getFont(), 
fm.getFontRenderContext());

return (int) layout.getAdvance();
} else {
return fm.stringWidth(string);
}
}

The only thing Swing is looking at is one TextAttribute and whether 
we have complex text.
That is not enough. This is an existing implementation issue but one 
we should fix here.

You need to examine all the methods for similar issues.

  I created an enhancement for this:
JDK-8153662 
SwingUtilities2.drawString()/getStringWidth()/clipString() should use 
more text attributes

  https://bugs.openjdk.java.net/browse/JDK-8153662


you mean bug ? I upgraded it to P3 because it matters a lot more now
with this public API


  I do not think that it is a bug because the main request was to have 
methods which draw strings in the same way as it is done by Swing L
  This will allow to have custom UI component which mimic to the 
standard L


  It is also can be considered from the following point of view: is the 
proposed 

Re: Review Request of 8152492: [macosx swing] double key event actions when using Mac menubar

[Avik Niyogi]

Hi Sergey,
I have tried all these Input sources:

I am unable to reproduce the test failure as mentioned by you with these 
inputs. Please provide some screenshots and/or details of the procedure used 
for this test failure.
Thank you in advance.

With Regards,
Avik Niyogi

> On 08-Apr-2016, at 8:15 pm, Sergey Bylokhov  
> wrote:
> 
> Hi, Avik.
> It seems there is one more bug in this "if".
> Change the input source to "Hindi Dev.."/"Russian.." then this test fails 
> anyway(it will produce the double action).
> Moreover if I change the source input to Hindi Transliteration, the test 
> sometimes crashed.
> Please double check.
> 
> On 06.04.16 13:42, Avik Niyogi wrote:
>> Hi All,
>> 
>> Kindly review the bug fix for JDK 9.
>> 
>> *Bug:*
>> https://bugs.openjdk.java.net/browse/JDK-8152492
>> 
>> *Webrev:*
>> http://cr.openjdk.java.net/~aniyogi/8152492/webrev.00/
>> 
>> *Issue:*
>> For OSX, having there is provision of non-integrated menubar. for this
>> case, triggering the menu action by key press triggered double action in
>> some cases
>> 
>> *Cause:*
>> Due to the use of Shift modifier being a soft modifier in case of mac.
>> For example “Shift + m” is actually “M” and “Shift + ,” is “<“ .
>> So such cases can not be used as shortcuts. This needs to be accounted for.
>> Also, cases where “Cmd + Shift + c” is used as a shortcut, it should not
>> morph that to a “Cmd + C” command and should detect c itself.
>> Also, “Shift + ↑” has no equivalent soft modification and can be taken
>> as a shortcut. This has to be accounted for.
>> 
>> *Fix:*
>> All required edge case scenarios for the soft modification of Shift and
>> cases of valid modifiers were taken into account.
>> 
>> With Regards,
>> Avik Niyogi
> 
> 
> -- 
> Best regards, Sergey.