Re: [dev] [PATCH] Removing wrapping newlines from selection

From: Ben Hendrickson <ben_AT_1m7.com>
Date: Fri, 22 Aug 2014 12:54:55 -0700

On Fri, Aug 22, 2014 at 12:00 PM, Silvan Jegen <s.jegen_AT_gmail.com> wrote:
>> + if (term.line[y][i - 1].mode & ATTR_WRAP)
>
> The preferred style in st.c seems to be the one without a space
> after the 'if'. There still are about 18 other places where this
> convention is broken however.

Good point. What is protocol here? Should I send a v2 patch without the space?

>> + return i;
>> +
>> while (i > 0 && term.line[y][i - 1].c[0] == ' ')
>> --i;
>>
>> _AT_@ -959,7 +962,7 @@ getsel(void) {
>> * st.
>> * FIXME: Fix the computer world.
>> */
>> - if(sel.ne.y > y || lastx >= linelen)
>> + if((y < sel.ne.y || lastx >= linelen) && !(last->mode & ATTR_WRAP))
>
> Why did you change the order in the first clause? Not that I mind too
> much, just curious.

Writing the clause as "y < sel.ne.y" makes it more consistent with the
condition in the for loop which is "y < sel.ne.y + 1". Making these
more consistent makes it clearer that this is a check for the last
iteration of the loop.

That said, I notice now there are a couple places in the function with
"el.ne.y == y". My reordering made it less consistent with those
places. So perhaps it is a wash.

Regardless, if you'd like a v2 patch, I'm happy to swap the ordering back.

Ben
Received on Fri Aug 22 2014 - 21:54:55 CEST