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 for issue #11205 :terminal #11348

Closed
wants to merge 9 commits into from
Closed

Fix for issue #11205 :terminal #11348

wants to merge 9 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 7, 2019

Changing the line termination from \n to \r. Fixing an issue, where the :terminal powershell would have its paste in reverse order.
Checked on windows as well as on mac with powershell core.

Fixes #11205.

@bfredl
Copy link
Member

bfredl commented Nov 7, 2019

Makes sense. Could you update the failing tests? (and check that they will fail for the old behavior, maybe that is enough for tests then).

@justinmk justinmk added this to the 0.4.4 milestone Nov 8, 2019
@ghost
Copy link
Author

ghost commented Nov 8, 2019

I'll do so as soon as I get back to my windows machine.

Nick Anthony added 2 commits December 13, 2019 14:30
…ue. Pasting now works on windows with \r and on unix with \n
@justinmk
Copy link
Member

@Liberatys what changed in the latest push? Do you plan to update the tests any time soon?

@justinmk justinmk added the needs:response waiting for reply from the author label Dec 17, 2019
@ghost
Copy link
Author

ghost commented Dec 17, 2019

@justinmk I have to apologize. I had a lot going on and always pushed it. After multiple times of going through the tests and trying to figure out what to change .. I have to admit that I don't seem to get how to test for the new behaviour.
I'm sorry.

Could someone with more knowledge finish this fix? Thank you very much.

@blueyed
Copy link
Contributor

blueyed commented Dec 21, 2019

Pushed a lint fix.
It does not look like there are failing tests (the ones on AppVeyor looked unrelated, but maybe I misunderstood and they can be used for testing this change).

@blueyed
Copy link
Contributor

blueyed commented Dec 21, 2019

AppVeyor failures are relevant:

[  FAILED  ] 2 tests, listed below:
[  FAILED  ] test/functional\terminal\buffer_spec.lua @ 86: :terminal buffer sends data to the terminal when the "put" operator is used
test/functional\terminal\buffer_spec.lua:90: Row 2 did not match.
Expected:
  |^tty ready                                         |
  |*appended tty ready                                |
  |*appended tty ready                                |
  |*{2: }                                                 |
  |                                                  |
  |                                                  |
  |:let @a = "appended " . @a                        |
Actual:
  |^tty ready                                         |
  |*{2:a}ppended tty ready                                |
  |*                                                  |
  |*                                                  |
  |                                                  |
  |                                                  |
  |:let @a = "appended " . @a                        |
To print the expect() call that would assert the current screen state, use
screen:snapshot_util(). In case of non-deterministic failures, use
screen:redraw_debug() to show all intermediate screen states.  
stack traceback:
	test\functional\ui\screen.lua:579: in function '_wait'
	test\functional\ui\screen.lua:361: in function 'expect'
	test/functional\terminal\buffer_spec.lua:90: in function <test/functional\terminal\buffer_spec.lua:86>
[  FAILED  ] test/functional\terminal\buffer_spec.lua @ 112: :terminal buffer sends data to the terminal when the ":put" command is used
test/functional\terminal\buffer_spec.lua:116: Row 2 did not match.
Expected:
  |^tty ready                                         |
  |*appended tty ready                                |
  |*{2: }                                                 |
  |                                                  |
  |                                                  |
  |                                                  |
  |:put a                                            |
Actual:
  |^tty ready                                         |
  |*{2:a}ppended tty ready                                |
  |*                                                  |
  |                                                  |
  |                                                  |
  |                                                  |
  |:put a                                            |
To print the expect() call that would assert the current screen state, use
screen:snapshot_util(). In case of non-deterministic failures, use
screen:redraw_debug() to show all intermediate screen states.  
stack traceback:
	test\functional\ui\screen.lua:579: in function '_wait'
	test\functional\ui\screen.lua:361: in function 'expect'
	test/functional\terminal\buffer_spec.lua:116: in function <test/functional\terminal\buffer_spec.lua:112>

Code for the first one:

it('sends data to the terminal when the "put" operator is used', function()
feed('<c-\\><c-n>gg"ayj')
feed_command('let @a = "appended " . @a')
feed('"ap"ap')
screen:expect([[
^tty ready |
appended tty ready |
appended tty ready |
{2: } |
|
|
:let @a = "appended " . @a |
]])

Should "\r" only be used for Powershell then maybe?

@justinmk
Copy link
Member

Would be great to finish this...

# ifdef UNIX
terminal_send(curbuf->terminal, "\n", 1);
# else
terminal_send(curbuf->terminal, "\r", 1);
Copy link

@NilsIrl NilsIrl Feb 6, 2020

Choose a reason for hiding this comment

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

Suggested change
terminal_send(curbuf->terminal, "\r", 1);
terminal_send(curbuf->terminal, "\r\n", 2);

This might do it? (untested)

@@ -526,7 +526,11 @@ void terminal_paste(long count, char_u **y_array, size_t y_size)
for (size_t j = 0; j < y_size; j++) {
if (j) {
// terminate the previous line
terminal_send(curbuf->terminal, "\n", 1);
# ifdef UNIX
Copy link

@NilsIrl NilsIrl Feb 6, 2020

Choose a reason for hiding this comment

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

I'm not sure what are the different possible platforms are. But would it be possible to have a non-unix platform that doesn't use \r\n?

If this is the case, it would mean they would fall under the #else and receive the wrong characters.

@ghost ghost closed this Jun 18, 2020
@NilsIrl
Copy link

NilsIrl commented Jun 18, 2020

Sorry, I haven't really been following. Why close this?

@ghost ghost reopened this Jun 18, 2020
@ghost
Copy link
Author

ghost commented Jun 18, 2020

@NilsIrl Oh wow .. I'm sorry ^^ I was rechecking what you wrote before and wanted write a comment... must have slipped .. sry. Currently checking your reviews on my machines.

@ghost
Copy link
Author

ghost commented Jun 20, 2020

The conditional sending of \n and \r\n as proposed by @NilsIrl to the terminal seem to pass on the appveyor test and also worked on my local machine.

The failure in builds.sr.ht: openbsd.yml is unrelated to the change.

The failures in continuous-integration/travis-ci/pr seem to be related to the change.

s390x - big-endian

[ FAILED ] 1 test, listed below:
[ FAILED ] test/functional/terminal/scrollback_spec.lua @ 481: 'scrollback' option :setlocal in a :terminal buffer
test/helpers.lua:73: bad argument #2 to 'format' (string expected, got table)

AMD64 - gcc-32bit

Failed: 0 Tests
reset: standard error: Inappropriate ioctl for device
++fail oldtests F 'Legacy tests failed'

Which both make no sense to me, because Linux xenial shouldn't be influenced by the change.

@jamessan jamessan modified the milestones: 0.4.4, 0.4.5 Aug 5, 2020
@github-actions github-actions bot added the terminal built-in :terminal or :shell label Jun 10, 2021
@justinmk justinmk modified the milestones: 0.4.5, 0.6 Jul 2, 2021
@bfredl bfredl modified the milestones: 0.6, 0.7 Nov 27, 2021
dundargoc pushed a commit to dundargoc/neovim that referenced this pull request May 23, 2022
dundargoc pushed a commit to dundargoc/neovim that referenced this pull request May 23, 2022
@dundargoc
Copy link
Member

Superseded by #18723

@dundargoc dundargoc closed this May 23, 2022
@zeertzjq zeertzjq modified the milestones: 0.9, 0.8 Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:response waiting for reply from the author terminal built-in :terminal or :shell
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pasting register in powershell terminal reverses lines
7 participants