WPF Remove duplicate DelegateCommand

[amaitland]

Sign CefSharp.Example and CefSharp.Wpf.Example so can be used with InternalsVisibleTo
Make CefSharp.Wpf internals visible to CefSharp.Wpf.Example
Remove duplicate DelegateCommand
Move DelegateCommandT into CefSharp.Wpf for consistency, mark as internal

[perlun]

This one I feel needs to be discussed a bit. What is the point of moving it into the CefSharp.Wpf project? In my eyes, all "MVVM framework" things should be kept in the CefSharp.Wpf.Example project, since that's how a "normal app" using CefSharp would do it anyway. If we move it into CefSharp.Wpf, we should make it public, but I don't think that's good either (since it will collide with many other peoples implementation).

So, I feel it's best to keep it where it is. 😄 Since you obviously had a different opinion, would you care to elaborate on it a bit? I see that we had a DelegateCommand in the CefSharp.Wpf project, I guess that had to do with it. Maybe that one should be moved out of it also?

[perlun]

I see now that it's used from here: https://github.com/cefsharp/CefSharp/blob/master/CefSharp.Wpf/WebView.cs#L337

So maybe we have to have it in CefSharp.Wpf after all. We could move in DelegateCommand<T> but I'm not so fond of that idea since it means bloating CefSharp.Wpf with stuff only used in the example = not such a good idea.

Btw, happy Easter to everone! 😃 "He is risen indeed".

[amaitland]

Happy Easter!

So DelegateCommand is used internally in the WPF Webview as your previous comment refers, so I think using the implementation in CefSharp.Wpf and making it internally visible to the CefSharp.Wpf.Example makes a lot of sense.

In relation to DelegateCommand I was planning on implementing more commands directly in WebView. One of those commands being GoCommand which would require the generic delegate. The GoCommand would allow for a more xaml based approach to interfacing with WebView. Just execute the GoCommand with a CommandParameter for the Address.

Thoughts?

[perlun]

Sorry for the lag on this one. Could you post some sample XAML for how you would want the interface to be? So we can discuss the design a bit based on that perhaps.

[amaitland]

Originally I though it might be nice to have a command which you could simply pass the Url as a CommandParameter

<Button Content="Go"
        Command="{Binding WebBrowser.GoCommand}"
        CommandParameter="{Binding Text, ElementName=Address}" />

Having looked at what's involved with implementing Text Search I believe DelegateCommand could be a fairly important piece in that puzzle. I was thinking it would be nice to implement some commands directly in the WebView to support this feature. Thoughts?

Are you happy to make CefSharp.Wpf internals visible to CefSharp.Wpf.Example? Perhaps start with removing the duplicate DelegateCommand and then discuss the other architectural points once a PR merging that change has been submitted?

[JanEggers]

i think amaitland changes go in the right direction

first i hate duplicated code
second WebView a WPF Control and should support mvvm with the least external code possible so having commands ready for command binding is a good thing

i think we should create a mvvm namespace in the wpf project and place the commands there as public classes that way we can use em in the wpf project and example and it will only collide for other users if they add the mvvm namespace to their usings which is not required if they just want to use the control

[amaitland]

@JanEggers That sounds like a sensible compromise. I think @perlun's objection somewhat relates to DelegateCommand<T> only being used in the Wpf.Example project and not in the WebView it's self. Personally I can see a couple of scenarios where I think it would be useful.

This PR has gone stale, so I'll update and incorporate the changes as discussed.

Thanks for taking the time to review and provide feedback, much appreciated!

[perlun]

Hi,

Here's my opinions on this. Sorry for being a bit away w/ regard to this, and much other CefSharp stuff lately. There's just been "too much of everything".

First the comment from @amaitland:

Are you happy to make CefSharp.Wpf internals visible to CefSharp.Wpf.Example? Perhaps start with removing the duplicate DelegateCommand and then discuss the other architectural points once a PR merging that change has been submitted?

I must say, absolutely no. In my eyes, CefSharp.Wpf.Example should be treated as "just another random project which happens to use CefSharp". In that sense, InternalsVisibleTo is a no-no; you can just make the type public in the first place (which we should definitely not do, for reasons which I will explain a few moments).

You should think of like this: CefSharp.Wpf.Example is the "canonical" example of how to make a (simple but fully functional) web browser using CefSharp. Letting it use internal parts of the framework unavailable to others will just cause confusion, from how I see it; people will try to do the same, but will get an "type not visible" or whatever when they try to copy over example code from CefSharp.Wpf.Example to their own project. I don't like that.

Then some of the comments from @JanEggers:

first i hate duplicated code

Me too. I love the DRY principle and try to live by it. But then again, I think duplication of code within a project is different beast than duplicating stuff in different projects. As explained above, you should not see the CefSharp.Wpf.Example as part of the CefSharp "code base". It just happens to live in the CefSharp solution because it makes it easier to develop the example, but perhaps we ought to split it out into a separate solution just to make it clearer... 😜 (@jornh - this is not really a serious suggestion from my end, but what do you think about the idea?)

second WebView a WPF Control and should support mvvm with the least external code possible so having commands ready for command binding is a good thing

Agreed.

i think we should create a mvvm namespace in the wpf project and place the commands there as public classes that way we can use em in the wpf project and example and it will only collide for other users if they add the mvvm namespace to their usings which is not required if they just want to use the control

Again, absolutely not from my POV. That's one of the most frustrating things you can run into when you use some MVVM framework (which I suppose most WPF apps do), that it has naming collisions with 3rd party libraries (like CefSharp). It makes ReSharper a lot more awkward to use, for example (which DelegateCommand should you chose from its list of suggested namespaces?).

So, I am very, very strongly opposed to this. That mistake has already been done by other parties in the past and I've gotten annoyed with them; I won't accept us repeating that mistake in CefSharp also... 😃

[amaitland]

Letting it use internal parts of the framework unavailable to others will just cause confusion, from how I see it; people will try to do the same, but will get an "type not visible" or whatever when they try to copy over example code from CefSharp.Wpf.Example to their own project. I don't like that

@perlun When you put it like that it puts the subject in a totally different context, thank you for taking the time to outline your thoughts in such detail! In one way or another it seems the issue of having adequate tools to support MVVM has come up a few times. Is there scope to include a dependency on a third party MVVM framework?

[perlun]

You're welcome. 😃 I had some time while waiting for a ferry...

Sounds like an idea. We must be careful so that only the example gets dependent on it though. Which one would you prefer, MVVM Light? It should be something available through NuGet, preferably, and with a reasonable architecture, IMHO.

[amaitland]

MVVM Light was indeed the framework I had in mind.

[amaitland]

I'm happy to close this PR.

[jornh]

(@jornh - this is not really a serious suggestion from my end, but what do you think about the idea?)

I think we want to keep the Examples in this repo for the foreseeable future:

• There is less overhead in quickly adding demo code (and in some respects works as a poor mans - or at least people with limited time - stand-in for test code + documentation; you can use the source, Luke)
• it might work as a gateway drug to attract contributors... "Hey, I've already cloned this repo to get the samples, I might as well hack on it" (I think that's what happened to me a few months back 😉)

I'm happy to close this PR.

Done ... inching towards my personal goal for this week of getting < 10 open PR's. Still it wasn't wasted as it triggered a valuable discussion we can point to in the future and it set expectations for the Examples straight with @perlun's thorough words. It also triggered an idea for adding MVVM Light as a WPF Example dependency for the future (after 31.0.0 ?). We still have the MinimalExample repo for the vanilla WPF flavor.

[perlun]

Still it wasn't wasted as it triggered a valuable discussion we can point to in the future and it set expectations for the Examples straight with @perlun's thorough words.

Thank you. 😄

It also triggered an idea for adding MVVM Light as a WPF Example dependency for the future (after 31.0.0 ?).

Possibly; it depends on when someone wants to do it (and also on how long 31.0.0 is delayed. 😉 I have an upcoming multi-hours flight a few weeks away... 😆)

We still have the MinimalExample repo for the vanilla WPF flavor.

Yes, and I think it makes sense to keep that "vanilla WPF" like you say it.

We should also add a Minimal WinForms example there at some point.

[jornh]

and also on how long 31.0.0 is delayed ...

To me 31.0.0 need not be delayed from the date currently set. My understanding of 'stable' does not require feature-complete if getting the JS-binding done is what you are referring to. I think with both dispose and WinForms - plus other stuff in the ChangeLog - what is there warrants a "first non beta". It is quite easy to communicate that #159 is the one big thing missing (for those who need that) but we think everything else is good

But alternatively we could also make another -pre to get it into the hands of current NuGet users. Or what do you think it takes to declare "first non beta"? I just think it would be nice to know 😄

We should also add a Minimal WinForms example there at some point.

I have an ultra minimal one locally I can make into a PR.

[perlun]

I don't know, Jørn. For me, #159 is a must - in the app I have currently in production, there's no way around that, so "for me personally", 31.0.0 is not stable until #159 is in place. It is in that sense still inferior to 1.25.x (but it's of course superior in many other aspects. 😄)

So I dunno, I kind of feel that we should wait with 31.0.0 until we reach the goal with #159. I will have some "extra time" in early June (spending time at airports and similar...) so in the best of cases, I might be able to squeeze in a couple of hours on cooperating with @JanEggers in getting that one ready.