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

Phil Race philip.race at oracle.com
Tue May 31 19:23:30 UTC 2016


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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20160531/f172b532/attachment.html>


More information about the swing-dev mailing list