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

Fix EventCallback Net8.0 #150

Merged
merged 4 commits into from
Oct 27, 2024

Conversation

Denny09310
Copy link
Contributor

During testing of the library, I discovered that EventCallbacks in .NET 8 are not invoking the Equals method correctly, which is documented in this issue. For instance, setting the OnLoaded EventCallback triggers an infinite loop as it continually sets and unsets the callback. This behavior also occurs with other callbacks, such as OnClick.

To work around this issue, I implemented a utility class to compare EventCallback instances. While this is a temporary solution, it appears necessary as the two properties required to check equality are internal, and the fix has been moved to .NET 10 as noted in the issue.

@Dreamescaper
Copy link
Owner

I'll take a closer look. It's one thing that it performs unnessessary renders (like mentioned in the original issue). But even with that it shouldn't fall into infinite loop.

What issues did you have with the exact version VS "$(MauiVersion) ?

@Denny09310
Copy link
Contributor Author

Denny09310 commented Oct 26, 2024

What issues did you have with the exact version VS "$(MauiVersion) ?

The project template (as the other maui templates) specify a $(MauiVersion) that is synced with the maui workload version. If we specify a fixed version inside the nuget package while the template still uses the $(MauiVersion) it could cause a "Detected package downgrade" exception (like it happened to me) because the $(MauiVersion) installed through the workload has a different version. With this we still assure that even with different workload version installed on the developer PC it uses the correct version without causing any issue. The only part that I'm not sure is working, is if we use the $(MauiVersion) variable inside a non-maui. In fact I divided the commits because it give me a strange warning and I don't know if I should ignore it as the project is still working fine

@Dreamescaper
Copy link
Owner

I think it is fine to leave it as exact version.
$(MauiVersion) is build time value anyway, version conflict is still possible, as workload version on Git build server and on user's PC do not have to be in sync.
Anyway, if you have other thoughts here - let's discuss them in a separate item.

@Dreamescaper
Copy link
Owner

Regarding EventCallbacks changes. I suggest to add two methods to NativeControlComponentBase:

    protected static bool Equals(EventCallback e1, object e2) { ... };
    protected static bool Equals<T>(EventCallback<T> e1, object e2) { ... };

This way no generator changes are needed, and it will be easier to remove them later.

Regarding equality check itself. Let's remove all that expression and reflection stuff, and use UnsafeAccessorAttribute to get private members.

@Denny09310
Copy link
Contributor Author

I think it is fine to leave it as exact version. $(MauiVersion) is build time value anyway, version conflict is still possible, as workload version on Git build server and on user's PC do not have to be in sync. Anyway, if you have other thoughts here - let's discuss them in a separate item.

Ok I'll revert the changes when possible

@Denny09310
Copy link
Contributor Author

Regarding EventCallbacks changes. I suggest to add two methods to NativeControlComponentBase:

    protected static bool Equals(EventCallback e1, object e2) { ... };
    protected static bool Equals<T>(EventCallback<T> e1, object e2) { ... };

This way no generator changes are needed, and it will be easier to remove them later.

Regarding equality check itself. Let's remove all that expression and reflection stuff, and use UnsafeAccessorAttribute to get private members.

I've never used this attribute I'll inform myself on that and replace with what you suggested

@Dreamescaper
Copy link
Owner

Could you provide a repro project, illustrating the issue?
I can reproduce unnessessary reasignments (so the changes you're suggesting make sense one way or another), but I don't understand how to make it fall into an infinite loop.

For future cases, please create an issue before submitting a PR with a fix. It is easier to discuss an issue itself separatelt from implementation details.

@Denny09310
Copy link
Contributor Author

Yes sorry. Thought that this was a quick fix. Let's do this, let's close this, I'll open an issue with the repo to try the issue and then create a PR when I'll manage to implement the UnsafeAccessor. Sounds good?

@Dreamescaper
Copy link
Owner

You can leave it open, up to you.

As for UnsafeAccessor - I think you need to use the exact type for the method parameter, not just object.

@Denny09310
Copy link
Contributor Author

Yes was just modifying that, only exception is giving me is some kind of "Bad Format" I'll try fix it later

@Denny09310
Copy link
Contributor Author

Unfortunately generic parameters are not supported yet, only .NET 9. See this issue

@Dreamescaper
Copy link
Owner

I see, that's unfortunate.
Considering that's fixed in net9.0, and net9.0 is releasing in two weeks, I suggest to leave it commented, and uncomment after net9.0 update.

@Dreamescaper Dreamescaper merged commit 1932978 into Dreamescaper:main Oct 27, 2024
1 check passed
@Dreamescaper
Copy link
Owner

Thanks a lot for your contribution!

@Denny09310 Denny09310 deleted the fix_event_callback branch October 31, 2024 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants