Redrawing bug in MacVim + Command-T since commit ba44868

[Wincent Colaiuta]

Hi,

I'm the author of the Command-T plug-in and I've received multiple
reports of a redraw issue in MacVim after moving to 7.3:

https://wincent.com/issues/1692

The bug is that Command-T updates the status line, and then MacVim
repaints it immediately, causing the status line to appear blank.

This issue is specific to MacVim and doesn't occur in upstream Vim
built from source.

`git bisect` reveals that the commit which introduced the glitch is
commit ba4486860:

https://github.com/b4winckler/macvim/commit/ba4486860ecda4b5e3584c0938abcc5046f1beaa

The commit message reads:

> Fix display corruption when dragging divider
>
> This fixes a bug where the screen would get corrupted when dragging a
> horizontal divider in full-screen mode.

So the problem here is that it doesn't just fix the corruption while
dragging the horizontal divider in full-screen mode, it actually
breaks things when not in full-screen mode, and in fact when running
from the Terminal too (ie. not inside the Mac GUI do there aren't any
draggable dividers in that case).

Any thoughts on a way to make this fix more specific (ie. to fix the
full-screen dragging issue without causing secondary breakage)? As a
quick guess, I thought I'd try swapping in the "NOT_VALID" flag
instead of the "CLEAR" one as shown in this gist:

https://gist.github.com/661327

This does indeed make the Command-T disappearing status line issue go
away, and in my playing around with dividers in full screen mode, I
was unable to provoke any corruption.

This is just my 30 second guess at a fix though, from someone who
doesn't really know the redrawing code at all, so it may be stupid.

Thoughts?

Cheers,
Wincent

[björn]

Thanks for looking into this -- I have to take a look but I think your
fix may be the right one. Please send me a patch (git format-patch)
so that you appear as the patch author (if you wish).

I don't remember how to recreate the display bug that the commit above
fixed...I think you need to have both vertical and horizontal splits
(with text in each view) and then drag the dividers.

Björn

[Wincent Colaiuta]

On Nov 3, 9:11 pm, björn <bjorn.winck...@gmail.com> wrote:
>
> Thanks for looking into this -- I have to take a look but I think your
> fix may be the right one.  Please send me a patch (git format-patch)
> so that you appear as the patch author (if you wish).
>
> I don't remember how to recreate the display bug that the commit above
> fixed...I think you need to have both vertical and horizontal splits
> (with text in each view) and then drag the dividers.

Ok, here's the patch:

https://gist.github.com/662226

Full disclosure:

- just to make sure that this change doesn't introduce a regression of
the display corruption issue, I tried many combinations of vertical
and horizontal splits with text in them, in both windowed and full-
screen mode, with and without the experimental renderer, and I
couldn't trigger any corruption

- to be doubly sure, I also built Vim without the initial fix (ie.
with SOME_VALID rather than CLEAR or NOT_VALID) and even then I
couldn't reproduce the display corruption

So, basically, I can't be 100% sure that this won't regress because
I'm unable to reproduce the original issue. Funnily enough, I reported
the display corruption ages ago:

http://code.google.com/p/macvim/issues/detail?id=262

And using the recipe described there, I can't reproduce with any of
SOME_VALID, CLEAR and NOT_VALID.

Cheers,
Wincent

[björn]

Thanks. I just tried reproducing with snapshot 52 (the version you
used when reporting this issue originally) and I can't repro either.
Weird.

I'm be happy to merge this, if there is a regression we'll just have
to find another fix, but...I notice several places in window.c where
the same line that you just patched appears. Should we perhaps change
all of them to NOT_VALID instead of CLEAR? I have to take a closer
look, but I'd value your input.

Björn

[Wincent Colaiuta]

On Nov 4, 3:35 pm, björn <bjorn.winck...@gmail.com> wrote:
>
> Thanks.  I just tried reproducing with snapshot 52 (the version you
> used when reporting this issue originally) and I can't repro either.
> Weird.

Perhaps a change in Mac OS X?

> I'm be happy to merge this, if there is a regression we'll just have
> to find another fix, but...I notice several places in window.c where
> the same line that you just patched appears.  Should we perhaps change
> all of them to NOT_VALID instead of CLEAR?  I have to take a closer
> look, but I'd value your input.

I can see three places where we are using CLEAR as a special case for
MacVim:

- in win_split_ins(), which is used for inserting a new split

- in win_split(), which is used for splitting an existing window

- in win_new_height(), which is used to set the vertical height of the
window

To be honest I am not sure why it's needed in any of those places, as
one would think that SOME_VALID or NOT_VALID would be enough; quoting
from the definitions in src/vim.h:

#define SOME_VALID 35 /* like NOT_VALID but may scroll
*/
#define NOT_VALID 40 /* buffer needs complete redraw
*/
#define CLEAR 50 /* screen messed up, clear it */

The meaning of these is described in depth in src/screen.c:

* Commands that change how a window is displayed (e.g., setting
'list') or
* invalidate the contents of a window in another way (e.g., change
fold
* settings), must call redraw_later(NOT_VALID) to have the whole
window
* redisplayed by update_screen() later.
*
* Commands that change how a buffer is displayed (e.g., setting
'tabstop')
* must call redraw_curbuf_later(NOT_VALID) to have all the windows
for the
* buffer redisplayed by update_screen() later.
*
* Commands that change highlighting and possibly cause a scroll too
must call
* redraw_later(SOME_VALID) to update the whole window but still use
scrolling
* to avoid redrawing everything. But the length of displayed lines
must not
* change, use NOT_VALID then.
*
* Commands that move the window position must call
redraw_later(NOT_VALID).
* TODO: should minimize redrawing by scrolling when possible.
*
* Commands that change everything (e.g., resizing the screen) must
call
* redraw_all_later(NOT_VALID) or redraw_all_later(CLEAR).

So, I don't know. I would think that CLEAR would never be needed, to
be honest, when NOT_VALID/SOME_VALID is enough on other platforms...

But yeah, as you say, there's always the risk of regressing.

Cheers,
Wincent

[björn]

The problem as I can remember it (with the Core Text renderer) is that
splitting a window may cause a scrollbar to appear, thereby causing
the entire view to be shifted to the side. In this situation the
entire view really needs to be redrawn since Cocoa doesn't shift the
contents to make place for the scrollbar.

From what I can tell NOT_VALID should still be enough though so your
patch may still not cause any regressions.

Björn

[björn]

On 4 November 2010 20:08, björn <bjorn.w...@gmail.com> wrote:
>
> The problem as I can remember it (with the Core Text renderer) is that
> splitting a window may cause a scrollbar to appear, thereby causing
> the entire view to be shifted to the side.  In this situation the
> entire view really needs to be redrawn since Cocoa doesn't shift the
> contents to make place for the scrollbar.
>
> From what I can tell NOT_VALID should still be enough though so your
> patch may still not cause any regressions.

I had another look and two of the CLEARs are definitely there because
of the scrollbar appearing. However, the CLEAR that your patch gets
rid of has nothing to do with this. Instead there seems to be some
problem with glyphs spilling over into the neighboring cell and under
certain circumstances these do not get cleared when dragging a
horizontal divider. This is how I can reproduce the problem:

1. go full screen (with Experimental Renderer!)
2. open a file, then :vsp
3. :sp some-other-file (this file should preferably have lots of text
with "w" in it)
4. drag the horizontal divider

Result: little blue dots litter the screen (left over by "w":s that
move as the result of dragging the divider). (I do this with a dark
color scheme and the default font.)

These artifacts disappear if the CLEAR flag is used instead of
NOT_VALID or SOME_VALID (in the spot your patch applies changes).

Mysteriously enough I cannot reproduce the same problems in windowed
mode (only full screen).

I must admit that the reason that I use "CLEAR" in the first place has
to do with how the experimental renderer tries to redraw as little of
the screen as possible and a more robust way of doing rendering would
get rid of all of these problems but it would also be slower.

I'm not sure what to do next. I could apply your patch (or revert the
one that introduced this problem) at the expense of introducing (tiny)
rendering artifacts that can be gotten rid of by pressing Ctrl-l. Or,
I could leave things as they are and try to come up with a better
rendering method (which I would not have time to do until the middle
of next year at the earliest). I'll have to think about it.

Björn

[björn]

Hello Wincent and vim_mac readers,

I had a look at this problem again last night and came up with a
solution [1] that seems to work fine but it may need some adjustment.
The CLEAR flag is no longer used so the Command-T plugin should work
fine now (please confirm).

To avoid the display corruption issue I had to be a bit creative, so
now whenever a cell is cleared the neighboring cells are cleared too
if they are blank. This takes care of the problem when a glyph spills
over into a neighboring (empty) display cell -- it used to be that
when such a glyph was erased it left a little trace behind in the
neighboring cell. The code path is pretty much the same as is used by
other GUIs when fake bold fonts are used (which often also spill over
into neighboring cells). I have not noticed slower rendering speeds
because of this change but in theory more clearing is done so keep an
eye out.

I'm guessing that there may still be a problem with glyphs which stick
out into display cells on top or below, but this will be font
dependent and I have not been able to reproduce with the default font.
Still, please report any reliable ways to reproduce display
corruptions and I will take a look (and probably not be able to fix
them easily, but I will still try).

For prosperity let me also mention that I experimented with clipping
each glyph to the size of one display cell to completely avoid the
"spilling" problem but this made the rendering unbearably slow.

Björn

[1] https://github.com/b4winckler/macvim/commit/caabb3f058870aec3baad9553d402d5d43a618e4

[Wincent Colaiuta]

On 21 dic, 11:35, björn <bjorn.winck...@gmail.com> wrote:
>
> I had a look at this problem again last night and came up with a
> solution [1] that seems to work fine but it may need some adjustment.
> The CLEAR flag is no longer used so the Command-T plugin should work
> fine now (please confirm).

Thanks, Björn. Will test this out now and let you know if I see any
problems.

Cheers,
Wincent

[Wincent Colaiuta]

On 21 dic, 12:49, Wincent Colaiuta <w...@wincent.com> wrote:
>
> Thanks, Björn. Will test this out now and let you know if I see any
> problems.

I've now run it through its paces and it looks ok.

Cheers,
Wincent

[björn]

Thanks for the confirmation and for the very thorough initial report as well!

Björn