AttackPattern / CSharpAnalytics

Google Analytics for Windows 8, Windows Phone & desktop applications
142 stars 36 forks source link

WPF support #22

Closed angularsen closed 9 years ago

angularsen commented 9 years ago

Based on WinForms implementation.

Improvements:

Breaking changes:

Other changes:

damieng commented 9 years ago

Hi Andreas, thanks for the PR.

I'd really like to get this merged in although I'm concerned that having a single .NET45 project do WinForms, WPF and core .NET45 means we're going to be including unnecessary assemblies into users applications via references?

What are the advantages of this over having a new .Wpf project alongside the current .NET45/.WinForms ones and a separate CSharpAnalytics.Wpf NuGet package?

angularsen commented 9 years ago

The way I see it, both are already part of .NET 4.5. Worst case your app will be loading some extra dlls into memory.

I have not verified this, but I believe that if user code only references the "cross platform" classes, the JIT won't load the winform or wpf dlls into memory. And if you instantiate the WinFormsAutoMeasurement then hopefully it does not load any wpf dlls. The wpf one does depend on winforms though, due to the primary monitor stuff. Not sure if there is any easy way around that.

I'll try to verify the dll loading scenarios. If wpf and winforms do load even when not using them, then I totally agree about separate projects and nugets.

Does this match your understanding of things?

Best, Andreas

angularsen commented 9 years ago

So I did a quick experiment with Process Explorer. Running the original winforms sample application (without debugger, in debug profile) I found it loads the following assemblies:

CSharpAnalytics.Net45
CSharpAnalytics.Sample.WinForms
SMDiagnostics
System
System.Configuration
System.Core
System.Drawing
System.Net.Http
System.Runtime.Serialization
System.ServiceModel.Internals
System.Windows.Forms
System.Xml

If I then create an instance of WpfAutoMeasurement, and log it with ToString() to avoid the JIT optimizing it away, there is no change in the loaded assemblies since the constructor does nothing WPF related.

If I assign the wpf instance to the AutoMeasurement.Instance property, there are four more assemblies loaded:

PresentationCore
PresentationFramework
System.Xaml
WindowsBase

So this confirms my theory that the JIT is a lazy slob and does not load any assemblies it doesn't yet need. This also means it should be safe to include both WinForms and WPF implementations in the .NET 4.5 nuget without worrying about unnecessary assemblies being loaded. So to me, I think it comes down to this:

Don't get me wrong, I'm not trying to push through my own preference here. This is a great project and I'd be fine with both solutions. I leave this one up to you.

damieng commented 9 years ago

My plan with the multiple NuGet's is that the WinForms and WPF projects would be completely self-contained and wouldn't require the .NET45 one (which would be there for people wanting to do server-side or other project types).

Given your findings I'm okay with WinForms and WPF being included in the .NET 4.5 project/package though.

[)amien

On Mon, Oct 13, 2014 at 10:56 AM, Andreas Gullberg Larsen < notifications@github.com> wrote:

So I did a quick experiment with Process Explorer. Running the original winforms sample application (without debugger, in debug profile) I found it loads the following assemblies:

CSharpAnalytics.Net45 CSharpAnalytics.Sample.WinForms SMDiagnostics System System.Configuration System.Core System.Drawing System.Net.Http System.Runtime.Serialization System.ServiceModel.Internals System.Windows.Forms System.Xml

If I then create an instance of WpfAutoMeasurement, and log it with ToString() to avoid the JIT optimizing it away, there is no change in the loaded assemblies since the constructor does nothing WPF related.

If I assign the wpf instance to the AutoMeasurement.Instance property, there are four more assemblies loaded:

PresentationCore PresentationFramework System.Xaml WindowsBase

So this confirms my theory that the JIT is a lazy slob and does not load any assemblies it doesn't yet need. This also means it should be safe to include both WinForms and WPF implementations in the .NET 4.5 nuget without worrying about unnecessary assemblies being loaded. So to me, I think it comes down to this:

  • WinForms and WPF are both part of .NET 4.5 and so when downloading the nuget I'd expect both to be available (that is what I originally hoped it would have)
  • Single nuget
    • Pros: It's a single nuget, only one assembly reference and one nuget to update/maintain for WPF/WinForm users, only one .nuspec file and one project to maintain in repo
    • Cons: If WinForms/WPF users want to use AutoMeasurement instead of Wpf/WinFormsAutoMeasurement types directly, they'll have do a one-liner to initialize the right implementation
    • Multiple nugets
    • Pros: It's explicit what application frameworks the nuget supports, the auto-initialization can be done by code in the respective nugets
    • Cons: There will be very little code in the WinForms/WPF nugets, right now I think only the Environment and the AutoMeasurement implementations. One could argue that if WinForms/WPF gets their own nugets, that Win8/Win81/WinPhone should also have their own nugets

Don't get me wrong, I'm not trying to push through my own preference here. This is a great project and I'd be fine with both solutions. I leave this one up to you.

— Reply to this email directly or view it on GitHub https://github.com/AttackPattern/CSharpAnalytics/pull/22#issuecomment-58930101 .

angularsen commented 9 years ago

Ah, yes it would probably make more sense to have self contained nugets instead of using nuget dependencies in that case.

damieng commented 9 years ago

Okay, that sounds good.

For the AutoMeasurement I'm not keen on having to know to new stuff up but let's get this in and take it from there.

Many thanks for your contribution!