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

Re: Redrawing bug in MacVim + Command-T since commit ba44868

Expand Messages
  • 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 1 of 11 , Nov 4, 2010
    • 0 Attachment
      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 2 of 11 , Nov 4, 2010
      • 0 Attachment
        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 3 of 11 , Nov 4, 2010
        • 0 Attachment
          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 4 of 11 , Nov 6, 2010
          • 0 Attachment
            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 5 of 11 , Dec 21, 2010
            • 0 Attachment
              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 6 of 11 , Dec 21, 2010
              • 0 Attachment
                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 7 of 11 , Dec 21, 2010
                • 0 Attachment
                  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 8 of 11 , Dec 21, 2010
                  • 0 Attachment
                    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.