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

Thu Jun 2 15:48:35 UTC 2016

On 02.06.16 18:46, Semyon Sadetsky wrote:
> 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?

That's fine.

>
> --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
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>
>>
>>
>

-- 
Best regards, Sergey.