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

Use the viewport-relative cursor pos for CCore.CursorPosition #13785

Merged
merged 1 commit into from
Aug 22, 2022

Conversation

DHowett
Copy link
Member

@DHowett DHowett commented Aug 19, 2022

In #13024, we removed Terminal::GetCursorPosition from TerminalCore.
This has been widely regarded as a good move.

Now, you might rightly be wondering: why didn't compilation immediately
fail? Well. It turns out that there were two copies of
GetCursorPosition. One for const Terminal, and one for Terminal.
This is important.

Terminal::GetCursorPosition() returned the cursor position relative to
the viewport. Terminal::GetCursorPosition() const, however, returns
the cursor position in absolute.

We removed the non-const one. Fortunately, thanks to the lookup rules
for const-qualified members, this didn't matter. Code that called
GetCursorPosition() still called GetCursorPosition(), and everything
was fine.

Except that part about the relative coordinates. That was not fine.

The TSF control is the only consumer of ControlCore.CursorPosition,
and that was the only consumer of relative-GetCursorPosition().

This commit restores equilibrium by introducing a new
GetViewportRelativeCursorPosition() member to Terminal and switching
over the only consumer of relative cursor position to use it.

Closes #13769.

In #13024, we removed `Terminal::GetCursorPosition` from TerminalCore.
This has been widely regarded as a good move.

Now, you might rightly be wondering: why didn't compilation immediately
fail? Well. It turns out that there were _two_ copies of
`GetCursorPosition`. One for `const Terminal`, and one for `Terminal`.
This is important.

`Terminal::GetCursorPosition()` returned the cursor position relative to
the viewport. `Terminal::GetCursorPosition() const`, however, returns
the cursor position in absolute.

We removed the non-`const` one. Fortunately, thanks to the lookup rules
for `const`-qualified members, this didn't matter. Code that called
`GetCursorPosition()` still called `GetCursorPosition()`, and everything
was fine.

Except that part about the relative coordinates. That was not fine.

The TSF control is the _only_ consumer of `ControlCore.CursorPosition`,
and that was the _only_ consumer of relative-`GetCursorPosition()`.

This commit restores equilibrium by introducing a new
`GetViewportRelativeCursorPosition()` member to `Terminal` and switching
over the only consumer of relative cursor position to use it.

Closes #13769.
@ghost ghost added Area-Input Related to input processing (key presses, mouse, etc.) Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. labels Aug 19, 2022
DHowett added a commit that referenced this pull request Aug 22, 2022
In #13024, we removed `Terminal::GetCursorPosition` from TerminalCore.
This has been widely regarded as a good move.

Now, you might rightly be wondering: why didn't compilation immediately
fail? Well. It turns out that there were _two_ copies of
`GetCursorPosition`. One for `const Terminal`, and one for `Terminal`.
This is important.

`Terminal::GetCursorPosition()` returned the cursor position relative to
the viewport. `Terminal::GetCursorPosition() const`, however, returns
the cursor position in absolute.

We removed the non-`const` one. Fortunately, thanks to the lookup rules
for `const`-qualified members, this didn't matter. Code that called
`GetCursorPosition()` still called `GetCursorPosition()`, and everything
was fine.

Except that part about the relative coordinates. That was not fine.

The TSF control is the _only_ consumer of `ControlCore.CursorPosition`,
and that was the _only_ consumer of relative-`GetCursorPosition()`.

This commit restores equilibrium by introducing a new
`GetViewportRelativeCursorPosition()` member to `Terminal` and switching
over the only consumer of relative cursor position to use it.

Closes #13769.
Backport of #13785.
@DHowett DHowett changed the title Use the viewport-relative cursor position for CCore.CursorPosition Use the viewport-relative cursor pos for CCore.CursorPosition Aug 22, 2022
@DHowett DHowett merged commit 2dedc9a into main Aug 22, 2022
DHowett added a commit that referenced this pull request Sep 6, 2022
In #13024, we removed `Terminal::GetCursorPosition` from TerminalCore.
This has been widely regarded as a good move.

Now, you might rightly be wondering: why didn't compilation immediately
fail? Well. It turns out that there were _two_ copies of
`GetCursorPosition`. One for `const Terminal`, and one for `Terminal`.
This is important.

`Terminal::GetCursorPosition()` returned the cursor position relative to
the viewport. `Terminal::GetCursorPosition() const`, however, returns
the cursor position in absolute.

We removed the non-`const` one. Fortunately, thanks to the lookup rules
for `const`-qualified members, this didn't matter. Code that called
`GetCursorPosition()` still called `GetCursorPosition()`, and everything
was fine.

Except that part about the relative coordinates. That was not fine.

The TSF control is the _only_ consumer of `ControlCore.CursorPosition`,
and that was the _only_ consumer of relative-`GetCursorPosition()`.

This commit restores equilibrium by introducing a new
`GetViewportRelativeCursorPosition()` member to `Terminal` and switching
over the only consumer of relative cursor position to use it.

Closes #13769.

(cherry picked from commit 2dedc9a)
Service-Card-Id: 85131461
Service-Version: 1.15
@ghost
Copy link

ghost commented Sep 13, 2022

🎉Windows Terminal v1.15.252 has been released which incorporates this pull request.:tada:

Handy links:

@ghost
Copy link

ghost commented Sep 13, 2022

🎉Windows Terminal Preview v1.16.252 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Input Related to input processing (key presses, mouse, etc.) Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggestions pop up of the Japanese MS IME got pushed down after multiple commands
3 participants