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

Fix prompt on windows #2986

Merged
merged 2 commits into from
Jun 5, 2023
Merged

Conversation

ShaneEverittM
Copy link
Contributor

It changes the following:

  • Moves prompt highlighting to the helper class
  • Fixes prompt width calculation on windows

Details

In rustyline, there are two implementations of calculate_position, one for windows and one for unix. The one for unix support ignoring ANSI escape sequences when calculating width, but the one on windows does not, by design.

This caused an issue where the cursor would be placed 12 columns in, instead of 3.

Before

before

After

after

@codecov
Copy link

codecov bot commented Jun 1, 2023

Codecov Report

Merging #2986 (30e9fc2) into main (14176a2) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #2986      +/-   ##
==========================================
- Coverage   50.20%   50.20%   -0.01%     
==========================================
  Files         446      446              
  Lines       45988    45993       +5     
==========================================
  Hits        23089    23089              
- Misses      22899    22904       +5     
Impacted Files Coverage Δ
boa_cli/src/helper.rs 0.00% <0.00%> (ø)
boa_cli/src/main.rs 0.60% <0.00%> (ø)
boa_engine/src/optimizer/mod.rs 61.70% <ø> (ø)

By moving the prompt coloring to be done in
Highlighter::highlight_prompt, we sidestep the issue on Windows where
the prompt width is calculated post-coloring AND without ignoring escape
codes.

By including it in the implementation of Highlighter, Editor::readline
now operates on a plain-text prompt, so width calculation is correct.

This commit also re-arranges the trait impl order to match the
definition.
@ShaneEverittM ShaneEverittM force-pushed the fix-prompt-on-windows branch from 2e2ce7b to 30e9fc2 Compare June 1, 2023 05:20
@jedel1043 jedel1043 added bug Something isn't working cli Issues and PRs related to the Boa command line interface. windows Issues and PRs related to the Windows platform. labels Jun 1, 2023
@jedel1043 jedel1043 requested a review from a team June 1, 2023 07:05
Copy link
Member

@HalidOdat HalidOdat left a comment

Choose a reason for hiding this comment

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

Nice work :)

@Razican Razican added this pull request to the merge queue Jun 5, 2023
Merged via the queue into boa-dev:main with commit eeeaaee Jun 5, 2023
Razican pushed a commit that referenced this pull request Jun 26, 2023
* Fix typo

* Refactor prompt highlighting into RLHelper

By moving the prompt coloring to be done in
Highlighter::highlight_prompt, we sidestep the issue on Windows where
the prompt width is calculated post-coloring AND without ignoring escape
codes.

By including it in the implementation of Highlighter, Editor::readline
now operates on a plain-text prompt, so width calculation is correct.

This commit also re-arranges the trait impl order to match the
definition.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cli Issues and PRs related to the Boa command line interface. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants