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

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

Expand Messages
  • 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 1 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 2 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 3 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 4 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 5 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 6 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 7 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.