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

Unused/obsolete code in XtermEngine #17280

Closed
j4james opened this issue May 16, 2024 · 1 comment · Fixed by #17287
Closed

Unused/obsolete code in XtermEngine #17280

j4james opened this issue May 16, 2024 · 1 comment · Fixed by #17287
Labels
Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting

Comments

@j4james
Copy link
Collaborator

j4james commented May 16, 2024

In the XtermEngine::StartPaint method, there's some code that calculates the dirty area here:

{
std::span<const til::rect> dirty;
RETURN_IF_FAILED(GetDirtyArea(dirty));
// If we have 0 or 1 dirty pieces in the area, set as appropriate.
auto dirtyView = dirty.empty() ? Viewport::Empty() : Viewport::FromExclusive(til::at(dirty, 0));
// If there's more than 1, union them all up with the 1 we already have.
for (size_t i = 1; i < dirty.size(); ++i)
{
dirtyView = Viewport::Union(dirtyView, Viewport::FromExclusive(til::at(dirty, i)));
}
}

However, the result of that calculation is not actually used. There was a time when there was more code in that branch that compared the dirty area against the last viewport, but that was removed in PR #4741.


And in the XtermEngine::_MoveCursor method there's a local variable performedSoftWrap which is initially set to false at the start of the method here:

auto performedSoftWrap = false;

And it's potentially changed to true later in the code:

if (previousLineWrapped)
{
performedSoftWrap = true;
_trace.TraceWrapped();
hr = S_OK;
}

However, the variable is otherwise never read. It looks like it was added as part of PR #5181, but even then it doesn't appear to have been used.

Unless there is something I'm missing, I'd suggest we should remove the unused code.

@j4james j4james added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label May 16, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels May 16, 2024
@DHowett
Copy link
Member

DHowett commented May 16, 2024

I'm alright removing it!

github-merge-queue bot pushed a commit that referenced this issue May 20, 2024
## Summary of the Pull Request

The dirty view calculation in the `XtermEngine::StartPaint` method was
originally used to detect a full frame paint that would require a clear
screen, but that code was removed as part of PR #4741, making this
calculation obsolete.

The `performedSoftWrap` variable in the `XtermEngine::_MoveCursor`
method was assumedly a remanent of some WIP code that was mistakenly
committed in PR #5181. The variable was never actually used.

## Validation Steps Performed

All the unit tests still pass and nothing seems obviously broken in
manual testing.

## PR Checklist
- [x] Closes #17280
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants