search

hardcodet / wpf-notifyicon

NotifyIcon (aka system tray icon or taskbar icon) for the WPF platform
Other
811 stars 126 forks source link

Code modernising #8

Closed Lakritzator closed 4 years ago

Lakritzator commented 4 years ago

These changes should improve the readability and stability of the code base, also when moving / modifying. Especially not using hard-coded strings.

As the project was already at C# 7.3, I modified some of the code to use more of the features, but only if these improve readability. I could have been a lot more aggressive, but assume that would be to much.

Removed the "this." references where possible, as it's unneeded from my standpoint. Moved 2 classes in their own files, it's a common convention not to have multiple classes in one file.

In TaskbarIcon there was a lot of lock(this), which is bad practice (but most likely not an issue here), I replaced this with a lock on a separate object. This might increase the memory usage with a few bytes, but I would advice to do it anyway.

Also fixed some typos in documentation.

What is missing, are some code / naming conventions (like are private fields prefixed with an _ or not etc)

Just look if you like these changes

punker76 commented 4 years ago

@hardcodet LGTM 👍 @Lakritzator

Lakritzator commented 4 years ago

BTW: I was "struggling" with my decision of applying "var", but didn't. I'm not a fan of var everywhere but it has it usages... It's already used in some places... but I don't know your view on it.

Like I said, I could have made more aggressive changes :grimacing:

hardcodet commented 4 years ago

BTW: I was "struggling" with applying "var", and didn't, I'm not a fan of var everywhere but it has it usages... And it's already used in some places...

@Lakritzator ACK, I use var if I can infer the type from looking at the code, otherwise I don't:

var user = new User("john");
User user = await GetUser("john");

Edit: When in doubt, I'd rather keep the type. IMO there's not a big win going through the code now and cherry picking suitable usages (unless ReSharper can properly do that now - it used to blindly suggest var everywhere the last time I checked).

Lakritzator commented 4 years ago

Some other things I noticed: Currently the solution is targeting many framework versions, are these needed? I guess you can reduce it on the essence, like 4.5 & 4.7.1 or so...

I found some sub-optimal interop calls, from a performance & memory perspective, but I think this doesn't need to be changed as it's not a critical path. I will check this another time.

Lakritzator commented 4 years ago

As @punker76 is involved, I would expect him to work on Azure Pipeline support too, for making automatic builds & publishing to nuget (if approved), if not I can help.

AppVeyor would be an alternative, but due to the gigantic forward momentum which Microsoft has, my advice would be to use Azure Pipelines.

Your organisation will need an "account" though, but it's free for open source and solves many issues with building locally.

hardcodet commented 4 years ago

Yes, this code has been unchanged for years now, it can most certainly use some brushing up with regards to targets. Where possible (and easy!), I'd like to support older framework versions, but given it's been mostly stable, I'm not against cutting off some old version at all.

Lakritzator commented 4 years ago

Actually you don't seem to be using any >.NET 4.5 features... it might be okay to leave that in for now and loose all the rest. Why build a library for many different frameworks when there is no difference? Anyway, that would be something which @punker76 can decide on.

The only part I found which has any different "framework" code is this: https://github.com/hardcodet/wpf-notifyicon/blob/master/Hardcodet.NotifyIcon.Wpf/Source/NotifyIconWpf/Interop/WindowMessageSink.cs#L188 I actually would remove this, okay? Silverlight :laughing:

hardcodet commented 4 years ago

@Lakritzator I'm sure Silverlight will have a massive comeback one day. Not :)

Feel free to kill that code 👍 👍 👍

hardcodet commented 4 years ago

@Lakritzator - @punker76 I created an Azure pipeline, feel free to take over (Azure created a PR on the fly here, I'll add you guys as reviewers).

Jan, I saw that you already added a NuGet issue here, so I guess you'll know what to do there? @Lakritzator, if you'd like, I'll gladly add you to Azure as well, I'd need an email address though to send you an invite.

punker76 commented 4 years ago

Actually you don't seem to be using any >.NET 4.5 features... it might be okay to leave that in for now and loose all the rest. Why build a library for many different frameworks when there is no difference? Anyway, that would be something which @punker76 can decide on.

The only part I found which has any different "framework" code is this: https://github.com/hardcodet/wpf-notifyicon/blob/master/Hardcodet.NotifyIcon.Wpf/Source/NotifyIconWpf/Interop/WindowMessageSink.cs#L188 I actually would remove this, okay? Silverlight 😆

Maybe net40;net45;net462 and later netcoreapp3.0 could be enough (net462 has some fixed DPI chamges inside).

hardcodet commented 4 years ago

Actually you don't seem to be using any >.NET 4.5 features... it might be okay to leave that in for now and loose all the rest. Why build a library for many different frameworks when there is no difference? Anyway, that would be something which @punker76 can decide on. The only part I found which has any different "framework" code is this: https://github.com/hardcodet/wpf-notifyicon/blob/master/Hardcodet.NotifyIcon.Wpf/Source/NotifyIconWpf/Interop/WindowMessageSink.cs#L188 I actually would remove this, okay? Silverlight 😆

Maybe net40;net45;net462 and later netcoreapp3.0 could be enough (net462 has some fixed DPI chamges inside).

Sounds good! I don't see a compelling reason to continue supporting CLR 2. Unless Silverlight vnext will support it, of course :p

Lakritzator commented 4 years ago

Thanks, it has been fun! :smiley: