Re: [Nano-devel] [PATCH 1/2] new feature: pipe selected text to external command when executing one

[Marco Diego Aurélio Mesquita]

On Fri, May 11, 2018 at 1:30 PM, Benno Schulenberg <address@hidden> wrote:
>
> Hello Marco,

Hi Benno!

>
> Op 11-05-18 om 04:13 schreef Marco Diego Aurélio Mesquita:
>> These patches improve somewhat on what we had. It uses two
>> simultaneous processes to pipe in and out, so we won't stuck because
>> of buffers; implements what was discussed about the '|' to enable
>> "pipeing"
>
> Yeah, I had forgotten about that.  Hmm... I thought implementation of
> this would be simpler, but... for now it works.
>

Good!

> What does not work correctly is that when nothing is marked, and we're
> piping, the entire buffer should be replaced with the output of the
> external command -- the output should not be inserted into it, which
> kind of doubles the the buffer.
>

Fixed on the attached patch set.

>> I really would like to get this feature in.
>
> It won't make it into 2.9.7, that's for sure.
>

Ok.

>> Is the undo thing really
>> necessary for now? Can't it be done later?
>
> Maybe.
>

Hope so.

>> +                                     fprintf(stderr, "First char is a pipe, 
>> we should remove it.\n");
>
> A debugging leftover?
>

Yes. Removed on the attached patchset.

>> +                             statusbar_x = strlenpt(answer) + 1;
>
> This is not right.  If the cursor is somewhere in the middle of the command,
> then toggling the pipe thing should leave the cursor on the exact same
> character.
>

Done.

>> +             add_to_sclist(MEXTCMD, "^\\", 0, flip_pipe, 0);
>
> I don't like ^\ as a toggle.  The other toggles are with Meta.
> See attached patch for other minor adjustments -- for example
> to not change the position of the M-F toggle in the help lines.
>

Done.

>> +     if (should_pipe) {
>> +             if (ISSET(MULTIBUFFER))
>> +                     switch_to_prev_buffer();
>
> Why do you need to change buffer?  What if there is no other buffer?
>

To allow the output of the command to go to another buffer. This gives
the user the flexibility of marking some part of the text, pipe it to
an external tool and have its output in a new buffer.

>> +             if(has_selection) {
>> +             #ifndef NANO_TINY
>> +                     add_undo(CUT);
>> +             #endif
>> +                     do_cut_text(ISSET(MULTIBUFFER), !has_selection);
>> +             #ifndef NANO_TINY
>> +                     update_undo(CUT);
>> +             #endif
>> +             }
>
> Oof...  This looks like a duplication of something else.  And besides,
> the #ifs end #endifs should be at the start of the line.
>

Fixed.

>> @@ -476,7 +476,7 @@ size_t strlenpt(const char *text)
>>  {
>>       size_t span = 0;
>>
>> -     while (*text != '\0')
>> +     while (text && *text != '\0')
>>               text += parse_mbchar(text, NULL, &span);
>
> No, don't make any of the util functions slower.  Which should be easy,
> because you don't need strlenpt() at all.
>

Fixed.

Thanks to the review. Hope it is good enough now.

0001-Pipe-slected-text-to-external-command.patch
Description: Text Data

0004-Minor-improvements-to-pipe-to-external-tool-feature.patch
Description: Text Data

0003-Reshuffle-some-code-and-improve-some-variable-names.patch
Description: Text Data

0002-Use-in-the-first-char-at-a-command-to-determine-if-t.patch
Description: Text Data