Skip to content

Remove LineIsMultiline in favor of multiline agnostic algorithms (#1125) - #2047

Merged
Dongbo Wang (daxian-dbw) merged 4 commits into
PowerShell:masterfrom
springcomp:#1125
Feb 11, 2021
Merged

Dongbo Wang (daxian-dbw) merged 4 commits into
PowerShell:masterfrom
springcomp:#1125

Conversation

@springcomp

@springcomp Maxime Labelle (springcomp) commented Dec 17, 2020 •

Copy link
Copy Markdown
Contributor

PR Summary

Fixes #1125.

This PR removes as many calls as possible of the method LineIsMultiline() where the underlying algorithm is already behaving consistently irrespective of the number of logical lines in the buffer.

PR Checklist

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • Make sure you've added one or more new tests - All non-regression changes covered by existing unit-tests.
  • Make sure you've tested these changes in terminals that PowerShell is commonly used in (i.e. conhost.exe, Windows Terminal, Visual Studio Code Integrated Terminal, etc.)
  • User-facing changes
    • Not Applicable
Microsoft Reviewers: Open in CodeFlow

@daxian-dbw Dongbo Wang (daxian-dbw) 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.

Thanks for taking the time to elide the unneeded LineIsMultiLine() check. I left a few comments.

Comment thread PSReadLine/BasicEditing.cs Resolved
Comment thread PSReadLine/Movement.vi.cs Resolved
Comment thread PSReadLine/Movement.vi.cs Resolved
Comment thread PSReadLine/Movement.vi.multiline.cs Resolved
@daxian-dbw

Dongbo Wang (daxian-dbw) commented Feb 10, 2021 •

Copy link
Copy Markdown
Member

Maxime Labelle (@springcomp) I apologize for taking so long to get back to this.
I rebased your branch against master, and left 2 comments above. If you agree with my comments, I can help make the according changes.

@springcomp

Copy link
Copy Markdown
Contributor Author

No worries. I will address the changes.
Thank you for your help.

@ghost ghost removed the Needs-Author Feedback label Feb 10, 2021

@daxian-dbw Dongbo Wang (daxian-dbw) 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.

@daxian-dbw
Dongbo Wang (daxian-dbw) merged commit 0de6368 into PowerShell:master Feb 11, 2021
@ghost

Copy link
Copy Markdown

🎉 v2.2.0-beta2 has been released which incorporates this pull request. 🎉

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Minor code efficiency issue, use of LineIsMultiLine()

2 participants