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

From: Silvan Jegen <s.jegen_AT_gmail.com>
Date: Fri, 22 Aug 2014 22:13:06 +0200

On Fri, Aug 22, 2014 at 12:54:55PM -0700, Ben Hendrickson wrote:
> 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?

There are about 328 instances of 'if(' so I would say sending a v2 would
be preferrable. See below though.


> >> + 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.

I don't have write access to the repo so waiting for someone who does
to review the patch is probably best.
Received on Fri Aug 22 2014 - 22:13:06 CEST