<Swing Dev> [9] Review request for 8154431: Allow source and target based validation for the focus transfer between two JComponents.
Semyon Sadetsky
semyon.sadetsky at oracle.com
Sat May 28 00:51:11 UTC 2016
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/20160528/9bf68c6c/attachment.html>
More information about the swing-dev
mailing list