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

Re: [PATCH] IM support with ATSUI rendering

Expand Messages
  • björn
    Hi Kaoru,Thanks for the patch(es). I ve looked at the last one and it is starting to look like it could be merged soon. I did some cleanup to your patch
    Message 1 of 16 , Sep 3, 2008
      Hi Kaoru,

      Thanks for the patch(es). I've looked at the last one and it is
      starting to look like it could be merged soon. I did some cleanup to
      your patch and am attaching it to this post (my cleanup is in a second
      commit on top of yours so that you may easily see what I have
      changed).

      2008/9/3 Kaoru YOSHIDA <to.kaoru@...>:
      >>
      >> Could you explain why would you choose 'fontWide' for marked
      >> text in NSAttributedString and 'font' for NSString? Because
      >> AFAIK, a marked text can contain both CJK characters *and*
      >> Latin characters, say, I'm composing a Chinese word "如果",
      >> then in certain state the marked text will be "如guo". In
      >> that case, choosing 'fontWide' will not be appropriate since
      >> normally we don't want to use 'fontWide' for Latin characters
      >> "guo".
      >
      > This is a result of my trial-and-error hacking. The motivation
      > to do so is to draw IM characters properly when the length of IM
      > characters is longer than single line. Say if your window has 80
      > single byte character width for each line, and you start typing
      > IM characters at 77th column. In such a case, the third character
      > should appear at the next line as long as you type only two-byte
      > characters, right? To achieve this, I think an IM character should
      > have the same character width of a character which is drawn by
      > 'native' MacVim. Based on this motivation, I introduced 'fontWide'.
      > So, this is somehow related to what you pointed out past.
      >
      > My open question is why native and IM characters have different
      > character width. If both has the same width, we don't need to
      > be bothered this...

      I think there is some kind of misunderstanding here (perhaps on my
      part) so I'll try to clarify.

      There are two different types of characters in Vim: "normal" and
      "wide". The former take up one column whereas the latter take up two.
      This distinction is in Vim and has nothing to do with IM. In your IM
      code you have the check Jiang pointed out which determines if a whole
      string of characters (the marked text) is wide or not by checking if
      the string is of type NSAttributedString or NSString. This test, as
      Jiang points out, is not correct when there are both wide and normal
      characters in the marked text. The only way I can think of how this
      (and other related problems in the IM code) could be fixed is to bring
      over the utf_char2cells() function from Vim and use that to determine
      which characters are wide or not.

      >>> One big remaining implementation is -firstRectForCharacterRange in
      >>> MMAtsuiTextView. Since I couldn't figure out a way to implement this,
      >>> I really appreciate if anyone can do it.
      >>
      >> I think you can use preEdit{Row,Column} to calculate the rect.
      >> In vim-cocoa, I used the following code:
      >
      > Great!
      > I implemented new code based on yours. Since I would like to show
      > a pop-up window more precious position, I added a little hack to
      > check which IM characters are selected.

      That looks better, but perhaps you can "merge" the MMTextView and
      MMAtsuiTextView code and put it in the helper class so that both ATSUI
      and the default renderer may benefit from the better placement of the
      IM popup window? (You'll have to wrap the
      boundingRectForCharacterAtRow:column: somehow to do this. You may
      want to make the convertRow:column:toPoint: method public and
      implement a similar one in ATSUI and then call this from the helper
      and get rid of the boundingRectFor... method.) In my attached patch I
      did something similar for setMarkedText: but I ran out of time for now
      so maybe you can complete that part?

      I've noticed that there are still some IM aspects that are missing in
      ATSUI; e.g. there is no insertion point when editing marked text. If
      you find the time it would be great if you could make the two
      renderers have the same features. :-)

      Thanks for your work,
      Björn

      --~--~---------~--~----~------------~-------~--~----~
      You received this message from the "vim_mac" maillist.
      For more information, visit http://www.vim.org/maillist.php
      -~----------~----~----~----~------~----~------~--~---
    • Kaoru YOSHIDA
      Hi Björn, Sorry for late reply. I was kinda busy these days... ... I merged firstRectForCharacterRange: method into MMTextViewHelper class. In
      Message 2 of 16 , Sep 8, 2008
        Hi Björn,

        Sorry for late reply. I was kinda busy these days...

        On 2008/09/04, at 4:26, björn wrote:

        >

        > That looks better, but perhaps you can "merge" the MMTextView and
        > MMAtsuiTextView code and put it in the helper class so that both ATSUI
        > and the default renderer may benefit from the better placement of the
        > IM popup window? (You'll have to wrap the
        > boundingRectForCharacterAtRow:column: somehow to do this. You may
        > want to make the convertRow:column:toPoint: method public and
        > implement a similar one in ATSUI and then call this from the helper
        > and get rid of the boundingRectFor... method.) In my attached patch I
        > did something similar for setMarkedText: but I ran out of time for now
        > so maybe you can complete that part?

        I merged firstRectForCharacterRange: method into MMTextViewHelper
        class. In MMTextViewHelper, I wrote the following code to check
        whether textView is MMTextView or MMAtsuiTextView. Since coordinate
        axes of MMTextView and MMAtsuiTextView is different, I need to check
        it.

        + if (![textView isKindOfClass:[MMTextView class]]) {
        + row = [textView maxRows] - row;
        + on = -1;
        + }

        This may not be a good idea to do this in MMTextViewHelper. If not
        appropriate, we should define a new function in MMAtsuiTextView
        and call it from MMTextViewHelper. Since no appropriate function
        name comes up to my mind, I didn't implement that way.
        The patch file is 0003-XXXX.

        > I've noticed that there are still some IM aspects that are missing in
        > ATSUI; e.g. there is no insertion point when editing marked text. If
        > you find the time it would be great if you could make the two
        > renderers have the same features. :-)

        I implemented this part in 0004-XXXX.
        While I use drawInsertionPointAtRow: method to draw the insertion
        point, the arguments are hard coded. If there's a smarter way,
        could you let me know?

        The patch 0005-XXX is to fix a drawing issue of Japanese/Chinese
        characters with ATSUI rendering. So, the problem is when shifting
        down those characters one or more lines, small black dots show up.
        I don't know this is a proper way to fix it, but this fixes the
        issue at least in my environment.

        0001-XXX and 0002-XXX are patches you posted.

        Thanks,

        -- kaoru



        --~--~---------~--~----~------------~-------~--~----~
        You received this message from the "vim_mac" maillist.
        For more information, visit http://www.vim.org/maillist.php
        -~----------~----~----~----~------~----~------~--~---
      • Kaoru YOSHIDA
        Hi Björn, Sorry for contiguous posts, but my last patches aren t compilable. The attached one should fix the issue. Thanks, -- kaoru ...
        Message 3 of 16 , Sep 9, 2008
          Hi Björn,

          Sorry for contiguous posts, but my last patches aren't
          compilable. The attached one should fix the issue.

          Thanks,

          -- kaoru

          On 2008/09/09, at 11:45, Kaoru YOSHIDA wrote:

          > Hi Björn,
          >
          > Sorry for late reply. I was kinda busy these days...
          >
          > On 2008/09/04, at 4:26, björn wrote:
          >
          >>
          >
          >> That looks better, but perhaps you can "merge" the MMTextView and
          >> MMAtsuiTextView code and put it in the helper class so that both
          >> ATSUI
          >> and the default renderer may benefit from the better placement of the
          >> IM popup window? (You'll have to wrap the
          >> boundingRectForCharacterAtRow:column: somehow to do this. You may
          >> want to make the convertRow:column:toPoint: method public and
          >> implement a similar one in ATSUI and then call this from the helper
          >> and get rid of the boundingRectFor... method.) In my attached
          >> patch I
          >> did something similar for setMarkedText: but I ran out of time for
          >> now
          >> so maybe you can complete that part?
          >
          > I merged firstRectForCharacterRange: method into MMTextViewHelper
          > class. In MMTextViewHelper, I wrote the following code to check
          > whether textView is MMTextView or MMAtsuiTextView. Since coordinate
          > axes of MMTextView and MMAtsuiTextView is different, I need to check
          > it.
          >
          > + if (![textView isKindOfClass:[MMTextView class]]) {
          > + row = [textView maxRows] - row;
          > + on = -1;
          > + }
          >
          > This may not be a good idea to do this in MMTextViewHelper. If not
          > appropriate, we should define a new function in MMAtsuiTextView
          > and call it from MMTextViewHelper. Since no appropriate function
          > name comes up to my mind, I didn't implement that way.
          > The patch file is 0003-XXXX.
          >
          >> I've noticed that there are still some IM aspects that are missing in
          >> ATSUI; e.g. there is no insertion point when editing marked text. If
          >> you find the time it would be great if you could make the two
          >> renderers have the same features. :-)
          >
          > I implemented this part in 0004-XXXX.
          > While I use drawInsertionPointAtRow: method to draw the insertion
          > point, the arguments are hard coded. If there's a smarter way,
          > could you let me know?
          >
          > The patch 0005-XXX is to fix a drawing issue of Japanese/Chinese
          > characters with ATSUI rendering. So, the problem is when shifting
          > down those characters one or more lines, small black dots show up.
          > I don't know this is a proper way to fix it, but this fixes the
          > issue at least in my environment.
          >
          > 0001-XXX and 0002-XXX are patches you posted.
          >
          > Thanks,
          >
          > -- kaoru
          >
          >
          > <IM080909.tar.bz2>


          --~--~---------~--~----~------------~-------~--~----~
          You received this message from the "vim_mac" maillist.
          For more information, visit http://www.vim.org/maillist.php
          -~----------~----~----~----~------~----~------~--~---
        • björn
          Hi Kaoru, ... Ok, that looks a bit weird so I fixed it. Instead of manually converting from (rows,columns) to a pixel value I now supply the methods: -
          Message 4 of 16 , Sep 9, 2008
            Hi Kaoru,

            2008/9/9 Kaoru YOSHIDA <to.kaoru@...>:
            >
            > I merged firstRectForCharacterRange: method into MMTextViewHelper
            > class. In MMTextViewHelper, I wrote the following code to check
            > whether textView is MMTextView or MMAtsuiTextView. Since coordinate
            > axes of MMTextView and MMAtsuiTextView is different, I need to check
            > it.
            >
            > + if (![textView isKindOfClass:[MMTextView class]]) {
            > + row = [textView maxRows] - row;
            > + on = -1;
            > + }
            >
            > This may not be a good idea to do this in MMTextViewHelper. If not
            > appropriate, we should define a new function in MMAtsuiTextView
            > and call it from MMTextViewHelper. Since no appropriate function
            > name comes up to my mind, I didn't implement that way.
            > The patch file is 0003-XXXX.

            Ok, that looks a bit weird so I fixed it. Instead of "manually"
            converting from (rows,columns) to a pixel value I now supply the
            methods:

            - (NSPoint)pointForRow:(int)row column:(int)col;
            - (NSRect)rectForRow:(int)row column:(int)col numRows:(int)nr
            numColumns:(int)nc;

            I amended your patch number 0003 using those methods instead so that
            the helper does not need to check if the text view is ATSUI or Cocoa
            (attached).

            While I was at it I fixed a minor detail in patch number 0002 and the
            amended patch is also in the attached archive.

            >
            >> I've noticed that there are still some IM aspects that are missing in
            >> ATSUI; e.g. there is no insertion point when editing marked text. If
            >> you find the time it would be great if you could make the two
            >> renderers have the same features. :-)
            >
            > I implemented this part in 0004-XXXX.
            > While I use drawInsertionPointAtRow: method to draw the insertion
            > point, the arguments are hard coded. If there's a smarter way,
            > could you let me know?

            I've run out of time now, but I'll check it tomorrow probably. The
            color for the cursor could be taken from defaultForegroundColor I
            suppose, but as for the shape and percentage I guess you just have to
            hard-code it.

            > The patch 0005-XXX is to fix a drawing issue of Japanese/Chinese
            > characters with ATSUI rendering. So, the problem is when shifting
            > down those characters one or more lines, small black dots show up.
            > I don't know this is a proper way to fix it, but this fixes the
            > issue at least in my environment.

            That looks a bit weird, but I haven't had a time to check. What
            happens if you change the text inset to something big like 10 pixels?
            Isn't the text going to be drawn in the wrong place then? You can try
            by typing the following line in Terminal:

            defaults write org.vim.MacVim MMTextInsetTop 10

            and restart. When you're done, enter the following in Terminal:

            defaults delete org.vim.MacVim MMTextInsetTop

            The patches I've attached replaces the corresponding ones in your
            archive...so you'll still need to apply your patch 0001 before the two
            I attach.

            I'll get back to you when I've had a chance to look at patch 0004 and 0005.

            Thanks,
            Björn

            --~--~---------~--~----~------------~-------~--~----~
            You received this message from the "vim_mac" maillist.
            For more information, visit http://www.vim.org/maillist.php
            -~----------~----~----~----~------~----~------~--~---
          • björn
            ... I have updated this patch a bit so that the color is now the proper cursor color (and not the foreground color as I suggested above). It is attached as an
            Message 5 of 16 , Sep 10, 2008
              2008/9/9 björn <bjorn.winckler@...>:
              >>
              >>> I've noticed that there are still some IM aspects that are missing in
              >>> ATSUI; e.g. there is no insertion point when editing marked text. If
              >>> you find the time it would be great if you could make the two
              >>> renderers have the same features. :-)
              >>
              >> I implemented this part in 0004-XXXX.
              >> While I use drawInsertionPointAtRow: method to draw the insertion
              >> point, the arguments are hard coded. If there's a smarter way,
              >> could you let me know?
              >
              > I've run out of time now, but I'll check it tomorrow probably. The
              > color for the cursor could be taken from defaultForegroundColor I
              > suppose, but as for the shape and percentage I guess you just have to
              > hard-code it.

              I have updated this patch a bit so that the color is now the proper
              cursor color (and not the foreground color as I suggested above). It
              is attached as an archive.

              >> The patch 0005-XXX is to fix a drawing issue of Japanese/Chinese
              >> characters with ATSUI rendering. So, the problem is when shifting
              >> down those characters one or more lines, small black dots show up.
              >> I don't know this is a proper way to fix it, but this fixes the
              >> issue at least in my environment.
              >
              > That looks a bit weird, but I haven't had a time to check. What
              > happens if you change the text inset to something big like 10 pixels?
              > Isn't the text going to be drawn in the wrong place then?

              This patch is indeed not working properly. You can't add the text
              inset to the drawing position in that way. What is the problem that
              you were trying to fix exactly? (A screenshot might help.)

              There are still some problems that I have noticed.

              1. The IM "popup" window is often too far to the right: see the
              attached screenshots

              2. The spacing of roman letters is sometimes wide when it should be
              normal as is also exhibited in the screenshot. This is the issue
              Jiang brought up earlier and which I've commented on twice before.

              I realize these issues are a bit hard to deal with because at the
              moment there simply isn't enough information to "get it right". If
              you find the time, maybe have a look at duplicating some of the code
              utf_char2cells() inside mbyte.c. Using such a function you would be
              able to decide whether to use the wide or normal font and that would
              potentially solve both of the above issues, but it would also
              complicate your existing code quite a bit. Since I don't use IM I
              can't really judge whether it is "worth it" to implement this just see
              this as an idea and decide for yourself if it makes sense.

              Finally, please look over the modified patched I've posted here and in
              my previous post (numbers 2, 3 and 4) and let me know if they look ok
              to you and I can start merging.

              Thanks,
              Björn

              --~--~---------~--~----~------------~-------~--~----~
              You received this message from the "vim_mac" maillist.
              For more information, visit http://www.vim.org/maillist.php
              -~----------~----~----~----~------~----~------~--~---
            • björn
              ... Sorry, I forgot the screenshots. I m attaching them to this post. Björn --~--~---------~--~----~------------~-------~--~----~ You received this message
              Message 6 of 16 , Sep 10, 2008
                > There are still some problems that I have noticed.
                >
                > 1. The IM "popup" window is often too far to the right: see the
                > attached screenshots
                >
                > 2. The spacing of roman letters is sometimes wide when it should be
                > normal as is also exhibited in the screenshot. This is the issue
                > Jiang brought up earlier and which I've commented on twice before.

                Sorry, I forgot the screenshots. I'm attaching them to this post.

                Björn

                --~--~---------~--~----~------------~-------~--~----~
                You received this message from the "vim_mac" maillist.
                For more information, visit http://www.vim.org/maillist.php
                -~----------~----~----~----~------~----~------~--~---
              Your message has been successfully submitted and would be delivered to recipients shortly.