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

[vim-multibyte] about your patch (PATCH: Fix for bold characters spilling over left side in GUI)

Expand Messages
  • Park Chong-Dae
    Dear Rob, I heard your patch from Mr. Nam and find it in vim-dev mailing list. (I m not subscribed the mailing list. I just see it via egroups.com) The
    Message 1 of 10 , Mar 22, 2000
    • 0 Attachment
      Dear Rob,

      I heard your patch from Mr. Nam and find it in vim-dev mailing list.
      (I'm not subscribed the mailing list. I just see it via egroups.com)

      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.

      Yes, the original patch which I had made is tricky. But it produce an optimal
      screen update. (I always tested it with "script" program.)
      When making MULTI_BYTE patch, I tried not to modify non MULTI_BYTE code.
      So, my code becomes somewhat hard-to-read.

      To Bram: don't apply Rob's patch until it produce an optimal screen update.

      To Rob: What trouble your patch resolves? I just find just a worse behavior in
      MULTI_BYTE environment.

      Best regards,

      Chong-Dae Park

      @ Cc'ed to "vim-multibyte"
    • 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 2 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 3 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 4 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 --/-/
          • Robert Webb
            ... 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. ... OK,
            Message 5 of 10 , Mar 24, 2000
            • 0 Attachment
              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.

              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.

              I think the attached patch should fix my previous one (must be applied
              after my previous patch).

              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 :-)).

              Rob.
            • 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 6 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 7 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 8 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 9 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 10 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.