<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
Wed Jun 1 07:18:55 UTC 2016
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/71a5728f/attachment.html>
More information about the swing-dev
mailing list