[elachlan]

Fixes #269

[CLAassistant]

All committers have signed the CLA.

[elachlan]

I think it might be a good idea to change the config/log fields in DefaultEnvironmentInfoCollector to be protected so that inherited classes can log warnings and check the config without holding a reference themselves. This would change the API surface though, so I'd thought I'd raise it first.

Additionally, we can use a lazy loaded static System.Diagnostics.Process field and then call its refresh() method once per GetEnvironmentInfo() call to reduce the expense per call.

[elachlan]

If we lazy load Process, we need to implement a dispose method as we access Process.Handle.
https://stackoverflow.com/questions/16957320/what-does-process-dispose-actually-do

[niemyjski]

Thanks for the PR, I will talk to the rest of the team about taking a dependency on win32. We do support this library on .NET 5+ even if it's windows only currently, it may not be some day. Also need to discuss if we feel like it's a good option to create new first-class properties that we would need to support everywhere. I feel like these properties probably should be in a data bag.

[elachlan]

Thanks for the PR, I will talk to the rest of the team about taking a dependency on win32. We do support this library on .NET 5+ even if it's windows only currently, it may not be some day. Also need to discuss if we feel like it's a good option to create new first-class properties that we would need to support everywhere. I feel like these properties probably should be in a data bag.

I'd like to hope that one-day winforms would run on platforms other than windows. I did ask Microsoft on GitHub about it. But was told winforms will forever be windows only due to its tight coupling with win32.

In WINE calls to these APIs will throw not implemented exceptions I think.

In any case, we can detect the platform and only populate the variables when it's running on windows. Handles are windows specific and so wouldn't make sense to attempt to be populated on Linux.

I'll move the properties to the databag, not a problem.

[ejsmith]

Thanks for the contribution @elachlan! Looks good, just a couple changes and we can get this merged.

[elachlan]

Thanks for the contribution @elachlan! Looks good, just a couple changes and we can get this merged.

Changes are applied, let me know what you both think.

[ejsmith]

LGTM thank you @elachlan

[elachlan]

The only nitpick I have is that GetGuiResources has the ability to query the peak count on GDI and User objects. Which might be helpful. But as per the documentation for it, it isn't supported on older windows versions:
Windows Server 2008, Windows Vista, Windows Server 2003 and Windows XP: This value is not supported until Windows 7 and Windows Server 2008 R2.

That being said, I don't think that matters because of the requirement for .NET 4.6.2 by this package. Which is Win 7 Minimum.

[niemyjski]

Thanks for the PR!

[elachlan]

@niemyjski thank you for the review! Are you able to do a release sometime soon?

[niemyjski]

We typically like to let a nightly sit for a little bit (week or two), we have a nightly NuGet feed you can use.