On 5/11/2016 12:17 AM, Phil Race wrote:
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.
I misspelled over-ridden as over-riden. This is a new situation is not
it? :)
- 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.
Sorry, if I missed your point. I didn’t mean to.
But the old shouldYieldFocus() method is not final and that was not an
issue to intercept it before (the interceptor pattern is the way how the
validation usually applied). OK, it may be non-final by mistake. I don't
argue.
But it may be necessary to change the default validation logic for
source&target since the method accepts two args now and so there are no
way to do this without over-riding it. Does it make sense?
- @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.
I agree that the method was supposed to be called from Swing only, but
it had been being publicly non-final (as all other methods in this
class), so it was not prohibited from over-riding or using it from the
user code. Now the old shouldYieldFocus() is not needed since it is not
called by Swing anymore (its overloaded version is called instead) and
now it only proxies the verify() method for the backward compatibility,
because it might potentially be over-ridden or used in the user code
before the fix.
Assuming that your expectations are correct and the method is not
actually tied to any user code, then we are free to remove old
shouldYieldFocus().
It needs additional re-specifying to note that the focus
is not transferred out if the verifyTarget() method returns false.
That is true for the basic implementation of the shouldYieldFocus() only.
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.
Right. That is why shouldYieldFocus() should not be final since one may
need to validate both the source state in connection with the current
target state in some cases. And it is the only place where we have both
components.
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);
}
I see that we are talking about the same target design that may work
equally well for source and for source+target validation and be
transparent and clear for the user. And one constraint that we need to
add to this set of equations is the backward compatibility with the
current user code.
What I do not like in your suggestion that there are 2 verify() methods
for one object: source. It is not clear which one should be used to for
validation, especially that the verify(source) is an abstract and so
mandatory to implement. That could imply two validation phases but it is
not intuitive how they are separated. Adding the symmetric validation
method for the target component is more clear solution for me. For the
coupled source-target validation the shouldYieldFocus(sorce, target) may
be over-ridden. I also thought to add a package private proxy method to
call the shouldYieldFocus from Swing, but I didn't really see any sense
in it.
--Semyon
-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
