breakindent bug

[Christian Brabandt]

Hi,
I have found a breakindent bug.

Take the attached script and source it. Note, how the final d seems to be
further to the right, then expected. (It isn't even reachable, if you
press $ the cursor will stay before it, but pressing ga will show, that
you are actually on the correct location).

The bug seems to be, that under certain conditions, an extra indent is
applied, although it isn't necessary.

Here is a half-working patch that seems to fix it, but I am not sure,
this is the right way and also this doesn't work, if 'sbr' is set.

diff --git a/src/screen.c b/src/screen.c
--- a/src/screen.c
+++ b/src/screen.c
@@ -3713,6 +3713,10 @@ win_line(wp, lnum, startrow, endrow, noc
c_extra = ' ';
n_extra = get_breakindent_win(wp,
ml_get_buf(wp->w_buffer, lnum, FALSE));
+ /* reset n_extra, when the tabstop would be at the correct
+ * location without bri */
+ if (col == 0 && n_extra == vcol % wp->w_buffer->b_p_ts)
+ n_extra = 0;
/* Correct end of highlighted area for 'breakindent',
* required when 'linebreak' is also set. */
if (tocol == vcol)


Best,
Christian
--
Frage an Radio Eriwan:
"Darf ich jetzt wieder Äpfel aus Tschernobyl essen?"
Antwort:
"Im Prinzip ja. Aber die Kerne müssen Sie danach in einem Bleifaß vergraben."

[Bram Moolenaar]

Christian Brabandt wrote:

> Hi,
> I have found a breakindent bug.
>
> Take the attached script and source it. Note, how the final d seems to be
> further to the right, then expected. (It isn't even reachable, if you
> press $ the cursor will stay before it, but pressing ga will show, that
> you are actually on the correct location).
>
> The bug seems to be, that under certain conditions, an extra indent is
> applied, although it isn't necessary.
>
> Here is a half-working patch that seems to fix it, but I am not sure,
> this is the right way and also this doesn't work, if 'sbr' is set.

Thanks, but I think I'll wait until you have a full-working patch :-).


--
hundred-and-one symptoms of being an internet addict:
33. You name your children Eudora, Mozilla and Dotcom.

/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ an exciting new programming language -- http://www.Zimbu.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///

[Christian Brabandt]

On So, 17 Aug 2014, Bram Moolenaar wrote:

>
> Christian Brabandt wrote:
>
> > Hi,
> > I have found a breakindent bug.
> >
> > Take the attached script and source it. Note, how the final d seems to be
> > further to the right, then expected. (It isn't even reachable, if you
> > press $ the cursor will stay before it, but pressing ga will show, that
> > you are actually on the correct location).
> >
> > The bug seems to be, that under certain conditions, an extra indent is
> > applied, although it isn't necessary.
> >
> > Here is a half-working patch that seems to fix it, but I am not sure,
> > this is the right way and also this doesn't work, if 'sbr' is set.
>
> Thanks, but I think I'll wait until you have a full-working patch :-).

Haha ;)

Okay, here we go:

diff --git a/src/charset.c b/src/charset.c
--- a/src/charset.c
+++ b/src/charset.c
@@ -1195,10 +1195,7 @@ win_lbr_chartabsize(wp, line, s, col, he
if (wp->w_p_bri)
added += get_breakindent_win(wp, line);

- if (tab_corr)
- size += (added / wp->w_buffer->b_p_ts) * wp->w_buffer->b_p_ts;
- else
- size += added;
+ size += added;
if (col != 0)
added = 0;
}

I am not entirely sure, about this and win_lbr_chartabsize is used in
many different places, so please review. I'll try to update
test_breakindent, but so far, I wasn't able to reproduce the issue
there.

Best,
Christian
--
Von den Gipfeln dieser Pyramiden blicken 40 Jahrhunderte auf uns
herab.
-- Napoleon I. Bonaparte

[Bram Moolenaar]

Christian Brabandt wrote:

> > > Hi,
> > > I have found a breakindent bug.
> > >
> > > Take the attached script and source it. Note, how the final d seems to be
> > > further to the right, then expected. (It isn't even reachable, if you
> > > press $ the cursor will stay before it, but pressing ga will show, that
> > > you are actually on the correct location).
> > >
> > > The bug seems to be, that under certain conditions, an extra indent is
> > > applied, although it isn't necessary.
> > >
> > > Here is a half-working patch that seems to fix it, but I am not sure,
> > > this is the right way and also this doesn't work, if 'sbr' is set.
> >
> > Thanks, but I think I'll wait until you have a full-working patch :-).
>
> Haha ;)
>
> Okay, here we go:

Thanks!

[..]

> I am not entirely sure, about this and win_lbr_chartabsize is used in
> many different places, so please review. I'll try to update
> test_breakindent, but so far, I wasn't able to reproduce the issue
> there.

You could reproduce the problem manually but not in a test?

--
hundred-and-one symptoms of being an internet addict:

37. You start looking for hot HTML addresses in public restrooms.

[Christian Brabandt]

On Mo, 18 Aug 2014, Bram Moolenaar wrote:

> Christian Brabandt wrote:

> > > > I have found a breakindent bug.

It's actually not a breakindent bug. The breakindent feature just makes
it more apparent. I can reproduce the issue on a plain 7.4 version.
Looking at the mercurial repository, that particular code is present
since the beginning of version 7. So I suspect, even the first Vim 7
also had the same problem.

The problem is, the display is correct, but the underlying data is
wrong.

Attached is a patch including a test.

Best,
Christian
--

[Bram Moolenaar]

If it's not a breakindent bug, then why did you call the test
test_breakindent_bug? If it is related to breakindent, perhaps we can
add this to the existing test. It's faster to execute.


--
Change is inevitable, except from a vending machine.

[Christian Brabandt]

On Di, 19 Aug 2014, Bram Moolenaar wrote:

> Christian Brabandt wrote:
>
> > On Mo, 18 Aug 2014, Bram Moolenaar wrote:
> >
> > > Christian Brabandt wrote:
> > > > > > I have found a breakindent bug.
> >
> > It's actually not a breakindent bug. The breakindent feature just makes
> > it more apparent. I can reproduce the issue on a plain 7.4 version.
> > Looking at the mercurial repository, that particular code is present
> > since the beginning of version 7. So I suspect, even the first Vim 7
> > also had the same problem.
> >
> > The problem is, the display is correct, but the underlying data is
> > wrong.
> >
> > Attached is a patch including a test.
>
> If it's not a breakindent bug, then why did you call the test
> test_breakindent_bug? If it is related to breakindent, perhaps we can
> add this to the existing test. It's faster to execute.

I started writing the test, when I thought it would be related to the
breakindent. I did rename the file, but unfortunately, I sent an old
version. Here is the updated patch.

Best,
Christian
--
Wer den Schaden hat, spottet jeder Beschreibung.
-- Heinz Erhardt

[Bram Moolenaar]

Thanks. Somehow for me appending _bug means you test for the bug to be
present :-). We can just call it test_linebreak. Oh, and keep them in
alphabetical order. I started doing that recently.


--
It is hard to understand how a cemetery raised its burial
cost and blamed it on the cost of living.

[Christian Brabandt]

On Di, 19 Aug 2014, Bram Moolenaar wrote:

> Thanks. Somehow for me appending _bug means you test for the bug to be
> present :-). We can just call it test_linebreak. Oh, and keep them in
> alphabetical order. I started doing that recently.

Okay, how about that one (I added a small README.txt for the test
directory, that contains your remarks).

Best,
Christian
--

[Bram Moolenaar]

Thanks!


--
The 50-50-90 rule: Anytime you have a 50-50 chance of getting
something right, there's a 90% probability you'll get it wrong.