Avalonia icon indicating copy to clipboard operation
Avalonia copied to clipboard

Remove public constructors for KeyEventArgs, PointerEventArgs and friends

Open Takoooooo opened this issue 1 year ago • 6 comments

Fixed issues

Part of #6666

Takoooooo avatar Aug 30 '22 14:08 Takoooooo

Should we also remove public ctors from Raw...EventArgs from Avalonia.Input.Raw namespace?

Takoooooo avatar Aug 30 '22 14:08 Takoooooo

We support platform backends being written by a 3rd party, but without any binary compatibility guarantees. So everything required to write such backend should be available

kekekeks avatar Aug 30 '22 14:08 kekekeks

You can test this PR using the following package version. 11.0.999-cibuild0023486-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]

avaloniaui-team avatar Aug 30 '22 14:08 avaloniaui-team

I could still create my own event args derived from EventArgs, right?

timunie avatar Aug 30 '22 16:08 timunie

Ah yes now I see it's removed for specific events which is unlikely you create on your own.

timunie avatar Aug 30 '22 16:08 timunie

You can test this PR using the following package version. 11.0.999-cibuild0024462-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]

avaloniaui-team avatar Oct 09 '22 23:10 avaloniaui-team

You can test this PR using the following package version. 11.0.999-cibuild0025706-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]

avaloniaui-team avatar Nov 03 '22 11:11 avaloniaui-team

@Takoooooo @kekekeks @danwalmsley Why was this done? As far as I can tell there isn't much risk exposing these classes unless there are known changes coming up. It however is causing issues for control-library authors: https://github.com/amwx/FluentAvalonia/pull/241#issue-1447061483.

@maxkatz6 Also mentioned it here but that also doesn't make sense to me since constructors like this will likely never change in the future.

I can foresee some custom controls on my end that won't benefit from this PR. It's going to require creating new event args instead of re-using what is already in the framework.

I'm really struggling to understand why this PR was done.

robloo avatar Nov 13 '22 20:11 robloo

Aside from the reasoning of this PR, we should make API flexible enough, so users won't need to simulate behavior by passing a fake input argument. I am specifically saying about TextBox.Redo/Undo which make all sense to be public methods.

maxkatz6 avatar Nov 13 '22 21:11 maxkatz6

Just created a new feature request for enhancing TextBox (#9433). That's a better solution anyway (I've never liked doing it via fake key event).

However, in my other cases:

  • My MenuFlyout control reuses some logic from the Avalonia menus, specifically the MenuItem.PointerEnteredItemEvent (and exited) and I copied the implementation which creates a new PointerEventArgs from the existing one - which I can't do anymore, so now I'm recycling the pointer args from the InputElement.PointerEnteredEvent and hoping there's no side effects
  • I was also using this in unit tests for quick pointer over stuff so I don't have to implement do a full mock pointer implementation for something simple

Last time I checked WPF still has these as public classes - WinUI doesn't though. At the very least, can these classes gain a default protected constructor (or make them protected internal) and make the properties protected set so at least they can be sub-classed to get around this (sort of)?

amwx avatar Nov 13 '22 23:11 amwx

I also am not 100% sure why this change was made. @Takoooooo or @kekekeks could you explain in more detail why it's not a good idea to create instances of these classes manually.

The only reason I can think of is that on certain platforms pointer capture is carried out automatically by the OS on pointer press, and raising a fake event won't have this behavior, is this true? There may be other reasons.

grokys avatar Nov 14 '22 19:11 grokys

It's mostly to limit the public API surface so we could add features and improve the perf later. We've already had a problem with public constructors when we were adding pointer histroy API.

In general I don't think that triggering fake events is a good idea, we should provide a better API for cases where it's currently required.

kekekeks avatar Nov 15 '22 03:11 kekekeks

Pointer capture also plays some role but not with PointerEntered event, I think.

kekekeks avatar Nov 15 '22 03:11 kekekeks

In my application the majority of input controls are not created until they get focused (the content is drawn directly), for example TextBox. In order to allow the created control to handle the PointerPressed event (like text inputs), I capture that event and need to resend to have the same experience:

        public void SendPointerPressed()
        {
            if (this.CapturedPointerEventArgs != default)
            {
                if (PART_InputCtrl is IDxFocusHelper focusHelper)
                {
                    var newArgs = this.CapturedPointerEventArgs;

                    /*  11.0.0-preview4 does not allow to create event anymore
                    var pointer = new Pointer(Pointer.GetNextFreeId(), PointerType.Mouse, true);
                    var newArgs = new PointerPressedEventArgs(this as IInteractive,
                                                              pointer,
                                                              this,
                                                              this.CapturedPointerEventArgs.GetPosition(this),
                                                              0,
                                                              new PointerPointProperties(RawInputModifiers.None, PointerUpdateKind.LeftButtonPressed),
                                                              KeyModifiers.None
                                                            );
                    */
                    focusHelper.SendPointerPressed(newArgs);
                }
                this.CapturedPointerEventArgs = default;
            }
        }
       -------
       derived TextBox class

       public void SendPointerPressed(PointerPressedEventArgs e)
        {
            this.Focus();
            this.OnPointerPressed(e);
        }
       

So currently, this is still working, with sending the already handled, captured event again (feels strange though).

Q: Is it save to resend captured PointerEventArgs?

doublecount avatar Nov 20 '22 11:11 doublecount