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

Thu Jun 2 09:45:55 UTC 2016

There is a tiny typo in the specification:
34  * {@code InputVerifier} and, using {@code> JComponent}'s
Is ">" inside {@code } necessary here?

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.