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

[Semyon Sadetsky]

On 6/2/2016 12:45 PM, Sergey Bylokhov wrote:
> There is a tiny typo in the specification:
> 34  * {@code InputVerifier} and, using {@code> JComponent}'s
> Is ">" inside {@code } necessary here?

yes it is typo.

Don't you mind if I remove this upon the push to avoid a new review 
iteration?

--Semyon
>
> On 01.06.16 22:04, Alexandr Scherbatiy wrote:
>>
>> The fix looks good to me.
>>
>> Thanks,
>> Alexandr.
>>
>> On 6/1/2016 7:46 PM, Philip Race wrote:
>>> +1
>>>
>>> -phil.
>>>
>>> On 6/1/16, 12:18 AM, Semyon Sadetsky wrote:
>>>>
>>>> Please the corrected webrev:
>>>> http://cr.openjdk.java.net/~ssadetsky/8154431/webrev.03/
>>>>
>>>> Also the CCC request was updated.
>>>>
>>>> --Semyon
>>>>
>>>> On 5/31/2016 10:23 PM, Phil Race wrote:
>>>>> The {@code InputVerifier} also provides possibility to validate
>>>>> against "provides the possibility" "the target of the focus transfer
>>>>> which may not allow to navigate to it" "the target of the focus
>>>>> transfer which may reject the focus"
>>>>>
>>>>> 136 * Checks whether the target JComponent that is receiving the
>>>>> focus "that will be ..."
>>>>>
>>>>> Other than that no comments except that I still have mixed feelings
>>>>> about not making shouldYieldFocus final in particular ..
>>>>>
>>>>> -phil.
>>>>>
>>>>> On 05/27/2016 05:51 PM, Semyon Sadetsky wrote:
>>>>>>
>>>>>> Hi Phil,
>>>>>>
>>>>>> please review the updated webrev:
>>>>>> http://cr.openjdk.java.net/~ssadetsky/8154431/webrev.02/
>>>>>>
>>>>>> I changed the example in the class spec to demonstrate
>>>>>> source+target validation and tried to improve spelling.
>>>>>>
>>>>>> --Semyon
>>>>>>
>>>>>>
>>>>>> On 5/24/2016 1:26 AM, Phil Race wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>>
>>>>>>> If the design stays as proposed with separate verify() and
>>>>>>> verifyTarget() calls, it needs to be carefully explained how these
>>>>>>> must be co-ordinated by Swing + the app.
>>>>>>>
>>>>>>> The class doc should  be augmented with an example showing how 
>>>>>>> to do
>>>>>>> source+target verification.
>>>>>>>
>>>>>>> As I previously noted (so you don't miss it) : you have a spelling
>>>>>>> mistake here :-
>>>>>>> 125      * @depricated use {@link #shouldYieldFocus(JComponent,
>>>>>>> JComponent)}
>>>>>>>
>>>>>>> If this stays deprecated I think the javadoc needs a couple of
>>>>>>> sentences to
>>>>>>> explain it.
>>>>>>>
>>>>>>> I think I am still not convinced that the new overload of
>>>>>>> shouldYieldFocus
>>>>>>> should be non-final. I know you write that it might help make
>>>>>>> amends for
>>>>>>> not having a single two arg verify method but we still need to 
>>>>>>> be able
>>>>>>> to show that the separate verify() and verifyTarget() can be 
>>>>>>> used and
>>>>>>> not do anything to direct people to over-riding this method as 
>>>>>>> well.
>>>>>>>
>>>>>>> (BTW the CCC you drafted also mis-spells Yield as Yeild in at
>>>>>>> least two places)
>>>>>>>
>>>>>>> -phil.
>>>>>>>
>>>>>>>
>>>>>>> On 05/11/2016 04:20 AM, Semyon Sadetsky wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> 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
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>
>
>