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

Retain wrapped line state when copying #693

Merged
merged 3 commits into from
Jun 13, 2017

Conversation

Tyriar
Copy link
Member

@Tyriar Tyriar commented Jun 10, 2017

Fixes #443

@Tyriar Tyriar added the work-in-progress Do not merge label Jun 10, 2017
@Tyriar Tyriar added this to the 2.8.0 milestone Jun 10, 2017
@Tyriar Tyriar self-assigned this Jun 10, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 69.518% when pulling 346b517 on Tyriar:443_retain_wrapped_lines_copy into a889fef on sourcelair:master.

@Tyriar Tyriar requested a review from parisk June 10, 2017 00:23
@Tyriar Tyriar removed the work-in-progress Do not merge label Jun 10, 2017
@Tyriar
Copy link
Member Author

Tyriar commented Jun 10, 2017

I created #695 to follow up the isWrapped property on an array situation. Should be fine for the time being, it's mainly just an issue when we move the buffer to TS.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 69.518% when pulling 0a99f65 on Tyriar:443_retain_wrapped_lines_copy into c33814a on sourcelair:master.

Copy link
Contributor

@parisk parisk left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 69.504% when pulling 405d1ee on Tyriar:443_retain_wrapped_lines_copy into 5f9ad5c on sourcelair:master.

@Tyriar Tyriar merged commit 55f51ad into xtermjs:master Jun 13, 2017
@Tyriar Tyriar deleted the 443_retain_wrapped_lines_copy branch June 13, 2017 21:40
@cancan101
Copy link

Any chance of getting a minor release with this change?

@Tyriar
Copy link
Member Author

Tyriar commented Jun 22, 2017

Ping @parisk

@parisk
Copy link
Contributor

parisk commented Jun 26, 2017

Since this is the last week of June, let's hold on until next Monday. I can make an early release for 2.8 on Monday for sure.

@marcosnils
Copy link

@Tyriar @parisk I've updated PWD (http://play-with-docker.com) with this release but it seems that for some reason I'm still getting \n when copying and pasting long lines. Is there something I'm missing?

@Tyriar
Copy link
Member Author

Tyriar commented Jul 7, 2017

@marcosnils what command are you running? The program may be formatting based on the width of the terminal? It's also worth verifying whether copying works fine on Terminal.app/gnome-terminal/etc.

A new issue with more details would be great 😃

@marcosnils
Copy link

@Tyriar if you go to PWD, type echo asdsadasdasdasdasdasasdsadasdasdasdasdasasdsadasdasdasdasdasasdsadasdasdasdasdasasdsadasdasdasdasdasasdsadasdasdasdasdasasdsadasdasdasdasdasasdsadasdasdasdasdasasdsadasdasdasdasdasasdsadasdasdasdasdasasdsadasdasdasdasdasasdsadasdasdasdasdasasdsadasdasdasdasdasasdsadasdasdasdasdasasdsadasdasdasdasdasasdsadasdasdasdasdasasdsadasdasdasdasdasasdsadasdasdasdasdasasdsadasdasdasdasdasasdsadasdasdasdasdasasdsadasdasdasdasdas and copy / paste that results you'll see that xterm adds newline when it wraps the text. If you try the same thing in gnome-terminal this doesn't happen.

@Tyriar
Copy link
Member Author

Tyriar commented Jul 7, 2017

@marcosnils hmm, same thing works fine in VS Code using the new changes. Debugging PWD, it looks like bufferLine.isWrapped is not set for the wrapped line in SelectionManager.selectionText.

@Tyriar
Copy link
Member Author

Tyriar commented Jul 7, 2017

I can reproduce the issue on master, however it doesn't occur in my vscode-release/1.14 branch which explains why it's working in VS Code.

@Tyriar
Copy link
Member Author

Tyriar commented Jul 7, 2017

Ok I can't seem to reproduce in master anymore 😕

@Tyriar
Copy link
Member Author

Tyriar commented Jul 7, 2017

Repro steps are flaky, copying from PWD just worked fine:

screen shot 2017-07-07 at 11 03 14 am

@Tyriar
Copy link
Member Author

Tyriar commented Jul 7, 2017

I know what's happening... Wrapped line status isn't being recorded correctly for the initial viewport that the terminal was initialized with (the first 24 rows). Get beyond this point and it works fine.

@Tyriar
Copy link
Member Author

Tyriar commented Jul 7, 2017

@marcosnils it would be handy to be able to add sessions without needing to resize the window 😄

screen shot 2017-07-07 at 11 06 10 am

@Tyriar
Copy link
Member Author

Tyriar commented Jul 7, 2017

@marcosnils also selection seems not to be aligned with the cursor in your app, I'm guessing this is related to your CSS or something as it works fine in the demo and VS Code.

@Tyriar
Copy link
Member Author

Tyriar commented Jul 7, 2017

Moved to issue #767, PR out in #768

@marcosnils
Copy link

@marcosnils also selection seems not to be aligned with the cursor in your app, I'm guessing this is related to your CSS or something as it works fine in the demo and VS Code

thx for reporting this and providing a fix so quickly. Do you think it'll take long to be merged?. So I don't have to merge manually :)

@Tyriar
Copy link
Member Author

Tyriar commented Jul 7, 2017

@marcosnils thanks for reporting so quickly 😄

@parisk normally does the publishing to npm, unless other issues come in I'd say we should publish this with 2.8.1 as soon as he's free. Might need to wait until Sunday night/Monday due to it being the weekend over there.

@marcosnils
Copy link

@Tyriar thx. I was referring to merge to master. I can build from there and deploy manually. PWD is not using npm actually 😨

@Tyriar
Copy link
Member Author

Tyriar commented Jul 7, 2017

@marcosnils you could probably point at https://github.com/Tyriar/xterm.js/tree/767_initial_viewport_wrap in the meantime then. If you're using a custom npm dep this is how you point at a branch: https://github.com/Microsoft/vscode/blob/b32d2d44902eafbbdc252f0260cdbd938a5686f8/package.json#L46

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.

5 participants