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

Wed May 11 11:20:43 UTC 2016

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/20160511/de21505e/attachment.html>