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

[1.21] Selecting commands, output with the context menu is totally in the wrong place #17131

Closed
zadjii-msft opened this issue Apr 25, 2024 · 2 comments · Fixed by #17138
Closed
Assignees
Labels
In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements

Comments

@zadjii-msft
Copy link
Member

open pwsh, with shell integration enabled

do a

cd Z:\dev\public\OpenConsole\
git status
dir
foo
whatever

try and right-click select the output of dir, and it's... wrong:

image

surely, I'm the murderer, but I could have swore there were tests for this

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Apr 25, 2024
@zadjii-msft
Copy link
Member Author

https://github.com/microsoft/terminal/blame/f36d589a8eb60fe9ff4c8efaad8eac088647e8a8/src/cascadia/TerminalCore/terminalrenderdata.cpp#L205-L214

or prior:

https://github.com/microsoft/terminal/blame/d632c39cc3cef3075f09d881e03801518310d641/src/cascadia/TerminalCore/terminalrenderdata.cpp#L171-L207

Just staring at that in isolation, it's not clear to me what coordinate system the coord params of

SelectNewRegion(const til::point coordStart, const til::point coordEnd)

are

@zadjii-msft
Copy link
Member Author

diff --git a/src/cascadia/TerminalCore/terminalrenderdata.cpp b/src/cascadia/TerminalCore/terminalrenderdata.cpp
index ec28b59d5..3d989f31f 100644
--- a/src/cascadia/TerminalCore/terminalrenderdata.cpp
+++ b/src/cascadia/TerminalCore/terminalrenderdata.cpp
@@ -199,7 +199,7 @@ til::CoordType Terminal::_ScrollToPoints(const til::point coordStart, const til:
         _NotifyScrollEvent();
     }

-    return _scrollOffset;
+    return _VisibleStartIndex();
 }

 void Terminal::SelectNewRegion(const til::point coordStart, const til::point coordEnd)

this fixes it. Need to write a test

@carlos-zamora carlos-zamora added this to the Terminal v1.21 milestone Apr 25, 2024
@carlos-zamora carlos-zamora added the Issue-Bug It either shouldn't be doing this or needs an investigation. label Apr 25, 2024
@carlos-zamora carlos-zamora removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Apr 25, 2024
zadjii-msft added a commit that referenced this issue Apr 26, 2024
I think this subtly regressed in

`Terminal::SelectNewRegion` is the only thing that uses the return value from
`Terminal::_ScrollToPoints`. Before that PR, `_ScrollToPoints` was just a part
of `SelectNewRegion`, and it moved the start & end coords by the
`_VisibleStartIndex`, not the `_scrollOffset`.

Kinda weird there weren't any _other_ tests for `SelectNewRegion`?

I also caught a second bug while I was here - If you had a line with an exact
wrap, and tried to select that like with selectOutput, we'd explode.

Closes #17131
@zadjii-msft zadjii-msft added the In-PR This issue has a related PR label Apr 29, 2024
github-merge-queue bot pushed a commit that referenced this issue May 1, 2024
I think this subtly regressed in #16611. Jump to
90b8bb7#diff-f9112caf8cb75e7a48a7b84987724d754181227385fbfcc2cc09a879b1f97c12L171-L223

`Terminal::SelectNewRegion` is the only thing that uses the return value
from `Terminal::_ScrollToPoints`. Before that PR, `_ScrollToPoints` was
just a part of `SelectNewRegion`, and it moved the start & end coords by
the `_VisibleStartIndex`, not the `_scrollOffset`.

Kinda weird there weren't any _other_ tests for `SelectNewRegion`?

I also caught a second bug while I was here - If you had a line with an
exact wrap, and tried to select that like with selectOutput, we'd
explode.

Closes #17131
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants