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

[vim-multibyte] Re: PATCH: Fix for bold characters spilling over left side in GUI.

Expand Messages
  • Park Chong-Dae
    ... Your new patch does not seem to be efficient. It updates the whole line. I ll try to make another patch based on your idea. -- Chong-Dae Park
    Message 1 of 10 , Mar 23, 2000
    • 0 Attachment
      On Fri, Mar 24, 2000 at 12:58:42PM -0000, Robert Webb wrote:
      > Chong-Dae Park wrote:
      >
      > > The variable "is_dbcs" is always set to TRUE in MULTI_BYTE
      > > envionment. So in MULTI_BYTE, your patch makes vim force to redraw
      > > the whole line. I think it is not a good behavior.
      >
      > Woops! This really was just a silly error of mine. Attached is a
      > patch to put on top of my first patch, which should fix things for
      > multi-byte.

      Your new patch does not seem to be efficient.
      It updates the whole line.

      I'll try to make another patch based on your idea.

      --
      Chong-Dae Park
    • Park Chong-Dae
      ... Yes. second occuerance of *(screenp_to + 1) = 0; is redundant. I just leave it there just for readability (Yes. It is better to comment out!) and the
      Message 2 of 10 , Mar 24, 2000
      • 0 Attachment
        On Fri, Mar 24, 2000 at 12:58:42PM -0000, Robert Webb wrote:
        > I wrote:
        >
        > > I think there was something wrong with this bit of code:
        > >
        > > #ifdef MULTI_BYTE
        > > if (is_dbcs)
        > > {
        > > /* The trick to make a force update */
        > > if (!IsLeadByte(*screenp_from) && IsLeadByte(*screenp_to))
        > > *(screenp_to + 1) = 0;
        > > else if ((char_bytes == 2)
        > > && mb_isbyte1(LinePointers[row], col + 1))
        > > {
        > > *(screenp_to + 1) = 0;
        > > *(screenp_to + 2) = 0;
        > > }
        > > }
        > > #endif
        > >
        > > *screenp_to = *screenp_from;
        > > #ifdef MULTI_BYTE
        > > if (char_bytes == 2)
        > > *(screenp_to + 1) = *(screenp_from + 1);
        > > #endif
        >
        > And Bram replied:
        >
        > > You are missing something: This code checks if the new byte is a
        > > single-byte character while the old byte was a double-byte
        > > character. The next character must be redrawn then, since the right
        > > halve of the double-width character would still be there. The other
        > > situation is when writing a double-width character which overwrites
        > > the left halve of an existing double-width character.
        >
        > OK, so I still don't really understand multi-byte stuff, but the line
        > "*(screenp_to + 1) = 0;" above was redundant, since it only happens
        > when char_bytes is 2, and if char_bytes is 2 then the last part of the
        > above code will change the value of *(screenp_to + 1) again anyway.
        > It's only the line "*(screenp_to + 2) = 0;" which had any effect, and
        > the effect is to redraw the following character.

        Yes. second occuerance of "*(screenp_to + 1) = 0;" is redundant.
        I just leave it there just for readability (Yes. It is better to comment out!)
        and the line "*(screenp_to + 2) = 0" has two important side effects.
        First, it makes to redraw the following character, and second, it makes
        IsLeadByte() to work correctly.

        --
        박종대
        --
        ``Truth is a matter of the imagination.''
        - "The Left Hand of Darkness", Ursula K. LeGuin -
      • Bram Moolenaar
        ... No, this can also happen when char_bytes is 1. That is the situation where the new character is single-byte, while the currently displayed character at
        Message 3 of 10 , Mar 24, 2000
        • 0 Attachment
          Robert Webb wrote:

          > And Bram replied:
          >
          > > You are missing something: This code checks if the new byte is a
          > > single-byte character while the old byte was a double-byte
          > > character. The next character must be redrawn then, since the right
          > > halve of the double-width character would still be there. The other
          > > situation is when writing a double-width character which overwrites
          > > the left halve of an existing double-width character.
          >
          > OK, so I still don't really understand multi-byte stuff, but the line
          > "*(screenp_to + 1) = 0;" above was redundant, since it only happens
          > when char_bytes is 2,

          No, this can also happen when char_bytes is 1. That is the situation where
          the new character is single-byte, while the currently displayed character at
          this position is double-byte. Thus the left halve of the old double-byte
          character is going to be overwritten by a single-byte character. The right
          halve of the old double-byte character then also needs to be redrawn.

          Your code appears to work correctly anyway, but I'll leave it up to people
          that actually use multi-byte text to check this.

          > Bram also wrote:
          >
          > > I also wonder about the performance, since the function call to
          > > screen_char_needs_redraw() looks a bit expensive (lots of
          > > arguments).
          >
          > Doesn't seem very expensive to me, but if you think it is, the whole
          > function could be inlined again (in two places though), or turned into
          > a #define (can #defines contain #ifdef's in them?!). But I really
          > doubt there'd be much difference. I turned it into a function because
          > the code needed to be called in two places now instead of just one as
          > before (and because it's a lot neater :-)).

          Performance is very important in this location, since display updating happens
          often!

          --
          A bug in the hand is better than one as yet undetected.

          /-/-- Bram Moolenaar --- Bram@... --- http://www.moolenaar.net --\-\
          \-\-- Vim: http://www.vim.org ---- ICCF Holland: http://www.vim.org/iccf --/-/
        • Park Chong-Dae
          Dear vim-dev and vim-multibyte teams, I make another patch based on the Robert s work. I make some code clean up. screen_char_needs_redraw() is becomes
          Message 4 of 10 , Mar 24, 2000
          • 0 Attachment
            Dear vim-dev and vim-multibyte teams,

            I make another patch based on the Robert's work.

            I make some code clean up.

            screen_char_needs_redraw() is becomes lighter.
            (I also changed the function name shorter.)
            And the function has no side effects now. (no char_bytes setting)

            This patch is based on the original vim-5.6 source.
            Please test the newer patch. I've just tested it in the MULTI_BYTE environment
            in console mode(xterm) only. ^^;

            Chong-Dae Park
          • Bram Moolenaar
            ... I didn t check if this patch works properly, but spotted a few things: - is_need_update() has ANSI style declarations. Old compilers can t handle this. -
            Message 5 of 10 , Mar 25, 2000
            • 0 Attachment
              Park Chong-Dae wrote:

              > I make another patch based on the Robert's work.

              I didn't check if this patch works properly, but spotted a few things:

              - is_need_update() has ANSI style declarations. Old compilers can't handle
              this.

              - The last two "if" statements in is_need_update() can be combined.

              - The "endcol = Columns" can be put inside the "if (rlflag)". I wonder of the
              combination of right-left text and multibyte works? Probably doesn't make
              sense anyway.

              - It looks like it's possible that update_next is set for the character just
              right of the screen.

              --
              They now pass three KNIGHTS impaled to a tree. With their feet off the
              ground, with one lance through the lot of them, they are skewered up
              like a barbecue.
              "Monty Python and the Holy Grail" PYTHON (MONTY) PICTURES LTD

              /-/-- Bram Moolenaar --- Bram@... --- http://www.moolenaar.net --\-\
              \-\-- Vim: http://www.vim.org ---- ICCF Holland: http://www.vim.org/iccf --/-/
            • Park Chong-Dae
              ... I fix it. ... I fix it. ... I fix it. Is there a charset that uses right-left and multibyte? I know some codes in MULTI_BYTE have rlflag settings. It could
              Message 6 of 10 , Mar 25, 2000
              • 0 Attachment
                On Sat, Mar 25, 2000 at 12:01:43PM +0100, Bram Moolenaar wrote:
                >
                > Park Chong-Dae wrote:
                >
                > > I make another patch based on the Robert's work.
                >
                > I didn't check if this patch works properly, but spotted a few things:
                >
                > - is_need_update() has ANSI style declarations. Old compilers can't handle
                > this.

                I fix it.

                > - The last two "if" statements in is_need_update() can be combined.

                I fix it.

                > - The "endcol = Columns" can be put inside the "if (rlflag)". I wonder of the
                > combination of right-left text and multibyte works? Probably doesn't make
                > sense anyway.

                I fix it.
                Is there a charset that uses right-left and multibyte?
                I know some codes in MULTI_BYTE have rlflag settings.
                It could be removed. But I don't know about it. You can clean up the code.

                > - It looks like it's possible that update_next is set for the character just
                > right of the screen.

                ??

                This is yet another patch. And I tune up some code for performance and
                safe bound check. (removed a IsTrailByte() call)

                --
                Chong-Dae Park
                -- ' Clarke's Third Law
                "Any sufficiently advanced technology is indistinguishable from magic."
              • Bram Moolenaar
                ... Very good. Now it s up to the others to check out this version of the patch. ... Agree. ... I suppose it could be char_needs_redraw , since it s checking
                Message 7 of 10 , Mar 27, 2000
                • 0 Attachment
                  Robert Webb wrote:

                  > I have attached a new patch, to be applied to the original screen.c,
                  > which includes both of Chong-Dae Park's patches. I have also made the
                  > new fix for bold fonts only take place in the GUI, not in xterm as
                  > well, as per Bram's request.

                  Very good. Now it's up to the others to check out this version of the patch.

                  > I also renamed a few things:
                  > update_this --> redraw_this
                  > update_next --> redraw_next
                  > is_needs_update --> needs_redraw
                  >
                  > I think "redraw" is a more specific description of the action required
                  > than "update", and "needs_redraw" just reads better.

                  Agree.

                  > I still think that function could do with a longer more descriptive name
                  > though. "needs_redraw" is a very general description, but I guess it'll do.

                  I suppose it could be "char_needs_redraw", since it's checking only one
                  character.

                  --
                  FATHER: Did you kill all those guards?
                  LAUNCELOT: Yes ... I'm very sorry ...
                  FATHER: They cost fifty pounds each!
                  "Monty Python and the Holy Grail" PYTHON (MONTY) PICTURES LTD

                  /-/-- Bram Moolenaar --- Bram@... --- http://www.moolenaar.net --\-\
                  \-\-- Vim: http://www.vim.org ---- ICCF Holland: http://www.vim.org/iccf --/-/
                • Robert Webb
                  Hi Bram et al, ... Yep, there were two. They look fine. There was one place with #if MULTI_BYTE instead of #ifdef MULTI_BYTE which I ve fixed. I have
                  Message 8 of 10 , Mar 27, 2000
                  • 0 Attachment
                    Hi Bram et al,

                    > Did you see the patch from Chong-Dae Park? I'll await your comments
                    > before including this.

                    Yep, there were two. They look fine. There was one place with
                    "#if MULTI_BYTE" instead of "#ifdef MULTI_BYTE" which I've fixed.

                    I have attached a new patch, to be applied to the original screen.c,
                    which includes both of Chong-Dae Park's patches. I have also made the
                    new fix for bold fonts only take place in the GUI, not in xterm as
                    well, as per Bram's request.

                    I also renamed a few things:
                    update_this --> redraw_this
                    update_next --> redraw_next
                    is_needs_update --> needs_redraw

                    I think "redraw" is a more specific description of the action required
                    than "update", and "needs_redraw" just reads better. I still think
                    that function could do with a longer more descriptive name though.
                    "needs_redraw" is a very general description, but I guess it'll do.

                    > - It looks like it's possible that update_next is set for the
                    > character just right of the screen.

                    Only if force is set I think, and it still doesn't matter. The only
                    place that the flag is tested for (other than next time around the
                    loop, which won't be reached in this case), is for the new bold trick,
                    in which case it might decide to redraw the current character
                    unnecessarily. But if this only happens when force is set, then it'll
                    get redrawn anyway, so no harm done.

                    Rob.
                  Your message has been successfully submitted and would be delivered to recipients shortly.