Added support for input transfer during logging #92

AlexanderK666 · 2024-07-17T14:07:12Z

Now when logging commands in the console, input transfer works, which improves user interaction with the server console, where logs are written in a thread and begin to interfere with viewing the current input.

Tested on LocalAdmin v. 2.5.14 and Windows.

Now when logging commands in the console, input transfer works, which improves user interaction with the server console, where logs are written in a thread and begin to interfere with viewing the current input.

AlexanderK666 · 2024-07-17T14:14:42Z

Oh, I uploaded the changes from the wrong folder, I'll re-upload them now

AlexanderK666 · 2024-07-17T14:19:43Z

Oh, GitHub Desktop broke the commit.

iamalexrouse

Overall, this feels a bit bulky, but, as long as it can be tested on both Windows & Linux (Which I'll do now), should be ok.

iamalexrouse · 2024-07-17T15:36:20Z

Core/LocalAdmin.cs

+        var headerLines = new string[] {
+            $"SCP: Secret Laboratory - LocalAdmin v. {VersionString}",
+            string.Empty,
+            "Licensed under The MIT License (use command \"license\" to get license text).",
+            "Copyright by Łukasz \"zabszk\" Jurczyk and KernelError, 2019 - 2024",
+            string.Empty,
+            "Type 'help' to get list of available commands.",
+            string.Empty,
+        };
+
        ConsoleUtil.Clear();
-        ConsoleUtil.WriteLine($"SCP: Secret Laboratory - LocalAdmin v. {VersionString}", ConsoleColor.Cyan);
-        ConsoleUtil.WriteLine(string.Empty, ConsoleColor.Cyan);
-        ConsoleUtil.WriteLine("Licensed under The MIT License (use command \"license\" to get license text).", ConsoleColor.Cyan);
-        ConsoleUtil.WriteLine("Copyright by Łukasz \"zabszk\" Jurczyk and KernelError, 2019 - 2024", ConsoleColor.Cyan);
-        ConsoleUtil.WriteLine(string.Empty, ConsoleColor.Cyan);
-        ConsoleUtil.WriteLine("Type 'help' to get list of available commands.", ConsoleColor.Cyan);
-        ConsoleUtil.WriteLine(string.Empty, ConsoleColor.Cyan);
+
+        foreach (var line in headerLines)
+        {
+            ConsoleUtil.WriteLine(line, ConsoleColor.Cyan, inputIntro: false);
+        }


I don't see why this needs to happen as you're doing what is effectively the same job, but I would've thought it'd be better to use a verbatim string instead which looks a bit cleaner imo.

But I'll let @zabszk decide.

In my opinion it's an unnecessary array allocation and foreach loop. You use more resources just to achieve the same effect.

iamalexrouse · 2024-07-17T15:37:24Z

Core/LocalAdmin.cs

+                if (_commandsHistory.Contains(command))
+                {
+                    _commandsHistory.Remove(command);
+                }
+                else if (_commandsHistory.Count == CommandsHistorySize)
+                {
+                    _commandsHistory.RemoveAt(_commandsHistory.Count - 1);
+                }


This can be crushed for better readability.

Suggested change

if (_commandsHistory.Contains(command))

{

_commandsHistory.Remove(command);

}

else if (_commandsHistory.Count == CommandsHistorySize)

{

_commandsHistory.RemoveAt(_commandsHistory.Count - 1);

}

if (_commandsHistory.Contains(command))

_commandsHistory.Remove(command);

else if (_commandsHistory.Count == CommandsHistorySize)

_commandsHistory.RemoveAt(_commandsHistory.Count - 1);

Yes, it does look better that way

Then you can just hit commit to add this 👍 👍

Why do you use Contains check? It's completely redundant as Remove already checks that for you and returns a bool.

Just take a look at the docs: https://learn.microsoft.com/en-us/dotnet/api/system.collections.generic.list-1.remove?view=net-8.0

and how List.Remove() is implemented in .NET 8:

// Removes the first occurrence of the given element, if found. // The size of the list is decreased by one if successful. public bool Remove(T item) { int index = IndexOf(item); if (index >= 0) { RemoveAt(index); return true; } return false; }

Why do you use Contains check?

I replaced this in the improved readability commit 4 days ago.

iamalexrouse · 2024-07-17T15:38:38Z

Core/LocalAdmin.cs


                if (!_processRefreshFail && _gameProcess != null)
                {
                    try
                    {
                        if (_gameProcess.HasExited)
                        {
-                            ConsoleUtil.WriteLine("Failed to send command - the game process was terminated...", ConsoleColor.Red);
+                            ConsoleUtil.WriteLine("Failed to send command - the game process was terminated...", ConsoleColor.Red, inputIntro: false);


You don't really need the inputIntro as far as I remember. I'm not gonna make the exact same comment to every single instance of this specific comment.

Ok, it just seems to me that without it, an unnecessary input line will appear

Interesting. I've never seen that behaviour.

zabszk · 2024-07-19T16:07:54Z

Hi,

Unfortunately this PR makes LA unusable in environments that support only stdin, stdout and stderr, just like Pterodactyl (which is the most popular way of launching SCP:SL servers). It doesn't support ReadKey. I just tested your PR there and it was not possible to run any command in Pterodactyl. Furthermore the output is completely broken there. I attached a few screenshots below.

Additionally I have noticed a significant negative performance impact.

I tested it on regular Ubuntu as well. It works, but it only prints output when you press enter key. I had to press enter key 9 times just to get this output.

iamalexrouse · 2024-07-20T00:59:06Z

Hi,

Unfortunately this PR makes LA unusable in environments that support only stdin, stdout and stderr, just like Pterodactyl (which is the most popular way of launching SCP:SL servers). It doesn't support ReadKey. I just tested your PR there and it was not possible to run any command in Pterodactyl. Furthermore the output is completely broken there. I attached a few screenshots below.

Additionally I have noticed a significant negative performance impact.

I tested it on regular Ubuntu as well. It works, but it only prints output when you press enter key. I had to press enter key 9 times just to get this output.

lmao.

This is pretty much why these things need to be tested on both Windows & Linux before they're marked as ready for review.

zabszk · 2024-07-20T12:28:49Z

The primary issue isn't lack of testing (bugs can be fixed, of course before merging the PR), but the fact that some environments don't support ReadKey() and other used features.

iamalexrouse · 2024-07-20T15:15:35Z

The primary issue isn't lack of testing (bugs can be fixed, of course before merging the PR), but the fact that some environments don't support ReadKey() and other used features.

Is it possible to implement an os dependent ReadKey()?

zabszk · 2024-07-20T17:17:58Z

The primary issue isn't lack of testing (bugs can be fixed, of course before merging the PR), but the fact that some environments don't support ReadKey() and other used features.

Is it possible to implement an os dependent ReadKey()?

It's not an OS issue. For example pterodactyl doesn't support "all interactions". It only supports stdin and stdout (and likely stderr, I'm not sure). You can't press a key there. You can only type a line of text and send it as stdin. So it's not an OS limitation (linux supports it), but specific environment limitation.

Just like LA doesn't pass key presses to SCP:SL process itself. It only reads stdout and stderr. So if you for some reason call ReadKey() in SCP:SL itself (no LA), it will never be triggered, even if you press a key in LA.

AlexanderK666 · 2024-07-20T22:09:42Z

Hmm, I think we can add a check to see if the environment supports ReadKey. If not, we should use the regular ReadLine. Additionally, we should add an argument to the startup options to forcefully change the operating mode.

iamalexrouse · 2024-07-21T01:56:39Z

Hmm, I think we can add a check to see if the environment supports ReadKey. If not, we should use the regular ReadLine. Additionally, we should add an argument to the startup options to forcefully change the operating mode.

While you can add an argument, the change would have to be communicated with people using pterodactyl who decide to get the latest localadmin, so I'd prefer the auto detection, but how?

AlexanderK666 · 2024-07-21T07:55:19Z

Hmm, I think we can add a check to see if the environment supports ReadKey. If not, we should use the regular ReadLine. Additionally, we should add an argument to the startup options to forcefully change the operating mode.

While you can add an argument, the change would have to be communicated with people using pterodactyl who decide to get the latest localadmin, so I'd prefer the auto detection, but how?

Yes, I meant to implement automatic detection and, in addition to that, a startup argument.

AlexanderK666 · 2024-07-21T10:17:44Z

implement automatic detection

So, I think I found a way to implement this automatic detection.

Console.IsInputRedirected is True when running through Docker Compose; I think it will be the same in Pterodactyl. I temporarily commented it out to see if there is another way to implement this. And yes, the catch works successfully. If there is no exception (for example, on Windows), we need to remove the key press wait; for this, I used .Wait(50).

What do you think?

AlexanderK666 · 2024-07-21T10:37:01Z

I read the Dotnet Runtime code, and it seems that performing additional checks besides Console.IsInputRedirected is unnecessary.

zabszk · 2024-07-21T10:57:35Z

I didn't see an exceptions when running in pterodactyl, but the entire output was broken, so I can't tell for sure (and there could be some try/catch somewhere in the code).

Of course this will not catch all the edge cases, but the most important is to make sure it works in docker in pterodactyl.

We should decide whether this feature should be opt-in (disabled by default) or opt-out (enabled by default).

AlexanderK666 · 2024-07-21T11:05:17Z

I didn't see an exceptions when running in pterodactyl, but the entire output was broken, so I can't tell for sure (and there could be some try/catch somewhere in the code).

Of course this will not catch all the edge cases, but the most important is to make sure it works in docker in pterodactyl.

We should decide whether this feature should be opt-in (disabled by default) or opt-out (enabled by default).

I'm thinking of making this feature enabled by default in environments that support the use of Console.ReadKey, while adding the option to disable this via the configuration file or the startup argument. In environments that do not support the use of Console.ReadKey turn it off.

AlexanderK666 · 2024-07-21T11:07:31Z

I didn't see an exceptions when running in pterodactyl

I think this is because the input is happening in a thread, and there's no try-catch block there.

AlexanderK666 · 2024-07-21T11:13:45Z

I didn't see an exceptions when running in pterodactyl

I think this is because the input is happening in a thread, and there's no try-catch block there.

In Visual Studio, with debugger:

In runtime, without debugger:

zabszk · 2024-07-21T14:31:53Z

Is it possible for you to test it in linux and in pterodactyl? If not, I can test it after you commit fixes to this PR.

Don't forget about the issue that exists on linux (outside pterodactyl) that I mentioned (the one that requires to press enter key to get new output).

AlexanderK666 · 2024-07-21T14:35:46Z

Is it possible for you to test it in linux and in pterodactyl? If not, I can test it after you commit fixes to this PR.

Don't forget about the issue that exists on linux (outside pterodactyl) that I mentioned (the one that requires to press enter key to get new output).

Yes, I have the ability to test in Linux, but I haven't figured out how to work with Pterodactyl yet. I have Docker Compose and Portainer, which theoretically should work in a similar way. Additional testing would definitely be useful.

AlexanderK666 · 2024-07-21T18:15:40Z

Don't forget about the issue that exists on linux (outside pterodactyl) that I mentioned (the one that requires to press enter key to get new output).

It seems I managed to fix it. To solve this, I added a check for Console.KeyAvailable.

Currently partially tested on Windows, but some issues were found.

AlexanderK666 · 2024-07-21T19:36:18Z

So far, I found that the exit command on Windows does not close the window, but I haven't been able to figure out why yet.

AlexanderK666 · 2024-07-21T20:26:15Z

I did the build via GitHub Actions on my fork repository. P.S.: workflow dispatch is not necessary to contribute to the main repository, it was needed for the fork.

iamalexrouse · 2024-07-25T08:10:00Z

So far, I found that the exit command on Windows does not close the window, but I haven't been able to figure out why yet.

If you CTRL + C, does it quit the application?

AlexanderK666 · 2024-07-25T13:18:00Z

If you CTRL + C, does it quit the application?

This closes SCPSL.exe, but the LocalAdmin window in Windows has to be closed manually. On Linux, everything closes correctly.

iamalexrouse · 2024-07-25T15:42:10Z

If you CTRL + C, does it quit the application?

This closes SCPSL.exe, but the LocalAdmin window in Windows has to be closed manually. On Linux, everything closes correctly.

Console apps are designed by Microsoft to terminate if there is no valid code to process meaning somewhere in your changes, your preventing the console from reaching a termination state.

You’ll need to pause the application state after entering the state and discover what the JIT compiler is struggling to figure out.

added support for input transfer during logging

fdec9dc

Now when logging commands in the console, input transfer works, which improves user interaction with the server console, where logs are written in a thread and begin to interfere with viewing the current input.

Uploaded from the correct folder

eb15613

Looks like it's fixed

361a198

iamalexrouse reviewed Jul 17, 2024

View reviewed changes

improved readability

da53c86

New commands input has been reworked

a606ecf

Currently partially tested on Windows, but some issues were found.

Added workflow_dispatch event for CI.

1f6237e

Added support for input transfer during logging #92

Are you sure you want to change the base?

Added support for input transfer during logging #92

Conversation

AlexanderK666 commented Jul 17, 2024

AlexanderK666 commented Jul 17, 2024

AlexanderK666 commented Jul 17, 2024 • edited Loading

iamalexrouse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamalexrouse Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zabszk commented Jul 19, 2024

iamalexrouse commented Jul 20, 2024

zabszk commented Jul 20, 2024

iamalexrouse commented Jul 20, 2024

zabszk commented Jul 20, 2024

AlexanderK666 commented Jul 20, 2024 • edited Loading

iamalexrouse commented Jul 21, 2024

AlexanderK666 commented Jul 21, 2024

AlexanderK666 commented Jul 21, 2024

AlexanderK666 commented Jul 21, 2024 • edited Loading

zabszk commented Jul 21, 2024

AlexanderK666 commented Jul 21, 2024

AlexanderK666 commented Jul 21, 2024

AlexanderK666 commented Jul 21, 2024

zabszk commented Jul 21, 2024

AlexanderK666 commented Jul 21, 2024

AlexanderK666 commented Jul 21, 2024

AlexanderK666 commented Jul 21, 2024

AlexanderK666 commented Jul 21, 2024 • edited Loading

iamalexrouse commented Jul 25, 2024

AlexanderK666 commented Jul 25, 2024

iamalexrouse commented Jul 25, 2024

AlexanderK666 commented Jul 17, 2024 •

edited

Loading

iamalexrouse Jul 17, 2024 •

edited

Loading

AlexanderK666 commented Jul 20, 2024 •

edited

Loading

AlexanderK666 commented Jul 21, 2024 •

edited

Loading

AlexanderK666 commented Jul 21, 2024 •

edited

Loading