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

Improve Console for iOS and Android #37078

Closed
wants to merge 7 commits into from

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented May 27, 2020

Fixes #36440 for both iOS and Android

We have to accumulate content written via Write and flush it only when \n is found.

Also, fix the iOS sample.

@ghost
Copy link

ghost commented May 27, 2020

Tagging subscribers to this area: @eiriktsarpalis
Notify danmosemsft if you want to be subscribed.

EgorBo and others added 2 commits May 27, 2020 22:41
Co-authored-by: campersau <buchholz.bastian@googlemail.com>
Co-authored-by: campersau <buchholz.bastian@googlemail.com>
Copy link
Member

@rolfbjarne rolfbjarne left a comment

Choose a reason for hiding this comment

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

There seems to be a significant amount of code duplication between iOS and Android, maybe there could be a way to share that code?

Also I think this implementation won't handle the following correctly:

Console.Write ("A\na");
Console.Write ("B\nb");
Console.WriteLine ("C");

I think it will show up as:

A
a
B
b
C

when it should be:

A
aB
bC

@EgorBo
Copy link
Member Author

EgorBo commented May 28, 2020

@rolfbjarne fixed to print they way you described

Console.Write ("A\na");
Console.Write ("B\nb");
Console.WriteLine ("C");

prints

A
aB
bC

{
// we found a new line, append content from accumulator if it's not empty
accumulator.Append(sliceWithNl);
printer(accumulator.ToString());
Copy link
Member

Choose a reason for hiding this comment

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

There are lots of strings created. Would it be possible to pass ReadOnlySpans around instead?

private static void AccumulateNewLines(StringBuilder accumulator, ReadOnlySpan<char> buffer, Action<string> printer)
{
int lineStartIndex = 0;
for (int i = 0; i < buffer.Length; i++)
Copy link
Member

Choose a reason for hiding this comment

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

This will always walk the entire buffer.

I think a more performant algorithm would be to walk the buffer backwards, and if you find a newline character, you can print everything up to that character (prepending anything in the buffer), and then put the rest in the buffer. This would be much faster for Console.WriteLine statements, since it would only check the last character in the buffer.

for (int i = buffer.Length - 1; i >= 0; i--) {
    if (buffer[i] == '\n') {
        if (accumulator.Length > 0) {
            printer (accumulator + buffer.Slice(0, i + 1));
            accumulator.Clear();
        } else {
            printer (buffer.Slice(0, i + 1));
        }
        if (i < buffer.Length - 1) {
            // there's text after the last newline, add it to the accumulator
            accumulator.Append(buffer.Slice(i + 1));
        }
        return;
    }
}
// no newlines found, add the entire buffer to the accumulator
accumulator.Append(buffer);

Debug.Assert(ConsolePal.OutputEncoding == Encoding.Unicode);
Debug.Assert(bufferSpan.Length % 2 == 0);

ReadOnlySpan<char> charSpan = MemoryMarshal.Cast<byte, char>(bufferSpan);
Copy link
Member

Choose a reason for hiding this comment

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

This won't necessarily be two-byte aligned. Is that ok?

@adamsitnik adamsitnik added this to the 5.0.0 milestone Jun 24, 2020
@adamsitnik
Copy link
Member

@EgorBo could you please respond to the most recent review comments and solve the merge conflict? Thanks!

@EgorBo
Copy link
Member Author

EgorBo commented Jun 24, 2020

@adamsitnik I'll, but the xamarin ios/android stuff was postponed to 6.0

@adamsitnik adamsitnik modified the milestones: 5.0.0, 6.0.0 Jun 26, 2020
@EgorBo
Copy link
Member Author

EgorBo commented Aug 6, 2020

Will re-open it once we pass 5.0.0 milestone to keep amount of active PRs smaller.

@EgorBo EgorBo closed this Aug 6, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Console.Write prints a new line on iOS
7 participants