[Toybox] [PATCH] vi.c: line ranges, fixed line gotos, CTRL-D, etc

[Oliver Webb]

------- Original Message -------
On Sunday, October 1st, 2023 at 1:33 PM, Jarno Mäkipää <jmakip87 at gmail.com> wrote:


> On Sun, Oct 1, 2023 at 6:30 PM Oliver Webb aquahobbyist at proton.me wrote:
> 
> > ------- Original Message -------
> > On Sunday, October 1st, 2023 at 7:50 AM, Jarno Mäkipää jmakip87 at gmail.com wrote:
> > 
> > > Hello Oliver
> > > 
> > > ex mode command parsing is not something that has been designed
> > > carefully. Its more of hack to accept write file and exit commands in
> > > order to make editor somewhat usable. Even the vi mode command
> > > execution is not very good. At least one issue is that it has function
> > > dispatch table that handles most of the commands and also switch case
> > > that handles some... It should probably have only one or the other and
> > > not both...
> > > 
> > > I see that you added some direct calls to run_vi_cmd() in ex mode and
> > > other places. I am not sure is that going to work out since run_vi_cmd
> > > was supposed to be only called when in vi mode.
> > 
> > Originally, I wanted to stay away from run_vi_cmd() for speed reasons. And
> > instead call what run_vi_cmd called. As a example: after some debug
> > printf-ing, I figured out what the "G" vi command did (calling vi_go())
> > and, I called vi_go(NUMBER, 1, 0) instead of run_vi_cmd(xmprintf("%dG",NUMBER)),
> > Problems arose with this though that don't normally appear whilst
> > doing run_vi_cmd("G"). Not moving around at the top or bottom files, Weird
> > stuff happening when moving around with "hjkl" before calling the command,
> > etc. It must be some setup procedure that run_vi_cmd does before or after
> > calling the vi_go() in the dispatch table because I call it in the
> > exact same manner with the same arguments. I haven't done enough debugging to
> > pin down the reason, and since run_vi_cmd() works I just called that.
> > 
> > > I don’t really use ex
> > > commands other than r, w, q so I don’t really have idea how they work.
> > > But if you wish to implement some commonly used ones,
> > 
> > I am planning to implement 'g' ('v' follows once you have that)
> > and maybe 's' if I can manage it reasonably.
> > After some consideration 'p' is obsolete in the same way that
> > 'i' and 'a' (the ex commands, not the vi ones) are.
> > It may be useful for g/re/p-ing though a file for lines far apart.
> > But after looking at how error messages work in vi, I honestly
> > think it would be more trouble then it's worth. 'e' would be nice.
> > But reloading the buffer and essentially resetting the editor is also probably
> > also more trouble then it's worth. I plan to implement system commands to give
> > it the UNIX modularity that's ever so useful. and if I am going to be reading
> > from the stdout of a command and placing it back into a file, reading a file with 'r'
> > wouldn't be that heavy of a lift.
> 
> 
> e[dit] and r[ead] are trivial to implement this point. all the
> plumbing for loading and reloading buffer is already there and also
> mapping another file into current position of buffer.

I was unaware of this. I haven't read through the entirety of vi.c to see exactly what it does.
But if reloading the buffer and mapping files is already built it then that would make r[ead],
e[dit] and the !system commands a lot easier to implement.

> > > you could try to
> > > implement better parsing logic for ex commands so that they are parsed
> > > similar logic as vi mode: do movement, execute action based on
> > > movement, repeat if counter has been given.
> > 
> > A lot of ex commands don't necessarily do movement, 's' for example for example shouldn't
> > effect cursor position. If a "counter" (Line range) is given, it loops through the line range
> > and executes the command for each line before going down to the next one, this is why 'd' goes up after
> > deleting the line, It should stay on the current line so we don't skip any.
> 
> 
> By looking man page for ex. There is this [2addr]
> cmd[buffer][count][flags] logic that some commands follow, where 2addr
> argument is relatively same thing as movement in vi. By meaning that
> you search beginning and end positions for operation. But then again
> most of commands dont exactly follow this and there is special case
> after special case. making generic ex command parsing logic might be
> pain to implement.
> 
> > > If you plan to add features you could add some tests into /tests/vi.test
> > 
> > I have added a few test cases for the ex commands in the patch attached to this email
> > 
> > > While tests don’t cover how buffer looks visually in terminal
> > > emulator, it can cover how buffer is edited and stored on output file.
> > > This can detect at least some regressions.
> > > 
> > > -Jarno
> > 
> > - Oliver Webb aquahobbyist at proton.me
> 
> 
> I was looking at the implementation you have added and noticed there
> is some code indentation issues, could you replace tabs with 2 spaces
> in order to follow project rules. Aim is to follow this style guide.

The below patch replaces tabs with 2 spaces in vi.c.  

> http://landley.net/toybox/design.html#codestyle
> 
> -Jarno

- Oliver Webb <aquahobbyist at proton.me>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Replace-tabs-with-spaces-in-vi.c.patch
Type: text/x-patch
Size: 3379 bytes
Desc: not available
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20231001/61d6eb4a/attachment.bin>