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

Change interactive window behavior when typing inside readonly area #5339

Merged
merged 10 commits into from
Sep 23, 2015

Conversation

genlu
Copy link
Member

@genlu genlu commented Sep 18, 2015

Fix issue #5130

Here's the new behavior for all deletion without selected text related operations:

  • if caret is in editable area, no change in behavior.
  • if caret is in active prompt area, move caret to the closest location first.
  • if caret is in other readonly area, do nothing.

Here's the new behavior for all deletion with selected text related operations:

  • if the selection overlaps with editable area, caret will be moved to a position in
    editable area that is closest to it's original position.
  • Otherwise, do nothing (and preserve the selection).

Deletion related operations including:

  • Delete (and TypeChar)
  • Backspace
  • Return and BreakLine
  • Paste
  • Cut

Question:
InsertCode method also depends on CutOrDeleteSelection method, but I'm not sure if this change will affect it, can I assume the caret will always in editable area when InsertCode is called?

Affected operations including:
- Delete (and TypeChar)
- Backspace
- Return
- Cut
- Paste
Here's the new behavior for all deletion without selected text related operations:
- if caret is in editable area, no change in behavior.
- if caret is in active prompt area, move caret to the closest location first.
- if caret is in other readonly area, do nothing.

Here's the new behavior for all deletion with selected text related operations:
- if the selection overlaps with editable area, caret will be moved to a position in
  editable area that is closest to it's original position.
- Otherwise, do nothing (and preserve the selection).

Deletion related operations including:
- Delete (and TypeChar)
- Backspace
- Return and BreakLine
- Paste
- Cut
@genlu
Copy link
Member Author

genlu commented Sep 18, 2015

@amcasey @tmat @cston @KevinH-MS @ManishJayaswal Please review.

This is not completed, I will add tests in later commits. Just wanna send out a PR so we can discuss if this new behavior makes sense.

By the way, here's another PR which fix this issue differently (move caret to closest editable location first if it's not originally) #5287

@genlu
Copy link
Member Author

genlu commented Sep 19, 2015

@tmat Have you reviewed the lastest commit as well? I have changed the behavior of CutOrDeleteSelection method,please let me know if it might break anything.

@tmat
Copy link
Member

tmat commented Sep 19, 2015

Looks reasonable. Would need to play with it to see if everything is actually good.

if (CutOrDeleteSelection(isCut: false))
{
MoveCaretToClosestEditableBuffer();
}
Copy link
Member

Choose a reason for hiding this comment

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

Else return false?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

var index = GetSourceSpanIndex(sourceSpans, point);
if (index == sourceSpans.Count)
{
index--;
Copy link
Member

Choose a reason for hiding this comment

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

return false;?

Copy link
Member Author

Choose a reason for hiding this comment

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

@amcasey Well, GetSourceSpanIndex returns the length of the collection if the point is at the end of the last span. Are you suggesting that the prompt will never be in the last span?

Copy link
Member

Choose a reason for hiding this comment

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

That's true in practice, but we probably don't want to depend on it. I'm very surprised to learn that GetSourceSpanIndex behaves that way, but it looks like all callers handle it.

@amcasey
Copy link
Member

amcasey commented Sep 22, 2015

👍

@tannergooding
Copy link
Member

test full please

genlu added a commit that referenced this pull request Sep 23, 2015
Change interactive window behavior when typing inside readonly area
@genlu genlu merged commit 50b4086 into dotnet:master Sep 23, 2015
@genlu
Copy link
Member Author

genlu commented Sep 23, 2015

FYI @ManishJayaswal @kuhlenh @tmat @amcasey @cston @KevinH-MS
If you found any of the new behaviors doesn't make sense, please let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants