Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove public constructors for KeyEventArgs, PointerEventArgs and friends #8860

Conversation

Takoooooo
Copy link
Contributor

Fixed issues

Part of #6666

@Takoooooo
Copy link
Contributor Author

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

@Takoooooo Takoooooo marked this pull request as ready for review August 30, 2022 14:19
@kekekeks
Copy link
Member

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

@avaloniaui-team
Copy link
Contributor

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]

@timunie
Copy link
Contributor

timunie commented Aug 30, 2022

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

@timunie
Copy link
Contributor

timunie commented Aug 30, 2022

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

@maxkatz6 maxkatz6 enabled auto-merge September 30, 2022 06:31
@danwalmsley danwalmsley disabled auto-merge October 8, 2022 14:32
@danwalmsley danwalmsley enabled auto-merge October 8, 2022 14:32
@danwalmsley danwalmsley disabled auto-merge October 8, 2022 14:34
@avaloniaui-team
Copy link
Contributor

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]

@maxkatz6 maxkatz6 mentioned this pull request Nov 1, 2022
1 task
@danwalmsley danwalmsley enabled auto-merge November 3, 2022 11:24
@danwalmsley danwalmsley merged commit a26d515 into master Nov 3, 2022
@danwalmsley danwalmsley deleted the remove-public-constructors-from-eventargs-from-avalonia.base branch November 3, 2022 11:39
@avaloniaui-team
Copy link
Contributor

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]

@robloo
Copy link
Contributor

robloo commented Nov 13, 2022

@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: amwx/FluentAvalonia#241 (comment).

@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.

@maxkatz6
Copy link
Member

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.

@amwx
Copy link
Contributor

amwx commented Nov 13, 2022

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)?

@grokys
Copy link
Member

grokys commented Nov 14, 2022

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.

@kekekeks
Copy link
Member

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
Copy link
Member

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

@doublecount
Copy link

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?

maxkatz6 added a commit that referenced this pull request Jan 5, 2023
…uctors-from-eventargs-from-avalonia.base"

This reverts commit a26d515, reversing
changes made to 4670536.

# Conflicts:
#	src/Avalonia.Base/Input/KeyEventArgs.cs
#	src/Avalonia.Base/Input/PointerDeltaEventArgs.cs
#	src/Avalonia.Base/Input/PointerEventArgs.cs
#	src/Avalonia.Base/Input/PointerWheelEventArgs.cs
#	src/Avalonia.Base/Input/TextInputEventArgs.cs
maxkatz6 added a commit that referenced this pull request Jan 24, 2023
…uctors-from-eventargs-from-avalonia.base"

This reverts commit a26d515, reversing
changes made to 4670536.

# Conflicts:
#	src/Avalonia.Base/Input/KeyEventArgs.cs
#	src/Avalonia.Base/Input/PointerDeltaEventArgs.cs
#	src/Avalonia.Base/Input/PointerEventArgs.cs
#	src/Avalonia.Base/Input/PointerWheelEventArgs.cs
#	src/Avalonia.Base/Input/TextInputEventArgs.cs
maxkatz6 added a commit that referenced this pull request Jan 24, 2023
…uctors-from-eventargs-from-avalonia.base"

This reverts commit a26d515, reversing
changes made to 4670536.

# Conflicts:
#	src/Avalonia.Base/Input/KeyEventArgs.cs
#	src/Avalonia.Base/Input/PointerDeltaEventArgs.cs
#	src/Avalonia.Base/Input/PointerEventArgs.cs
#	src/Avalonia.Base/Input/PointerWheelEventArgs.cs
#	src/Avalonia.Base/Input/TextInputEventArgs.cs
jmacato added a commit that referenced this pull request Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants