<Swing Dev> [9] Review request for 8154431: Allow source and target based validation for the focus transfer between two JComponents.
Alexandr Scherbatiy
alexandr.scherbatiy at oracle.com
Wed Jun 1 19:04:45 UTC 2016
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
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20160601/6f659f74/attachment.html>
More information about the swing-dev
mailing list