On 04/22/2016 01:11 AM, Semyon Sadetsky wrote:
Forget to attach the link:
http://cr.openjdk.java.net/~ssadetsky/8154431/webrev.01/
--Semyon
On 4/22/2016 11:08 AM, Semyon Sadetsky wrote:
Please review the updated webrev:
32 * need to ensure that the components are in valid state before allowing
the
"are in a .."
actually the class doc needs cleaning up and so does the example
but before we do that maybe there are some other things to discuss.
The fix is updated according to the reviewers comments:
Not all how I imagined it though
> BTW I see you mis-spell over-ridden as overriden in verifyTarget(..)
I see you did not fix this.
- the test was updated to check the target object and year typo was
updated
- new shouldYieldFocus() javadoc improved
I said this should be final but it still isn't.
Since the existing one is not final, applications may be relying on Swing
calling this and intercept its behaviour and making the new one final
will ensure that doesn't happen since it is not how the class is
supposed to be used.
- @depricated documentation is added to the old shouldYieldFocus()
125 * @depricated use {@link #shouldYieldFocus(JComponent, JComponent)}
126 * instead.
I said this should not be deprecated.
The point about the missing javadoc tag was just that you should
have neither or both. Also you mis-spelt this.
And advice to use the other one is confusing since only Swing
is expected to call this.
It needs additional re-specifying to note that the focus
is not transferred out if the verifyTarget() method returns false.
Also I wonder if making verifyTarget independent is ideal
for people who want to verify both as they may want
a chance to see the src and target before they decide.
With the proposed design they get two separate calls
hopefully close together but in an un-specified order
and have to piece together what is happening.
The following looks tempting
public boolean verify(JComponent src, JComponent dst) {
....
}
but then this independence was useful here as you were
able to maintain calling verify(JComponent)
If we try this
129 public boolean shouldYieldFocus(JComponent input) {
130 return verify(input);
131 }
public boolean verify(JComponent src, JComponent dst) {
return shouldYieldFocus(src);
}
public final boolean shouldYieldFocus(JComponent source, JComponent
target) {
return verify(src, dst);
}
and a client over-rides verify(src, dst), where does that leave
the existing verify(src) which they have to implement as it is abstract ?
So that is unsatisfactory.
Stronger guarantees that we call verify() then verifyTarget() might help
a little. Or verify(src, dst) could be actually speced like
"verifyTarget()" but
just take the additional parameter so developers can examine the src
before making a final decision. Maybe that is the way to go ?
The code would look like :-
public boolean verify(JComponent src, JComponent dst) {
return true;
}
public final boolean shouldYieldFocus(JComponent source, JComponent
target) {
return shouldYieldFocus(src) && verify(source, target);
}
-phil.
- Swing component emphasized in the class javadoc
--Semyon
On 4/20/2016 11:40 AM, Semyon Sadetsky wrote:
On 4/19/2016 10: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 ?
I can repeat my reply to your anther e-mail in this thread if you
did not noticed it.
> 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.
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.
Yes, users may continue to use their old code after the fix. No
changes required on user side. It seems that is what is the backward
compatibility means.
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.
Agreed. I will add @depricated to the method javadoc and ask user do
not use it for the newly written code.
BTW I see you mis-spell over-ridden as overriden in verifyTarget(..)
Just stats from JDK: 5 usages in comments of "over-riden" and more
then 300 "overriden"... Google Translate was spoiled :)
--Semyon
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
