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

New changes #45

Merged
merged 7 commits into from
Mar 12, 2018
Merged

New changes #45

merged 7 commits into from
Mar 12, 2018

Conversation

tym32167
Copy link
Contributor

@tym32167 tym32167 commented Mar 3, 2018

What was done:

  1. Now it will use async stuff instead of creation new Thread per connection - this will economy resources. Tested on WPF app and .NET Core Web app - worked fine.
  2. Moved to .NET Standard 2.0. Worked fine on WPF (fw 4.6.1) and .NET Core 2 apps. Not sure about Nuget Packaging - not checked
  3. small refactoring
  4. added proper gitignore
  5. client version was changed to fw 4.6.1

regarding support. Keep in mind that according this
.NET Standard 2.0 is supported on the following platforms (source):

  • .NET Framework 4.6.1
  • .NET Core 2.0
  • Mono 5.4
  • Xamarin.iOS 10.14
  • Xamarin.Mac 3.8
  • Xamarin.Android 7.5
  • Upcoming version of UWP (expected to ship later this year)

As result:

  • less eating resources and processor (because I removed thread creation, no need 10 threads for 10 connections)
  • becomes cross-platform

Need to keep in mind that for .NET Core 2 apps support for Encoding 1252 should be setted up manually (SO).

  • nuget package System.Text.Encoding.CodePages needs to be installed
  • provider should be registered Encoding.RegisterProvider(CodePagesEncodingProvider.Instance);

@tym32167
Copy link
Contributor Author

tym32167 commented Mar 8, 2018

Hi, @marceldev89. Can you pls take a look on this?

@marceldev89
Copy link
Owner

Yep, I will take a look this weekend. :)

@marceldev89 marceldev89 self-requested a review March 11, 2018 16:44
Copy link
Owner

@marceldev89 marceldev89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like I said before, I can't really test this and it's been a long while since I've worked with C# so I'm not up to date and all that but it seems fine to me besides some API breaking changes. If they are reverted then I'll merge it in. :)

Also, I assume the Encoding 1252 nuget stuff is only required for the .NET Core side of things right?

@@ -105,7 +105,7 @@ public enum BattlEyeCommand
/// loadEvents - (Re)load createvehicle.txt, remoteexec.txt and publicvariable.txt
/// </summary>
[Description("loadEvents")]
loadEvents,
LoadEvents,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Breaking API change

/// <summary>
/// admins - Gets connected RCON clients.
/// </summary>
[Description("admins")]
admins,
Admins,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Breaking API change

@tym32167
Copy link
Contributor Author

  • About testing - me and ~10-40 people using this (with my app https://github.com/tym32167/arma3beclient) on daily basis, so we testing it. But this is part of my sources, I cannot test nuget package.
  • About breaking changes - this is just typo... I cannot live when all of commands started from upper case and only 2 of them from lower case. If you dont agree with it - sure, I accepting it, but I dont want to change it :)
  • yep, 1252 - only for .Net Core 2

About breaking change - .Net Standard 2 will not be compatible with .Net Framework less than .NET Framework 4.6.1 - this is very important to know. Also I had plans to add another very annoying breaking change - related to #34.

Actually, this pull request and that commit - everything what I wanted to add - so if you want to have backward compatibility with old .NET FW and you dont want to fix typo in API - then no reason for me to create pull request at all.

@marceldev89
Copy link
Owner

I have a different idea for the enum changes; instead of removing the lowercase entries, add the uppercase ones and mark the lowercased as obsolete with the ObsoleteAttribute. Then if there will ever be a 2.x version that justifies API changes they can be removed.

I'm not too worried about the .NET Framework 4.6.1+ requirement, it's nearly 3 years old and anyone using the library that hasn't updated their tool in those 3 years will probably also not update the library.

As for that commit related to #34, it's already part of this PR and I kinda missed that. I'm not sure what to do here, I do feel that the library should keep handling the recovery after a connection loss and not move that to the user. The stack overflow related to that must be fixed though. If you can remove this commit from the PR, at least for now, and if the above is fine with you then I'll do the merge.

@tym32167
Copy link
Contributor Author

tym32167 commented Mar 12, 2018

About enum - hmm, that make sense. I will do this.
Commit for #34. This is annoying because stackoverflow exception impossible to handle and it killing process each time. Actually, I have different solution for keeping connection alive - I using timer for this - take a look on it and if you will like it, I can add it to your library as well.

@tym32167
Copy link
Contributor Author

Take a look on this again pls. Seems I fixed all stuff which you asked. (added 2 new commits)

@marceldev89
Copy link
Owner

Yep, nice. Thanks for the work, I'll take a look at the timer/recursion stuff at a later time. Maybe there are other solutions as well but I'll have to find some time and a server to test on. :)

@marceldev89 marceldev89 merged commit ea68d56 into marceldev89:master Mar 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants