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

Reconstructed Project #57

Closed
wants to merge 4 commits into from
Closed

Reconstructed Project #57

wants to merge 4 commits into from

Conversation

lgund
Copy link

@lgund lgund commented Aug 8, 2020

Hey @nikeee ,

first of all thanks for your work. You did a nice job and that safes me a lot of time. I hope you aren't angry cause I´ve reconstructed your project. I needed a lot of time to see the "red line" in your code and therefore I created this structure. Also I´ve changed all your asynchronous methods with the prefix async at the end. So in future everybody nows that this method is async and need to await it. All other changes I will explain now:

Events

    /// <summary>
    /// Events for handling in the correct moment
    /// </summary>
    public EventHandler OnConnectionLost;
    public EventHandler OnConnected;
    public EventHandler OnDisconnected;

I´ve added three events. They are selfexplain and fixes this issue #45

Connect to Teamspeak Telnet Query

    var headline = await _reader.ReadLineAsync().ConfigureAwait(false);
    if(headline != "TS3")
    {
        throw new QueryProtocolException("Telnet Query isn't a valid Teamspeak Query");
    }

I want to know if the query telnet is really a teamspeak query so I check if the headline start with TS3. It works nice for me and should fix your problem with issue #33

Bugfix: Client is banned

    private static QueryError ParseError(string errorString)
    {
        // Ex:
        // error id=2568 msg=insufficient\sclient\spermissions failed_permid=27
        if (errorString == null)
            throw new ArgumentNullException(nameof(errorString));
        errorString = errorString.Remove(0, "error ".Length);

        var errParams = errorString.Split(' ');
        /*
            id=2568
            msg=insufficient\sclient\spermissions
            failed_permid=27
        */
        var parsedError = new QueryError { FailedPermissionId = -1 };
        for (int i = 0; i < errParams.Length; ++i)
        {
            var errData = errParams[i].Split('=');
            /*
                id
                2568
            */
            var fieldName = errData[0].ToUpperInvariant();
            switch (fieldName)
            {
                case "ID":
                    parsedError.Id = errData.Length > 1 ? int.Parse(errData[1], NumberStyles.AllowLeadingWhite | NumberStyles.AllowTrailingWhite, CultureInfo.CurrentCulture) : -1;
                    continue;
                case "MSG":
                    parsedError.Message = errData.Length > 1 ? errData[1].TeamSpeakUnescape() : null;
                    continue;
                case "FAILED_PERMID":
                    parsedError.FailedPermissionId = errData.Length > 1 ? int.Parse(errData[1], NumberStyles.AllowLeadingWhite | NumberStyles.AllowTrailingWhite, CultureInfo.CurrentCulture) : -1;
                    continue;
                case "EXTRA_MSG":
                    parsedError.ExtraMessage = errData.Length > 1 ? errData[1].TeamSpeakUnescape() : null;
                    continue;
                default:
                    throw new QueryProtocolException();
            }
        }
        return parsedError;
    }

I´ve found a bug that happens when you failed the login to often. In that case you will get banned from the query but with a EXTRA_MSG parameter. But you don't catch this error in your switch for now and I get a QueryProtocolException instead of a QueryExecption. That sucks for me and is here fixed.

I hope I didn't forgott to much but you can ask me when there are some questions.

@nikeee
Copy link
Owner

nikeee commented Aug 8, 2020

Thanks fo your contribution, I really appreciate it.

Restructuring everything is a breaking change. Could you split this PR?

@lgund
Copy link
Author

lgund commented Aug 8, 2020

I am not sure how I split this PR. I know this code isn't compatible with the lower versions but it can be easy updated. Maybe we should make a bigger version step and mark this as not compatible with previously versions. And if you want to add ssh support #58 you need to make a pr wich will for sure break the branch compatiblity.

I did the same here in a php ts3lib and they had the same problem. This pr isn't accepted until now 👎

Edit: Maybe then create a new branch

@nikeee nikeee closed this Aug 8, 2020
@lgund
Copy link
Author

lgund commented Aug 8, 2020

Why closed this pr?

@nikeee
Copy link
Owner

nikeee commented Aug 8, 2020

I thought we'll work through this in multiple PRs. This will also make reviewing easier.

@lgund
Copy link
Author

lgund commented Aug 8, 2020

But how can I create a new folder structure clearly?

@nikeee
Copy link
Owner

nikeee commented Aug 8, 2020

Well, the fixed bugs and Async-suffixes could be filed as a PR (besides the folder structure).

@lgund lgund mentioned this pull request Aug 17, 2020
Merged
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.

2 participants