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

[Marco Diego Aurélio Mesquita]

Just a small oversight: we don't need to care about statusbar_x anymore.

Attached patchset should be good.

Thanks!

On Fri, May 11, 2018 at 3:51 PM, Marco Diego Aurélio Mesquita
<address@hidden> wrote:
> 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