Re: Working on a first bug - Have some questions

[Kai Willadsen <kai willadsen gmail com>]

On 9 October 2015 at 12:20, David Rabel <David Rabel noresoft com> wrote:
> On 08.10.2015 22:53, Kai Willadsen wrote:
> > On 7 October 2015 at 06:00, David Rabel <David Rabel noresoft com> wrote:
> > > ...
> >
> > Awesome! Could you please just make a note on that page that you're
> > looking at it so that no one else tries to pick it up at the same
> > time?
>
> I wasn't sure what to write. Is it okay as I did?

Yep, that's all good.

> > So for now, I'd ignore the issue of conflicting with style schemes and
> > make the drawing something that will definitely work, e.g., a light
> > grey foreground text colour.
>
> You are right. Attached you find my grey-proposal. :-)

This is looking pretty good I think. If you'd like to keep this on the
list that's cool, but at this point I'd suggest attaching the patch in
bugzilla and we can tidy up minor details on there.

> > I'm not sure if you were done with it, but that patch doesn't quite do
> > the right thing when you get multiple ignored regions. I think the
> > problem is that the _filter_text changes assume that it's only called
> > with the whole file, but we actually call it whenever we request a
> > slice from meldbuffer.BufferLines (which is quite a bit). I think
> > you'll have to change the _filter_text call to take buffer offsets
> > from the slice call and do offset calculations from there.
>
> I tried to fix this issue. I think it is correct, but maybe you could
> have a closer look at it, because I was not able to reproduce the
> problem reliably. Therefore I'm not sure, if I am lucky or it is fixed. ;-)

Yeah this looks good to me I think.

> I have one more problem:
>
> I don't really understand how the groups()-thing in killit() is supposed
> to work.
<snip>

You want to know something? Neither do I. This is very old code; it
hasn't been touched in quite literally ten years. One Day I will get
around to rewriting the text filter system... but today is not that
day.

For now I'd suggest erring on the side of keeping the current
behaviour, even if it seems slightly odd. I think your patch does
this. I'm going to try and take a look at this however, because I
think that the current code really doesn't do the right thing, and I
think the correct-if-somewhat-hairy offset calculations in this patch
could be a lot nicer if we just used the group offsets rather than
doing search + replace hacks.

> I did write some not-so-beautiful lines that dim exactly what is
> filtered out thou, because it was easier to see what is going on, when I
> could really visually see it.

Yep, that looks good to me. I'm very slightly concerned that this will
also dim text in inline highlighting, even though we don't actually
apply text filters for those differences. This isn't a big deal
though, and we should be able to fix it in the future if needs be.

cheers,
Kai