[API Review] RT-9383: javafx.scene.input.*Event classes construction

[Pavel Safrata]

Reviving this dead discussion.
Thanks,
Pavel

On 19.11.2012 10:19, Martin Sladecek wrote:
> Richard, what do you think?
>
> The approved changes were pushed to the repository already, so it 
> would be good to fix the issues Pavel found ASAP.
>
> And there's also one more thing that should be changed. In the 
> original proposal, there are integer codes in the KeyEvent 
> constructors, but the feature request to add them is still open ( 
> http://javafx-jira.kenai.com/browse/RT-20448). Since KeyEvents 
> currently operates with KeyCode enum only, these constructors should 
> be removed and only the constructor with KeyCode should be left in the 
> API.
>
> Thanks,
> -Martin
>
>
> On 11/08/2012 12:42 PM, Pavel Safrata wrote:
>> Hello,
>> shame on me, I'm really sorry for getting here so late. I've just 
>> read through the proposal and I'd like to propose several changes 
>> before it starts to be used by apps.
>>
>>> DragEvent:
>>> public DragEvent copyFor(Object source, EventTarget target, Object 
>>> gestureSource, Object gestureTarget, Dragboard dragboard)
>>> public DragEvent copyFor(Object source, EventTarget target, Object 
>>> gestureSource, Object gestureTarget, TransferMode transferMode, 
>>> EventType<DragEvent> eventType)
>>>
>>
>> I think we should not complicate the API with those two methods. They 
>> have no obvious usage for users and I think all existing internal 
>> usages can be removed by a simple refactoring.
>>
>>> MouseEvent:
>>> public MouseEvent(EventType<? extends MouseEvent> eventType, double 
>>> x, double y, double screenX, double screenY, MouseButton button,int 
>>> clickCount, boolean shiftDown, boolean controlDown, boolean altDown, 
>>> boolean metaDown,  boolean primaryButtonDown, boolean 
>>> middleButtonDown, boolean secondaryButtonDown, boolean synthesized, 
>>> boolean popupTrigger)
>>
>> I would drop this one, there is a similar one with one more argument: 
>> stillSincePress. This argument is not that special, we can use the 
>> other method instead. There are places in robot where we're not sure 
>> what to put there, but it doesn't really justify for having this 
>> duplicated method (it should rather be fixed there).
>>
>>> TouchEvent:
>>> public boolean isDirect() // was impl_ because (according to a 
>>> comment in code) there are no indirect events yet, but GestureEvent 
>>> already has this public.
>>
>> I think we should not publish isDirect() and that we should remove 
>> the "direct" argument from constructors. Currently touch events are 
>> always direct. We are able to produce indirect touch events from 
>> TrackPad, but our API specifically states that we don't and won't 
>> deliver them. They are just prepared for the possibility that we add 
>> custom gesture recognizers in the future that may work with those 
>> indirect events, but it is not really certain we'll ever do that, and 
>> if we will, it is not certain if it will be allowed to feed custom 
>> events to them. So publishing this useless flag would be pretty 
>> confusing right now.
>>
>> Finally, to make TouchEvent constructor usable, we need also 
>> constructor for TouchPoint.
>>
>> Let me once more apologize for the late response.
>> Thaks,
>> Pavel
>>
>>>
>>>
>>> -Martin
>>>
>>> On 06/15/2012 11:44 PM, Richard Bair wrote:
>>>>>>> As for the approach, I think you do the constructors with all 
>>>>>>> params (since events are immutable you have no choice really -- 
>>>>>>> static factory or constructor and I prefer in this case a 
>>>>>>> constructor) + builder (auto generated).
>>>>>> And what do you think about impl_copy methods? Personally I think 
>>>>>> we should remove them completely and turn them to some internal 
>>>>>> utility methods.
>>>>> My initial thought was a copy constructor.
>>>>>
>>>>>> Also, some events are not 100% immutable, as they are modified 
>>>>>> during the Scene processing through some impl_methods, like 
>>>>>> MouseEvent.impl_setClickParam. We'd either need to make some/all 
>>>>>> of the Event fields protected and do this modifications through 
>>>>>> subclasses or create some "accessor" in javafx.scene.input 
>>>>>> package for calling these methods from javafx.scene package.
>>>>> Good question, I guess we can revisit these in context of the 
>>>>> other impl_ method removal things we're working on.
>>>>>
>>>>> Richard
>>
>