inccommand: fix for wrapping of large string in preview mode #8283

nimitbhardwaj · 2018-04-15T18:26:29Z

When entering a large string in substitute command when set inccommand=nosplit is enabled, the string wraps around the single line. This was due to the condition that in Preview mode, we need a kind of behaviour where the DOCMD_NOWAIT is not completely necessary because there is change in the size of the cmdlilne, but we don't need to wait for the return, so ++no_wait_return was not necessary.

nimitbhardwaj · 2018-04-15T18:27:43Z

src/nvim/ex_docmd.c

@@ -577,6 +577,14 @@ int do_cmdline(char_u *cmdline, LineGetter fgetline,
        ++RedrawingDisabled;
        did_inc = TRUE;
      }
+      if (State & CMDPREVIEW) {


I could also add the condition in the above if stmt to check for CMDPREVIEW, and remove the DOCMD_NOWAIT from the part where this function will be called, for the case of previewable

If there is need to shorten the code that would be good

changing the above would be better. why did you removed ++no_wait_return ?

I did this because, I thought that for the completion of the preview command, there is finally need of a return, as told on Gitter, it NOWAIT was for hiding the Press Enter... error message.
Mostly this was I think work was of hit and trial, how I reached this, I had the idea that the code that will execute the Inccommand, so I tried to remove the DOCMD_NOWAIT it worked a little bit, but just give the notice Press Return or another key ..., so to remove this msg, I found the code what extra will happen if DOCMD_NOWAIT flag is given, I found out, and tried to hack code there, as described above using some common sense of the inccommands and a little bit about the functions which will be triggered by reading the comments, I had a lucky guess.

nimitbhardwaj · 2018-04-17T10:43:22Z

What about the tests of this PR, means it was a bug fix, how should i add tests.

bfredl · 2018-04-17T10:50:09Z

src/nvim/ex_getln.c

@@ -1911,7 +1911,7 @@ static int command_line_changed(CommandLineState *s)
    //       - Immediately undo the effects.
    State |= CMDPREVIEW;
    emsg_silent++;  // Block error reporting as the command may be incomplete
-    do_cmdline(ccline.cmdbuff, NULL, NULL, DOCMD_KEEPLINE|DOCMD_NOWAIT);


Can't we keep this and instead add State & CMDPREVIEW as an alternative in the enclosing if-statement?

I try to think of it as I reach room, now in class

By keeping DOCMD_NOWAIT, it means that the if part in ex_docmd.c will not work, I want that the if part should work, but only no_wait_return should not be incremented, if I have to edit it, the condition in if will become so messy, and according to my personal opinion, I don't like to read a messy, complicated and big condition, so I put this like the way.

Other alternative which I think that was good, was making a separate if condition for State & CMDPREVIEW but according to @justinmk its also not too better, so this option was left, which I thought was acceptable.
What are the views on it?

But this is a no-wait state, just like async and <Cmd> commands in cmdline mode. Just having this flag silently missing when it is expected is also messy.

Okk, then its an issue, then making the condition edit is better.

bfredl · 2018-04-17T10:52:10Z

As this is a visual glitch it should use a screen test. Follow the pattern of the existing screen tests in ui/inccommand_spec.lua.

nimitbhardwaj · 2018-04-17T11:16:43Z

Ok

nimitbhardwaj · 2018-04-17T13:01:10Z

For the time you analyze it further, I create the test for the this PR

justinmk · 2018-04-21T10:19:55Z

src/nvim/ex_docmd.c

@@ -568,14 +568,19 @@ int do_cmdline(char_u *cmdline, LineGetter fgetline,
       * from a script or when being called recursive (e.g. for ":e
       * +command file").
       */
-      if (!(flags & DOCMD_NOWAIT) && !recursive) {
+      bool isPreview = (State & CMDPREVIEW) == 0 ? false : true;


(State & CMDPREVIEW) == 0 evaluates to boolean, no need to use a ternary. !!(State & CMDPREVIEW) is also a common idiom.

Also we don't use camelCase , so name would be is_preview.

Okk, !!(State & CMDPREVIEW) is good, new style for me. 👍

justinmk · 2018-04-21T10:25:42Z

The AppVeyor failure looks related to this change.

[  ERROR   ] C:/projects/neovim/test/functional\ui\inccommand_spec.lua @ 135: :substitute, 'inccommand' preserves listed buffers (:ls)
test\functional\ui\screen.lua:307: Row 1 did not match.
Expected:
  |*{15:~                             }|
  |{15:~                             }|
  |{15:~                             }|
  |{15:~                             }|
  |{15:~                             }|
  |:ls                           |
  |  1 %a + "[No Name]"          |
  |          line 1              |
  |{13:Press ENTER or type command to}|
  |{13: continue}^                     |
Actual:
  |*^BAC                           |
  |{15:~                             }|
  |{15:~                             }|
  |{15:~                             }|
  |{15:~                             }|
  |{15:~                             }|
  |{15:~                             }|
  |{15:~                             }|
  |{15:~                             }|
  |                              |
To print the expect() call that would assert the current screen state, use

Maybe the test just needs to be adjusted. Need to look at the test, understand what it's doing, and decide if the change from this PR caused a bug, or if the test just needs to be a adjusted.

justinmk · 2018-04-21T10:30:13Z

src/nvim/ex_docmd.c

+        if (!isPreview) {
+          // For execution of a preview command we have to wait for a return
+          // key, but we don't want the message for it pop out
+          no_wait_return++;           // don't wait for return until finished


but we don't want the message for it pop out

If we don't want to wait, why no_wait_return++ for !isPreview ?

Ops, may be wrong msg, I think I was thinking some otherthing, I also change it

justinmk · 2018-04-21T10:31:46Z

The quickbuild BSD failure looks related:

05:06:52,403 INFO  - not ok 2743 - :substitute, inccommand=split shows correct line numbers with many lines
05:06:52,403 INFO  - # test/functional/ui/inccommand_spec.lua @ 1023
05:06:52,403 INFO  - # Failure message: ./test/functional/ui/screen.lua:307: Row 1 did not match.
05:06:52,403 INFO  - # Expected:
05:06:52,403 INFO  - #   |*BBo lines                     |
05:06:52,403 INFO  - #   |Inc substitution on           |
05:06:52,403 INFO  - #   |{12:X}o lines                      |
05:06:52,403 INFO  - #   |Inc substitution on           |
05:06:52,403 INFO  - #   |{12:X}o lines                      |
05:06:52,403 INFO  - #   |{11:[No Name] [+]                 }|
05:06:52,403 INFO  - #   ||1001| {12:X}o lines               |
05:06:52,403 INFO  - #   ||1003| {12:X}o lines               |
05:06:52,403 INFO  - #   ||1005| {12:X}o lines               |
05:06:52,403 INFO  - #   ||1007| {12:X}o lines               |
05:06:52,403 INFO  - #   ||1009| {12:X}o lines               |
05:06:52,403 INFO  - #   ||1011| {12:X}o lines               |
05:06:52,403 INFO  - #   ||1013| {12:X}o lines               |
05:06:52,403 INFO  - #   |{10:[Preview]                     }|
05:06:52,403 INFO  - #   |:%s/tw/X^                      |
05:06:52,403 INFO  - # Actual:
05:06:52,403 INFO  - #   |*Inc substitution on           |
05:06:52,403 INFO  - #   |BBo lines                     |
05:06:52,403 INFO  - #   |Inc substitution on           |
05:06:52,403 INFO  - #   |{12:X}o lines                      |
05:06:52,403 INFO  - #   |Inc substitution on           |
05:06:52,403 INFO  - #   |{11:[No Name] [+]                 }|
05:06:52,403 INFO  - #   ||1001| {12:X}o lines               |
05:06:52,403 INFO  - #   ||1003| {12:X}o lines               |
05:06:52,403 INFO  - #   ||1005| {12:X}o lines               |
05:06:52,403 INFO  - #   ||1007| {12:X}o lines               |
05:06:52,403 INFO  - #   ||1009| {12:X}o lines               |
05:06:52,403 INFO  - #   ||1011| {12:X}o lines               |
05:06:52,403 INFO  - #   ||1013| {12:X}o lines               |
05:06:52,403 INFO  - #   |{10:[Preview]                     }|
05:06:52,403 INFO  - #   |:%s/tw/X^                      |
05:06:52,403 INFO  - #-

nimitbhardwaj · 2018-04-21T11:55:35Z

I correct em

nimitbhardwaj · 2018-04-21T12:10:11Z

By the way why this error occurs, I didn't changed the code of that part, I even didn't touch the code for that describe i.e. of substitute, inccommand=split I put my test in describe of inccommand=nosplit

nimitbhardwaj · 2018-04-21T12:11:11Z

Perhaps I should make the commit again and test if that error persists, and if persists, I will try by removing the test.

nimitbhardwaj · 2018-04-21T12:38:06Z

Lets have a try on this

nimitbhardwaj · 2018-04-21T12:50:40Z

Yes error occurs, this may be due to my change in src, I try what can I do, I am sure, that error didn't occur previously, some changes occur after my work

This is surly now I think the fault of my code, it works for this case, for inccommand=nosplit but I think for inccommand=split it is not correct, I try to find some way to deal with it, I hope to try it later, this is week of my exams
For the other time, if there is some error in my other PRs please tell it to me too, it the error is not so big I would deal it, if it it would require some research, I would make it pending for now

justinmk · 2022-06-26T09:54:26Z

Seems to be fixed on b7084fe

'inccommand' impl changed with #18194

nimitbhardwaj commented Apr 15, 2018

View reviewed changes

marvim added the RFC label Apr 15, 2018

nimitbhardwaj force-pushed the longSubstitute branch 2 times, most recently from 0d341b5 to 745f904 Compare April 16, 2018 11:12

bfredl reviewed Apr 17, 2018

View reviewed changes

nimitbhardwaj force-pushed the longSubstitute branch 2 times, most recently from 4c4f4ac to f17729d Compare April 20, 2018 08:08

nimitbhardwaj changed the title ~~[RFC] Fix for wrapping of large string in preview mode~~ [RDY] Fix for wrapping of large string in preview mode Apr 21, 2018

marvim added RDY and removed RFC labels Apr 21, 2018

justinmk reviewed Apr 21, 2018

View reviewed changes

justinmk changed the title ~~[RDY] Fix for wrapping of large string in preview mode~~ [RFC] Fix for wrapping of large string in preview mode Apr 21, 2018

marvim added RFC and removed RDY labels Apr 21, 2018

Fix for wrapping of large string in preview mode

23ef787

nimitbhardwaj force-pushed the longSubstitute branch from f17729d to 23ef787 Compare April 21, 2018 12:37

justinmk added the gsoc community: Google Summer of Code project label Apr 24, 2018

justinmk changed the title ~~[RFC] Fix for wrapping of large string in preview mode~~ inccommand: fix for wrapping of large string in preview mode Dec 9, 2019

justinmk removed the RFC label Jan 28, 2020

justinmk closed this Jun 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

inccommand: fix for wrapping of large string in preview mode #8283

inccommand: fix for wrapping of large string in preview mode #8283

nimitbhardwaj commented Apr 15, 2018

nimitbhardwaj Apr 15, 2018

nimitbhardwaj Apr 15, 2018

justinmk Apr 16, 2018

nimitbhardwaj Apr 16, 2018

nimitbhardwaj commented Apr 17, 2018 •

edited

bfredl Apr 17, 2018

nimitbhardwaj Apr 17, 2018

nimitbhardwaj Apr 17, 2018

bfredl Apr 17, 2018

nimitbhardwaj Apr 17, 2018

bfredl commented Apr 17, 2018 •

edited

nimitbhardwaj commented Apr 17, 2018

nimitbhardwaj commented Apr 17, 2018

justinmk Apr 21, 2018 •

edited

nimitbhardwaj Apr 21, 2018

justinmk commented Apr 21, 2018

justinmk Apr 21, 2018

nimitbhardwaj Apr 21, 2018

justinmk commented Apr 21, 2018

nimitbhardwaj commented Apr 21, 2018

nimitbhardwaj commented Apr 21, 2018

nimitbhardwaj commented Apr 21, 2018

nimitbhardwaj commented Apr 21, 2018

nimitbhardwaj commented Apr 21, 2018 •

edited

justinmk commented Jun 26, 2022

inccommand: fix for wrapping of large string in preview mode #8283

inccommand: fix for wrapping of large string in preview mode #8283

Conversation

nimitbhardwaj commented Apr 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nimitbhardwaj commented Apr 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bfredl commented Apr 17, 2018 • edited

nimitbhardwaj commented Apr 17, 2018

nimitbhardwaj commented Apr 17, 2018

justinmk Apr 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmk commented Apr 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmk commented Apr 21, 2018

nimitbhardwaj commented Apr 21, 2018

nimitbhardwaj commented Apr 21, 2018

nimitbhardwaj commented Apr 21, 2018

nimitbhardwaj commented Apr 21, 2018

nimitbhardwaj commented Apr 21, 2018 • edited

justinmk commented Jun 26, 2022

nimitbhardwaj commented Apr 17, 2018 •

edited

bfredl commented Apr 17, 2018 •

edited

justinmk Apr 21, 2018 •

edited

nimitbhardwaj commented Apr 21, 2018 •

edited