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

Restructure IViewManager Interfaces #9469

Closed
chiaramooney opened this issue Feb 3, 2022 · 2 comments
Closed

Restructure IViewManager Interfaces #9469

chiaramooney opened this issue Feb 3, 2022 · 2 comments
Labels
Agenda Issue tagged for discussion at next weekly meeting Area: Fabric Support Facebook Fabric Area: View Managers enhancement Needs: Dev Design New Architecture Broad category for issues that apply to the RN "new" architecture of Turbo Modules + Fabric Platform: UWP Platform: WinAppSDK Recommend: Backlog Recommend that issue should be given Backlog milestone.
Milestone

Comments

@chiaramooney
Copy link
Contributor

Summary

@rozele notes -- transferred over from #9125:

Rather than adding this new interface, can we just create a new base interface for view managers that supports the entire lifecycle and property set? I think this API surface is getting quite complicated, we have:

IViewManager (FrameworkElement view base)
IViewManagerWithNativeProperties (FrameworkElement view base)
IViewManagerWithChildren (FrameworkElement view base)
IViewManagerWithDropViewInstanceEvent (FrameworkElement view base)
IViewManagerWithReactContext (any view base)
IViewManagerWithExportedViewConstants (any view base)
IViewManagerWithExportedEventTypeConstants (any view base)
IViewManagerCreateWithProperties (Object / any view base)
IViewManagerWithPointerEvents (Object / any view base)
The remaining issues we have are:

We need a corollary for Object / any view base for:
IViewManagerWithNativeProperties
IViewManagerWithChildren
IViewManagerWithDropViewInstanceEvent
We need a way to pass the root tag to the view manager at create time, so we know which XamlRoot to associate with the view for WinUI / XAML Islands.
My recommendation would be that we merge the following interfaces, since it's rare that they are not used together:

Merge IViewManager with IViewManagerCreateWithProperties, IViewManagerWithNativeProperties as it's very rare that a custom view manager would not define any of it's own properties (or else why not just use ?)
Note - 100% of our custom view managers in Messenger have custom additional props
When merging this, drop the base view type to Object or DependencyObject (preferably the former so the same compiled code can be used with either WinUI 3 or WinUI 2).
When merging this, add the tag and rootTag as arguments to CreateView so if the view manager sets up state for the corresponding tag, it can do so and can also do appropriate init for the correct XAML root associated with the root tag.
Consider also merging IViewManagerWithDropViewInstanceEvent as it's trivial to no-op this method and at least from our experience with Messenger, it's quite common to require per-view cleanup logic (more than half of our custom view managers require cleanup per view).
It's also the case that if you use IViewManagerWithExportedEventTypeConstants, you also need IViewManagerWithReactContext (or else how do you emit events). It seems redundant that you have to define both interfaces, perhaps make IViewManagerWithExportedEventTypeConstants derive from IViewManagerWithReactContext. In the case of Messenger, we actually only use IViewManagerWithReactContext for the purpose of emitting events.

So we'll probably have to support IViewManager for some time (hopefully mark it deprecated), and the new interface options would be:

IViewManager2
Object CreateView(Int64 tag, Int64 rootTag, JSValueObject props)
Map NativeProps()
void UpdateView(Object view, JSValueObject props)
void OnDropViewInstance(Object view)
IViewManagerWithReactContext
unchanged
IViewmanagerWithExportedViewConstants
unchanged
IViewManagerWithExportedEventTypeConstants2
derives from IViewManagerWithReactContext
otherwise identical to IViewManagerWithExportedEventTypeConstants
IViewManagerWithChildren2
void AddView(Object parent, Object child, Int64 index)
void RemoveAllChildren(Object parent)
void RemoveChildAt(Object parent, Int64 index)
void ReplaceChild(Object parent, Object oldChild, Object newChild)
IViewManagerWithPointerEvents
unchanged

Or, another option might be to build a base class that gets exposed through the ABI, something like:

class ViewManagerBase : winrt::implements<IViewManager, IViewManagerWithNativeProps, IViewManagerWithDropInstanceEvent, IViewManagerWithChildren, ...> {
...
}
This way you could keep the interfaces granular, but have default implementations for things like OnDropViewInstance that no-op and default implementations for AddChild/RemoveChild/etc. that throw.

We'd still need IViewManager2/IViewManagerWithNativeProps2/IViewManagerWithChildren2 to drop the view base dependency on xaml::FrameworkElement.

Motivation

Ease of use. Cleaner.

Basic Example

No response

Open Questions

No response

@ghost ghost added the Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) label Feb 3, 2022
@chrisglein
Copy link
Member

Related #5539?

@chrisglein
Copy link
Member

There's an intentional design here to avoid class inheritance as a requirement. In C# there are base classes that make this easier. But in C++/winrt there isn't a similar thing today (and perhaps additional barriers to inheritance). WinAppSDK is also coming and will impact some of these interfaces, as well as considerations for balancing UWP + WinAppSDK in tandem for some amount of time. Plus Fabric.

All that to say, there's design work to do here that should be done with all that mind.

@chrisglein chrisglein added Needs: Dev Design and removed Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) labels Feb 7, 2022
@chrisglein chrisglein added this to the Backlog milestone Feb 7, 2022
@chrisglein chrisglein added Area: Fabric Support Facebook Fabric Platform: UWP Platform: WinAppSDK Agenda Issue tagged for discussion at next weekly meeting labels Feb 7, 2022
@TatianaKapos TatianaKapos added the Recommend: Backlog Recommend that issue should be given Backlog milestone. label Aug 31, 2023
@jonthysell jonthysell added the New Architecture Broad category for issues that apply to the RN "new" architecture of Turbo Modules + Fabric label Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Agenda Issue tagged for discussion at next weekly meeting Area: Fabric Support Facebook Fabric Area: View Managers enhancement Needs: Dev Design New Architecture Broad category for issues that apply to the RN "new" architecture of Turbo Modules + Fabric Platform: UWP Platform: WinAppSDK Recommend: Backlog Recommend that issue should be given Backlog milestone.
Projects
Status: Done
Development

No branches or pull requests

4 participants