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 downcasts in BlazorWebView #5984

Merged
merged 5 commits into from
Apr 13, 2022
Merged

Remove downcasts in BlazorWebView #5984

merged 5 commits into from
Apr 13, 2022

Conversation

Eilon
Copy link
Member

@Eilon Eilon commented Apr 11, 2022

Instead, put "SendXyz" methods on the IBlazorWebView interface to enable the handlers to raise events on the control. The control has events for users to wire-up, or alternative systems can implement the interface with another event/command pattern if desired.

Motivation:

  • The concrete class BlazorWebView is what a MAUI user will program against. As such, it has friendly "events" that they can handle and perform various customizations
  • The IBlazorWebView interface is the minimum set of APIs needed for a Handler to interact with a concrete usable type, such as MAUI's built-in BlazorWebView, or perhaps some other variation that could be used in Comet or in some hypothetical future version of Hybrid Blazor Bindings. In such non-MAUI places, using events or even structuring the properties the way they are is not necessarily the best model, so they can choose to implement those features however they want (or even omit some that they don't want to use)
  • The handlers must talk only to the interface. This enables others to build their own handlers that talk to the interface and can be used with any concrete implementation. For example, a new Tizen BlazorWebViewHandler would talk to the interface and be 100% usable with the existing BlazorWebView MAUI control.

Related: #5904, #5905

Instead, put "SendXyz" methods on the IBlazorWebView interface to enable the handlers to raise events on the control. The control has events for users to wire-up, or alternative systems can implement the interface with another event/command pattern if desired.
@Eilon Eilon added the area-blazor Blazor Hybrid / Desktop, BlazorWebView label Apr 11, 2022
@Eilon Eilon requested a review from a team as a code owner April 11, 2022 22:44
@rmarinho
Copy link
Member

I would drop the "Send"

Copy link
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense and looks good!

As for whether or not to remove the Send name prefixes, I'd be in favour of removing them.

@SteveSandersonMS
Copy link
Member

About these names:

SendBlazorWebViewInitializing
SendBlazorWebViewInitialized

Do we need to say it's a BlazorWebView here? Could they be named just WebViewInitializing/WebViewInitialized?

I mean I know it could even go as far as Initializing/Initialized but at that point there's a high risk of clash. Since their purpose is to indicate that the external native webview is initializing/initialized (as opposed to the MAUI control) it seems reasonable to have WebView in the name still.

@Eilon
Copy link
Member Author

Eilon commented Apr 12, 2022

Do we need to say it's a BlazorWebView here? Could they be named just WebViewInitializing/WebViewInitialized?

Right now it matches the event names, which I think is valuable. Or are you suggesting renaming the events, too?

I would drop the "Send"

I will try to see if that's possible, but it might not work because the control has concrete events X, Y, and Z, and then also interface implementations for methods named X, Y, and Z. I doubt C# allows that.

@Eilon
Copy link
Member Author

Eilon commented Apr 12, 2022

OK, the Send prefix is gone. I guess C# is OK with having the same name on an event and an interface-implemented method!

@Eilon
Copy link
Member Author

Eilon commented Apr 12, 2022

BTW I didn't rename the methods/event names to drop BlazorWebView. We should discuss that separately because it's a somewhat unrelated change. I think keeping the method names the same as the event names (or extremely similar) is critical. But I'd be fine if we agreed to rename all of them to some other pattern.

@Eilon Eilon enabled auto-merge (squash) April 12, 2022 21:23
@Eilon Eilon merged commit cb06d9e into main Apr 13, 2022
@Eilon Eilon deleted the eilon/remove-blazor-downcasts branch April 13, 2022 00:07
@github-actions github-actions bot locked and limited conversation to collaborators Dec 21, 2023
@samhouts samhouts added the fixed-in-6.0.300-rc.2 Look for this fix in 6.0.300-rc.2! label Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Blazor Hybrid / Desktop, BlazorWebView fixed-in-6.0.300-rc.2 Look for this fix in 6.0.300-rc.2!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants