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

Make generic IViewFor<TViewModel> implementation optional in favor of non-generic IViewFor #3642

Closed
Metadorius opened this issue Oct 25, 2023 · 15 comments

Comments

@Metadorius
Copy link

Metadorius commented Oct 25, 2023

Is your feature request related to a problem? Please describe.

Windows Forms designer hates generics. Generics can't be designed properly, have to do nasty hacks to work around that. What it hates even more is inheriting generics. Most of the time you can't design the form that inherits from a generic, only once in a blue moon it can be opened (IIRC only before the first compilation, then designer breaks). The workaround for that is doing a proxy class in the inheritance chain but it's far from elegant and visual inheritance breaks as well. And it also can stop working without apparent reason.

Microsoft doesn't care about fixing that whatsoever, those bugs exist for like 20 years, and as such introducing generics in WinForms is effectively fighting WinForms. So the only proper way of doing it here is ditching generics in context of WinForms.

Describe the solution you'd like
IViewFor<TViewModel> not being a required interface to implement where not needed.

Describe alternatives you've considered
Just using generics. Led to countless hours of fighting WinForms designer.

Describe suggestions on how to achieve the feature
From a brief look in the ReactiveUI code those are the only usages (correct me if I am wrong):
image
I am not sure, but it seems that all of them except the last one are too specific because it gets upcasted to non-generic IViewFor later the call stack. I suggest to change signatures to IViewFor.

@Metadorius
Copy link
Author

One more thing. Same deal with ReactiveUserControl<TViewModel>, we need a non-generic counterpart to it. It's very easy to craft one myself but would still be nice to have it out of the box.

@glennawatson
Copy link
Contributor

We aren't likely to change the signatures in this scenario.

The advice is to generate a base class that's non-generic that implements IViewFor<T>. Eg public MyFancyViewModelUserControlBase : ReactiveUserControl<MyFancyViewModel> { }, then derive your view from that. You've got a class that accurately reflects your view model in this scenario.

We don't want a "object" based IView, it complicates a bunch of code for every other platform. It would force us to rely on more reflection then we already do. We been trying to minimise the amount of reflection used.

@Metadorius
Copy link
Author

Metadorius commented Oct 25, 2023

The advice is to generate a base class that's non-generic that implements IViewFor<T>. Eg public MyFancyViewModelUserControlBase : ReactiveUserControl<MyFancyViewModel> { }, then derive your view from that. You've got a class that accurately reflects your view model in this scenario.

Yeah this is a) blowing up the codebase with unnecessary meaningless code considerably, b) doesn't always work because as I said WinForms hate generics.

I in fact don't see why can't just changing signature of command binder methods to accept IViewFor instead of the generic counterpart work, they pass it as IViewFor without any generics? I already just do IViewFor<object> and it seems to just work from a brief testing, albeit being a bit ugly?

@glennawatson
Copy link
Contributor

Some options I can think of you can do as a alternative to IViewFor<T>

Write your own resolver. You could probably scan for classes with a property named ViewModel and determine the type from that.

If performance matters you can use a .NET source generator to do the scanning and put it into a nice dictionary inside a class. Probably will be faster lookup then the current RxUI implementation.

That way you can avoid the generic IViewFor

I don't think going down the route of having 'object' everywhere is a solution for the rxui code base though.

I'm tempted to do a optional component that does this one weekend from now. Won't be part of the main rxui though.

I'll create a separate issue to track the idea.

@Metadorius
Copy link
Author

Maybe I am using MVVM/RxUI wrong, but I personally don't use view resolution whatsoever. That's why I said "except the last one", I mean the command binder stuff only. View resolution should probably stay as is with requiring the generic counterpart.

@glennawatson
Copy link
Contributor

The view resolution is the primary reason why IViewFor exists. It relies on the generic.

@glennawatson
Copy link
Contributor

Anyway. There's no plan to introduce a non generic IViewFor.

@Metadorius
Copy link
Author

Metadorius commented Oct 26, 2023

I don't understand, I am just asking for a few methods which don't really require the IViewFor<TViewModel> to just relax the too specific input parameter types, because the moment the next method in call stack gets called this generic version is not needed but non generic IViewFor is needed. I don't ask to introduce any logic changes whatsoever, I just ask for command binder signatures to not be unnecessarily specific so I don't need a hack for it to work in my use case.

It seems like we're talking about different things.

@glennawatson
Copy link
Contributor

As mentioned we aren't doing any changes to the IViewFor.

@Metadorius
Copy link
Author

As mentioned we aren't doing any changes to the IViewFor.

I am not asking to...

@Metadorius
Copy link
Author

Re-reading your replies, sorry but I don't feel you ever understood what my request really is and just shot it down :\

@glennawatson
Copy link
Contributor

glennawatson commented Oct 26, 2023

You want to use the non generic IViewFor for the command binders or a version of the command binders that doesn't use the IViewFor at all is my understanding.

You're asking for a fundamental change to the project where it's pretty stable.

@glennawatson
Copy link
Contributor

Hopefully the linked issue will solve your problem.

@Metadorius
Copy link
Author

You want to use the non generic IViewFor for the command binders or a version of the command binders that doesn't use the IViewFor at all is my understanding.

Yes, that is what I wanted.

Hopefully the linked issue will solve your problem.

Yes. Thanks much and sorry for getting worked up!

Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants