Skip to content

Console.Unix: support backspace during Console.Read - #49649

Merged
adamsitnik merged 3 commits into
dotnet:mainfrom
tmds:console_read_backspace
Jul 29, 2021
Merged

Console.Unix: support backspace during Console.Read#49649
adamsitnik merged 3 commits into
dotnet:mainfrom
tmds:console_read_backspace

Conversation

@tmds

@tmds tmds commented Mar 15, 2021

Copy link
Copy Markdown
Member

Fixes #49129 (comment).

In addition to the fix, I've refactored the code a bit and added some Asserts to help make it more clear.

@stephentoub @eiriktsarpalis ptal

cc @webczat

@ghost ghost added the area-System.Console label Mar 15, 2021
tmds
tmds commented Mar 15, 2021
View reviewed changes
Comment thread src/libraries/System.Console/tests/ManualTests/ManualTests.cs
tmds
tmds commented Mar 15, 2021
View reviewed changes
Comment thread src/libraries/System.Console/src/System/IO/StdInReader.cs
{
_readLineSB.Length = len - 1;
if (!previouslyProcessed)
removed = _tmpKeys.TryPop(out _);

@tmds tmds Mar 15, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This TryPop is the actual bugfix.

@runfoapp runfoapp Bot mentioned this pull request Mar 15, 2021
Open
@tmds

tmds commented Mar 26, 2021

Copy link
Copy Markdown
Member Author

@adamsitnik @jozkee and @carlossanlop ptal

@tmds

tmds commented Apr 29, 2021

Copy link
Copy Markdown
Member Author

@adamsitnik @jozkee and @carlossanlop ptal

@adamsitnik @jozkee and @carlossanlop ping

@jeffhandley

jeffhandley commented May 21, 2021

Copy link
Copy Markdown
Member

I'm sorry for the delay getting this reviewed, @tmds! With the FileStream effort winding down, I expect @adamsitnik, @carlossanlop, and @jozkee will be able to take a look very soon.

@jeffhandley
jeffhandley requested a review from adamsitnik May 21, 2021 22:02
@jeffhandley

jeffhandley commented Jun 11, 2021

Copy link
Copy Markdown
Member

Tagging @dotnet/area-system-io. It would be great to get this reviewed before the Preview 6 snap so that we can include this fix in that release.

@tmds

tmds commented Jul 16, 2021

Copy link
Copy Markdown
Member Author

@dotnet/area-system-io ptal

This fixes an issue reported by a user in #49129 (comment).

@terrajobst terrajobst added the community-contribution Indicates that the PR has been added by a community member label Jul 19, 2021
@jeffhandley jeffhandley added the Priority:1 Work that is critical for the release, but we could probably ship without label Jul 23, 2021
@jeffhandley

jeffhandley commented Jul 23, 2021

Copy link
Copy Markdown
Member

@adamsitnik This PR is assigned to you for follow-up/decision before the RC1 snap, with a high priority.

@adamsitnik adamsitnik added this to the 6.0.0 milestone Jul 28, 2021
adamsitnik
adamsitnik approved these changes Jul 29, 2021
View reviewed changes

@adamsitnik adamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I've tested it on both Windows 10 and Ubuntu 18.04 and all Manual tests are passing. @tmds big thanks for the fix and appologies for such a delay in the review!

Comment thread src/libraries/System.Console/tests/ManualTests/ManualTests.cs Outdated
Comment thread src/libraries/System.Console/tests/ManualTests/ManualTests.cs Outdated
Comment thread src/libraries/System.Console/tests/ManualTests/ManualTests.cs Outdated
@adamsitnik
adamsitnik merged commit 48b3d37 into dotnet:main Jul 29, 2021
@carlossanlop carlossanlop mentioned this pull request Aug 13, 2021
Open
@ghost ghost locked as resolved and limited conversation to collaborators Aug 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@eiriktsarpalis eiriktsarpalis eiriktsarpalis left review comments
@adamsitnik adamsitnik adamsitnik approved these changes

Assignees

@adamsitnik adamsitnik

Labels

area-System.Console community-contribution Indicates that the PR has been added by a community member Priority:1 Work that is critical for the release, but we could probably ship without

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

System.Console.Read() needlessly changes terminal settings on linux

5 participants

@tmds @jeffhandley @eiriktsarpalis @adamsitnik @terrajobst