Skip to content

Conversation

@kumaran-14
Copy link
Contributor

Fixes #2658.

@kumaran-14
Copy link
Contributor Author

@Tyriar Some MoveToCell tests are failing. Should they be updated or, my implementation is wrong?

@Tyriar
Copy link
Member

Tyriar commented Jan 13, 2020

@kumaran-14 well this test was enforcing the old behavior which you've now broken:

 MoveToCell
       normal buffer
         should ignore the Y value

You'll need to update the test to respect the new behavior. Make sure there's a test for moving across a single line and multiple lines as well.

@Tyriar Tyriar added this to the 4.4.0 milestone Jan 13, 2020
@kumaran-14 kumaran-14 requested a review from Tyriar January 15, 2020 16:08
@kumaran-14
Copy link
Contributor Author

@Tyriar can you look into this?

@Tyriar
Copy link
Member

Tyriar commented Jan 31, 2020

@kumaran-14 I'll probably free up next week to power through xterm.js stuff and release 4.4

@kumaran-14
Copy link
Contributor Author

@Tyriar Sure. That sounds awesome. I'd like to finish this and contribute more.

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

It only seems to wrap around 1 row currently, so if the prompt spans multiple rows you need multiple clicks to get it where you want it.

@Tyriar
Copy link
Member

Tyriar commented Feb 2, 2020

Another thing I noticed is that the current algorithm doesn't look at whether the row is wrapped, it should only move along the current wrapped row. I'm looking into fixing these issues now so we can get this merged in.

@kumaran-14
Copy link
Contributor Author

@Tyriar How do I get the positions of buffer end?

localhost_3000_

@kumaran-14
Copy link
Contributor Author

CI fails on windows and mac, passes on Linux.
What system did you test on?
I get this behaviour on Linux
ezgif com-video-to-gif

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @kumaran-14, I made a couple of changes including adding some more complete tests so we can merge this in.

@Tyriar Tyriar merged commit 001d26d into xtermjs:master Feb 3, 2020
@kumaran-14
Copy link
Contributor Author

@Tyriar Sure. Glad to help.

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.

Take isWrapped cell status into account when alt+clicking to navigate the prompt in the normal buffer

2 participants