<Swing Dev> [9] Review request for 8154431: Allow source and target based validation for the focus transfer between two JComponents.
Philip Race
philip.race at oracle.com
Wed Jun 1 16:46:21 UTC 2016
+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/323f3066/attachment.html>
More information about the swing-dev
mailing list