Loading ...
Sorry, an error occurred while loading the content.
 

Redrawing bug in MacVim + Command-T since commit ba44868

Expand Messages
  • 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
    Message 1 of 11 , Nov 3, 2010
      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

      --
      You received this message from the "vim_mac" maillist.
      Do not top-post! Type your reply below the text you are replying to.
      For more information, visit http://www.vim.org/maillist.php
    • 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
      Message 2 of 11 , Nov 3, 2010
        On 3 November 2010 17:38, Wincent Colaiuta <win@...> wrote:
        > 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?

        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

        --
        You received this message from the "vim_mac" maillist.
        Do not top-post! Type your reply below the text you are replying to.
        For more information, visit http://www.vim.org/maillist.php
      • Wincent Colaiuta
        ... 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
        Message 3 of 11 , Nov 4, 2010
          On Nov 3, 9:11 pm, björn <bjorn.winck...@...> 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

          --
          You received this message from the "vim_mac" maillist.
          Do not top-post! Type your reply below the text you are replying to.
          For more information, visit http://www.vim.org/maillist.php
        • 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
          Message 4 of 11 , Nov 4, 2010
            On 4 November 2010 09:13, Wincent Colaiuta <win@...> wrote:
            > On Nov 3, 9:11 pm, björn <bjorn.winck...@...> 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.

            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

            --
            You received this message from the "vim_mac" maillist.
            Do not top-post! Type your reply below the text you are replying to.
            For more information, visit http://www.vim.org/maillist.php
          • Wincent Colaiuta
            ... Perhaps a change in Mac OS X? ... I can see three places where we are using CLEAR as a special case for MacVim: - in win_split_ins(), which is used for
            Message 5 of 11 , Nov 4, 2010
              On Nov 4, 3:35 pm, björn <bjorn.winck...@...> 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

              --
              You received this message from the "vim_mac" maillist.
              Do not top-post! Type your reply below the text you are replying to.
              For more information, visit http://www.vim.org/maillist.php
            • 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
              Message 6 of 11 , Nov 4, 2010
                On 4 November 2010 18:18, Wincent Colaiuta <win@...> wrote:
                > On Nov 4, 3:35 pm, björn <bjorn.winck...@...> 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.

                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

                --
                You received this message from the "vim_mac" maillist.
                Do not top-post! Type your reply below the text you are replying to.
                For more information, visit http://www.vim.org/maillist.php
              • björn
                ... 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
                Message 7 of 11 , Nov 6, 2010
                  On 4 November 2010 20:08, björn <bjorn.winckler@...> 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

                  --
                  You received this message from the "vim_mac" maillist.
                  Do not top-post! Type your reply below the text you are replying to.
                  For more information, visit http://www.vim.org/maillist.php
                • 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
                  Message 8 of 11 , Dec 21, 2010
                    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

                    --
                    You received this message from the "vim_mac" maillist.
                    Do not top-post! Type your reply below the text you are replying to.
                    For more information, visit http://www.vim.org/maillist.php
                  • Wincent Colaiuta
                    ... Thanks, Björn. Will test this out now and let you know if I see any problems. Cheers, Wincent -- You received this message from the vim_mac maillist. Do
                    Message 9 of 11 , Dec 21, 2010
                      On 21 dic, 11:35, björn <bjorn.winck...@...> 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

                      --
                      You received this message from the "vim_mac" maillist.
                      Do not top-post! Type your reply below the text you are replying to.
                      For more information, visit http://www.vim.org/maillist.php
                    • Wincent Colaiuta
                      ... I ve now run it through its paces and it looks ok. Cheers, Wincent -- You received this message from the vim_mac maillist. Do not top-post! Type your
                      Message 10 of 11 , Dec 21, 2010
                        On 21 dic, 12:49, Wincent Colaiuta <w...@...> 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

                        --
                        You received this message from the "vim_mac" maillist.
                        Do not top-post! Type your reply below the text you are replying to.
                        For more information, visit http://www.vim.org/maillist.php
                      • björn
                        ... Thanks for the confirmation and for the very thorough initial report as well! Björn -- You received this message from the vim_mac maillist. Do not
                        Message 11 of 11 , Dec 21, 2010
                          On 21 December 2010 17:59, Wincent Colaiuta <win@...> wrote:
                          > On 21 dic, 12:49, Wincent Colaiuta <w...@...> 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.

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

                          Björn

                          --
                          You received this message from the "vim_mac" maillist.
                          Do not top-post! Type your reply below the text you are replying to.
                          For more information, visit http://www.vim.org/maillist.php
                        Your message has been successfully submitted and would be delivered to recipients shortly.