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

Allow multiple spinners #13

Merged
merged 6 commits into from
Dec 4, 2021
Merged

Allow multiple spinners #13

merged 6 commits into from
Dec 4, 2021

Conversation

neuecc
Copy link
Contributor

@neuecc neuecc commented Dec 3, 2021

Currently Spinner can not run multiple in multi-threading.
broken

This PR stores CursorTop and lock Console operations.

fixed

Also I've found bug of !CanAcceptEscapeSequence's clear line code.

broken_clear

_lineLength = frame.Length + 1 + Text.Length; is not work for non-ascii character.
I've modified to _lineLength = Console.CursorLeft; after console out flushed.

@neuecc
Copy link
Contributor Author

neuecc commented Dec 3, 2021

Sorry, after first open PR, I've changed implementation.
to allow other console writing, always render terminator and add Start(string terminator) overload.

other1

Also added as a hack, using Console.s_syncObject for lock if can.

@mayuki
Copy link
Owner

mayuki commented Dec 4, 2021

Great! But unfortunately, it seems that the output is corrupted when running on CI or enabled = false state.

Expected:
image
Actual (Text will appear in unexpected places):
image

And, in the case of Linux terminal, it will not display correctly at the window buffer boundary.

2021-12-04.10-47-48.mp4

The window buffer (Console.BufferHeight) other boundary problem seems to exist on Windows as well.
For example, If you run Spinner after for (var i = 0; i < 10000; i++) { Console.WriteLine(i); }, an exception is thrown.

image

@neuecc
Copy link
Contributor Author

neuecc commented Dec 4, 2021

@mayuki
thanks, I've fixed both and checked Windows/Linux, CI=1/none enviroments.

If reached window boundary, I've added check of position and subtract new line.

_cursorTop = Console.CursorTop;
Console.Write(terminator);
Console.Out.Flush();

// reaches window boundary
if (_cursorTop == Console.CursorTop)
{
    _cursorTop = Math.Max(_cursorTop - 1, 0);
    foreach (var item in s_runningSpinners)
    {
        item._cursorTop = Math.Max(item._cursorTop - 1, 0);
    }
}

-1 doesn't work when the usual Console.WriteLine is sandwiched in between, but it's still better than the previous version (which completely broke the behavior).

@mayuki
Copy link
Owner

mayuki commented Dec 4, 2021

-1 doesn't work when the usual Console.WriteLine is sandwiched in between, but it's still better than the previous version (which completely broke the behavior).

I would like to consider addressing that case in the future.

@mayuki mayuki merged commit 1616c7f into mayuki:master Dec 4, 2021
@neuecc neuecc deleted the multi-threading branch December 4, 2021 09:24
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