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

Re: [patch] [test] test if :mksession saves cursor columns correctly in presence of tab and multibyte characters

Expand Messages
  • Roland Eggner
    Hi Christian, ... Mail header tells you are using mutt? If so, suggestion: • save my previous mail to a file by keys s • cancel mail deletion by
    Message 1 of 16 , Feb 16, 2013
    • 0 Attachment
      Hi Christian,

      On 2013-02-16 Saturday at 23:42 +0100 Christian Brabandt wrote:
      > Thanks for your effort and the patch. Can you please attach the patch? I find
      > it hard to understand what your problem is using copy/paste.

      Mail header tells you are using mutt? If so, suggestion:
      • save my previous mail to a file by keys <Esc>s
      • cancel mail deletion by key u
      • cd your/vim/source/tree/vim-7.3.816
      • patch -p1 -N -u -i path/to/just.saved.patch

      Just simple. No need for complicated copy-paste. patch binary is “smart” and
      skips surrounding text :)

      --
      Regards
      Roland Eggner
    • Christian Brabandt
      Hi Roland! ... Good to know, that patch ignores garbage. I still prefer attachments, though. Mit freundlichen Grüßen Christian -- Arzt zum Beamten: Wieviel
      Message 2 of 16 , Feb 17, 2013
      • 0 Attachment
        Hi Roland!

        On So, 17 Feb 2013, Roland Eggner wrote:

        > Mail header tells you are using mutt? If so, suggestion:
        > • save my previous mail to a file by keys <Esc>s
        > • cancel mail deletion by key u
        > • cd your/vim/source/tree/vim-7.3.816
        > • patch -p1 -N -u -i path/to/just.saved.patch
        >
        > Just simple. No need for complicated copy-paste. patch binary is “smart” and
        > skips surrounding text :)

        Good to know, that patch ignores garbage. I still prefer attachments,
        though.

        Mit freundlichen Grüßen
        Christian
        --
        Arzt zum Beamten: Wieviel Stunden schlafen Sie täglich?" - Höchstens
        zwei." - Das ist aber entschieden zu wenig!" - Finde ich nicht. Ich schlafe
        nachts noch mal ungefähr zehn Stunden."

        --
        --
        You received this message from the "vim_dev" 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

        ---
        You received this message because you are subscribed to the Google Groups "vim_dev" group.
        To unsubscribe from this group and stop receiving emails from it, send an email to vim_dev+unsubscribe@....
        For more options, visit https://groups.google.com/groups/opt_out.
      • Christian Brabandt
        Hi Roland! ... I see the error. The problem is, curwin- w_cursor.col is the byteindex in the buffer and not the screen-index (which is by what l moves) I
        Message 3 of 16 , Feb 17, 2013
        • 0 Attachment
          Hi Roland!

          On Sa, 16 Feb 2013, Roland Eggner wrote:

          > On 2013-02-06 Wednesday at 23:08 +0100 Bram Moolenaar wrote:
          > > Roland Eggner wrote:
          > > > > > :mksession writes wrong column number of cursor position in presence of
          > > > > > multibyte characters.
          > > > > >
          > > > > > I have been using vim-7.3.135 with this patch applied for several
          > > > > > months … it works for me:
          > > > >
          > > > > I do not see the problem. How to reproduce?
          > > > >
          > > > > Your patch can't be right, the "l" command moves over characters, not
          > > > > columns.
          > > >
          > > > Session files created with :mksession restore cursor line and column,
          > > > as long as there is no multibyte character between start of line and
          > > > cursor. Otherwise, without my patch restored column is off by the
          > > > difference between character position and byte position counted from
          > > > start of line.
          > > >
          > > > With my patch applied, cursor line and column is restored correctly,
          > > > with and without multibyte characters, with and without changing of
          > > > options fileencoding or binary, even with files preprocessed by
          > > > BufRead autocommands gzip -dc, bzip -dc, xz -dc, pdftotext, elinks
          > > > -dump, antiword, … which I find pretty cool, use and enjoy it nearly
          > > > every day.
          > >
          > > I asked how to reproduce. I suspect your 'encoding' matters.
          > > Please start with "vim -u NONE" and check what the default value of
          > > 'encoding' is then.
          > >
          > > Note that your patch most likely is wrong when there is a Tab before the
          > > cursor, try that.
          >
          > Indeed, you are right: my patch beeing wrong was hidden by autocommands
          > restoring cursor positions based on textmarks.
          >
          > For investigation of the problem I have created test{91,92}.
          > In my environment (TERM=linux, unicode-mode) and with locale
          > en_US.ISO-8859-1 both succeed, whereas with locale en_US.utf8 both fail:
          >
          >
          > pushd vim-7.3.816
          > patch -p1 -N -u -i my.patch.provided.below
          > pushd src/testdir
          > rm -f test{91,92}.{out,failed,messages}
          > env {LANG,LC_CTYPE,LC_MESSAGES}=en_US.ISO-8859-1 make -j1
          >
          > Test results:
          > ALL DONE
          >
          > grep '[=.]' test91.messages
          > LANG=en_US.ISO-8859-1
          > LC_MESSAGES=en_US.ISO-8859-1
          > LC_ALL=
          > test91.in
          > fileencoding=utf-8
          > termencoding=
          > encoding=latin1
          >
          > grep '[=.]' test92.messages
          > LANG=en_US.ISO-8859-1
          > LC_MESSAGES=en_US.ISO-8859-1
          > LC_ALL=
          > test92.in
          > fileencoding=latin1
          > termencoding=
          > encoding=latin1
          >
          >
          > rm -f test{91,92}.{out,failed,messages}
          > env {LANG,LC_CTYPE,LC_MESSAGES}=en_US.utf8 make -j1
          >
          > Test results:
          > test91 FAILED
          > test92 FAILED

          I see the error. The problem is, curwin->w_cursor.col is the byteindex
          in the buffer and not the screen-index (which is by what 'l' moves)

          I think, the attached patch fixes it.

          Mit freundlichen Grüßen
          Christian

          --
          --
          You received this message from the "vim_dev" 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

          ---
          You received this message because you are subscribed to the Google Groups "vim_dev" group.
          To unsubscribe from this group and stop receiving emails from it, send an email to vim_dev+unsubscribe@....
          For more options, visit https://groups.google.com/groups/opt_out.
        • Bram Moolenaar
          ... I think using mb_string2cells() is wrong, because it counts double-wide characters as two, while l counts them as one. How about using w_cursor.vcol but
          Message 4 of 16 , Feb 17, 2013
          • 0 Attachment
            Christian Brabandt wrote:

            > Hi Roland!
            >
            > On Sa, 16 Feb 2013, Roland Eggner wrote:
            >
            > > On 2013-02-06 Wednesday at 23:08 +0100 Bram Moolenaar wrote:
            > > > Roland Eggner wrote:
            > > > > > > :mksession writes wrong column number of cursor position in presence of
            > > > > > > multibyte characters.
            > > > > > >
            > > > > > > I have been using vim-7.3.135 with this patch applied for several
            > > > > > > months … it works for me:
            > > > > >
            > > > > > I do not see the problem. How to reproduce?
            > > > > >
            > > > > > Your patch can't be right, the "l" command moves over characters, not
            > > > > > columns.
            > > > >
            > > > > Session files created with :mksession restore cursor line and column,
            > > > > as long as there is no multibyte character between start of line and
            > > > > cursor. Otherwise, without my patch restored column is off by the
            > > > > difference between character position and byte position counted from
            > > > > start of line.
            > > > >
            > > > > With my patch applied, cursor line and column is restored correctly,
            > > > > with and without multibyte characters, with and without changing of
            > > > > options fileencoding or binary, even with files preprocessed by
            > > > > BufRead autocommands gzip -dc, bzip -dc, xz -dc, pdftotext, elinks
            > > > > -dump, antiword, … which I find pretty cool, use and enjoy it nearly
            > > > > every day.
            > > >
            > > > I asked how to reproduce. I suspect your 'encoding' matters.
            > > > Please start with "vim -u NONE" and check what the default value of
            > > > 'encoding' is then.
            > > >
            > > > Note that your patch most likely is wrong when there is a Tab before the
            > > > cursor, try that.
            > >
            > > Indeed, you are right: my patch beeing wrong was hidden by autocommands
            > > restoring cursor positions based on textmarks.
            > >
            > > For investigation of the problem I have created test{91,92}.
            > > In my environment (TERM=linux, unicode-mode) and with locale
            > > en_US.ISO-8859-1 both succeed, whereas with locale en_US.utf8 both fail:
            > >
            > >
            > > pushd vim-7.3.816
            > > patch -p1 -N -u -i my.patch.provided.below
            > > pushd src/testdir
            > > rm -f test{91,92}.{out,failed,messages}
            > > env {LANG,LC_CTYPE,LC_MESSAGES}=en_US.ISO-8859-1 make -j1
            > >
            > > Test results:
            > > ALL DONE
            > >
            > > grep '[=.]' test91.messages
            > > LANG=en_US.ISO-8859-1
            > > LC_MESSAGES=en_US.ISO-8859-1
            > > LC_ALL=
            > > test91.in
            > > fileencoding=utf-8
            > > termencoding=
            > > encoding=latin1
            > >
            > > grep '[=.]' test92.messages
            > > LANG=en_US.ISO-8859-1
            > > LC_MESSAGES=en_US.ISO-8859-1
            > > LC_ALL=
            > > test92.in
            > > fileencoding=latin1
            > > termencoding=
            > > encoding=latin1
            > >
            > >
            > > rm -f test{91,92}.{out,failed,messages}
            > > env {LANG,LC_CTYPE,LC_MESSAGES}=en_US.utf8 make -j1
            > >
            > > Test results:
            > > test91 FAILED
            > > test92 FAILED
            >
            > I see the error. The problem is, curwin->w_cursor.col is the byteindex
            > in the buffer and not the screen-index (which is by what 'l' moves)
            >
            > I think, the attached patch fixes it.

            I think using mb_string2cells() is wrong, because it counts double-wide
            characters as two, while "l" counts them as one.

            How about using w_cursor.vcol but move with "|" instead of "l"?
            Then no conversions are required.


            --
            "I've been teaching myself to play the piano for about 5 years and now write
            most of my songs on it, mainly because I can never find any paper."
            Jeff Lynne, ELO's greatest hits

            /// Bram Moolenaar -- Bram@... -- http://www.Moolenaar.net \\\
            /// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
            \\\ an exciting new programming language -- http://www.Zimbu.org ///
            \\\ help me help AIDS victims -- http://ICCF-Holland.org ///

            --
            --
            You received this message from the "vim_dev" 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

            ---
            You received this message because you are subscribed to the Google Groups "vim_dev" group.
            To unsubscribe from this group and stop receiving emails from it, send an email to vim_dev+unsubscribe@....
            For more options, visit https://groups.google.com/groups/opt_out.
          • Christian Brabandt
            Hi Bram! ... Oh, I didn t know that. ... You mean w_cursor.col and using | instead of l . That won t help here, I am afraid. If we have a multi-byte built,
            Message 5 of 16 , Feb 17, 2013
            • 0 Attachment
              Hi Bram!

              On So, 17 Feb 2013, Bram Moolenaar wrote:

              > I think using mb_string2cells() is wrong, because it counts double-wide
              > characters as two, while "l" counts them as one.

              Oh, I didn't know that.

              > How about using w_cursor.vcol but move with "|" instead of "l"?

              You mean w_cursor.col and using '|' instead of 'l'. That won't help
              here, I am afraid.

              If we have a multi-byte built, we also have eval feature, right?
              So can't we use something like this:

              norm! 06l
              if 1
              call setpos('.', ....)
              endif

              Since getpos() or winsaveview uses the byte-value of w_cursor.col this
              should work.


              regards,
              Christian
              --

              --
              --
              You received this message from the "vim_dev" 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

              ---
              You received this message because you are subscribed to the Google Groups "vim_dev" group.
              To unsubscribe from this group and stop receiving emails from it, send an email to vim_dev+unsubscribe@....
              For more options, visit https://groups.google.com/groups/opt_out.
            • Roland Eggner
              ... The idea using setpos() looks promising. Two patches in attachments, the first implements setpos() in src/ex_docmd.c, the second adds adapted v2 of my
              Message 6 of 16 , Feb 18, 2013
              • 0 Attachment
                On 2013-02-17 Sunday at 22:15 +0100 Christian Brabandt wrote:
                > On So, 17 Feb 2013, Bram Moolenaar wrote:
                >
                > > I think using mb_string2cells() is wrong, because it counts double-wide
                > > characters as two, while "l" counts them as one.
                >
                > Oh, I didn't know that.
                >
                > > How about using w_cursor.vcol but move with "|" instead of "l"?
                >
                > You mean w_cursor.col and using '|' instead of 'l'. That won't help
                > here, I am afraid.
                >
                > If we have a multi-byte built, we also have eval feature, right?
                > So can't we use something like this:
                >
                > norm! 06l
                > if 1
                > call setpos('.', ....)
                > endif
                >
                > Since getpos() or winsaveview uses the byte-value of w_cursor.col this
                > should work.

                The idea using setpos() looks promising. Two patches in attachments, the first
                implements setpos() in src/ex_docmd.c, the second adds adapted v2 of my
                test{91,92} scripts. In C locale and in en_US.ISO-8859-1 locale both tests
                succeed here, but in en_US.utf8 locale test91 fails and only test92 succeeds:

                pushd path/to/sourcetree/vim-7.3.822
                patch -N -p1 -u -i attachment1.patch
                patch -N -p1 -u -i attachment2.patch
                pushd src/testdir
                rm -f test{91,92}.{out,failed,messages}
                env {LANG,LC_CTYPE,LC_MESSAGES}=en_US.utf8 make -j1
                > Test results:
                > test91 FAILED

                diff -u test91.{failed,ok}
                > --- test91.failed 2013-02-18 19:10:56.448951101 +0100
                > +++ test91.ok 2013-02-18 19:07:26.238965475 +0100
                > @@ -1,6 +1,6 @@
                > call setpos('.', [0, line('.'), 16, 0])
                > call setpos('.', [0, line('.'), 16, 0])
                > call setpos('.', [0, line('.'), 16, 0])
                > -call setpos('.', [0, line('.'), 15, 0])
                > +call setpos('.', [0, line('.'), 16, 0])
                > call setpos('.', [0, line('.'), 16, 0])
                > call setpos('.', [0, line('.'), 16, 0])

                grep '[=.]' test91.messages
                > LANG=en_US.utf8
                > LC_MESSAGES=en_US.utf8
                > LC_ALL=
                > test91.in
                > fileencoding=utf-8
                > termencoding=
                > encoding=utf-8

                --
                Regards
                Roland Eggner
              • Bram Moolenaar
                ... No, that doesn t work. I meant w_virtcol. It has to be validated before using it. Might be tricky in other windows than the current window. ... It is
                Message 7 of 16 , Feb 18, 2013
                • 0 Attachment
                  Christian Brabandt wrote:

                  > On So, 17 Feb 2013, Bram Moolenaar wrote:
                  >
                  > > I think using mb_string2cells() is wrong, because it counts double-wide
                  > > characters as two, while "l" counts them as one.
                  >
                  > Oh, I didn't know that.
                  >
                  > > How about using w_cursor.vcol but move with "|" instead of "l"?
                  >
                  > You mean w_cursor.col and using '|' instead of 'l'. That won't help
                  > here, I am afraid.

                  No, that doesn't work. I meant w_virtcol. It has to be validated
                  before using it. Might be tricky in other windows than the current
                  window.

                  > If we have a multi-byte built, we also have eval feature, right?
                  > So can't we use something like this:
                  >
                  > norm! 06l
                  > if 1
                  > call setpos('.', ....)
                  > endif
                  >
                  > Since getpos() or winsaveview uses the byte-value of w_cursor.col this
                  > should work.

                  It is possible to have multi-byte without eval.

                  --
                  "Lisp has all the visual appeal of oatmeal with nail clippings thrown in."
                  -- Larry Wall

                  /// Bram Moolenaar -- Bram@... -- http://www.Moolenaar.net \\\
                  /// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
                  \\\ an exciting new programming language -- http://www.Zimbu.org ///
                  \\\ help me help AIDS victims -- http://ICCF-Holland.org ///

                  --
                  --
                  You received this message from the "vim_dev" 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

                  ---
                  You received this message because you are subscribed to the Google Groups "vim_dev" group.
                  To unsubscribe from this group and stop receiving emails from it, send an email to vim_dev+unsubscribe@....
                  For more options, visit https://groups.google.com/groups/opt_out.
                • Christian Brabandt
                  Hi Bram! ... Hm, that was what the original patch was all about I think? If I know how to validate it, I can make a patch, using the already suggested test.
                  Message 8 of 16 , Feb 18, 2013
                  • 0 Attachment
                    Hi Bram!

                    On Mo, 18 Feb 2013, Bram Moolenaar wrote:

                    >
                    > Christian Brabandt wrote:
                    >
                    > > On So, 17 Feb 2013, Bram Moolenaar wrote:
                    > >
                    > > > I think using mb_string2cells() is wrong, because it counts double-wide
                    > > > characters as two, while "l" counts them as one.
                    > >
                    > > Oh, I didn't know that.
                    > >
                    > > > How about using w_cursor.vcol but move with "|" instead of "l"?
                    > >
                    > > You mean w_cursor.col and using '|' instead of 'l'. That won't help
                    > > here, I am afraid.
                    >
                    > No, that doesn't work. I meant w_virtcol. It has to be validated
                    > before using it. Might be tricky in other windows than the current
                    > window.

                    Hm, that was what the original patch was all about I think? If I know
                    how to validate it, I can make a patch, using the already suggested
                    test.


                    regards,
                    Christian
                    --
                    Wenn Sie können, schieben sie diese Aufgabe (IT-Sicherheitschef) an
                    einen Kollegen ab. Wenn nicht, arbeiten Sie professionell.
                    -- Robert Niedermeier (Rechtsanwalt)

                    --
                    --
                    You received this message from the "vim_dev" 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

                    ---
                    You received this message because you are subscribed to the Google Groups "vim_dev" group.
                    To unsubscribe from this group and stop receiving emails from it, send an email to vim_dev+unsubscribe@....
                    For more options, visit https://groups.google.com/groups/opt_out.
                  • Roland Eggner
                    Hi Bram and Christian! ... test91 v2 failed under certain conditions, because vim erroneously changed encoding and damaged file “test91.in”. Here is
                    Message 9 of 16 , Feb 18, 2013
                    • 0 Attachment
                      Hi Bram and Christian!

                      On 2013-02-18 Monday at 23:09 +0100 Christian Brabandt wrote:
                      > On Mo, 18 Feb 2013, Bram Moolenaar wrote:
                      >
                      > >
                      > > Christian Brabandt wrote:
                      > >
                      > > > On So, 17 Feb 2013, Bram Moolenaar wrote:
                      > > >
                      > > > > I think using mb_string2cells() is wrong, because it counts double-wide
                      > > > > characters as two, while "l" counts them as one.
                      > > >
                      > > > Oh, I didn't know that.
                      > > >
                      > > > > How about using w_cursor.vcol but move with "|" instead of "l"?
                      > > >
                      > > > You mean w_cursor.col and using '|' instead of 'l'. That won't help
                      > > > here, I am afraid.
                      > >
                      > > No, that doesn't work. I meant w_virtcol. It has to be validated
                      > > before using it. Might be tricky in other windows than the current
                      > > window.
                      >
                      > Hm, that was what the original patch was all about I think? If I know
                      > how to validate it, I can make a patch, using the already suggested
                      > test.

                      test91 v2 failed under certain conditions, because vim erroneously changed
                      encoding and damaged file “test91.in”.

                      Here is test{91,92} v3.
                      Now I can report: My patch for “src/ex_docmd.c” works for me (tm) :)
                      I tried the following and all tests succeeded:

                      pushd vim-7.3.822
                      patch -p1 -N -u -i 'vim-7.3.816 src ex_docmd.c … .patch'
                      patch -p1 -N -u -i 'vim-7.3.816 src testdir test91 test92 v3 … .patch'
                      pushd src/testdir
                      make clean
                      env {LANG,LC_CTYPE,LC_MESSAGES}=C make -j1
                      make clean
                      env {LANG,LC_CTYPE,LC_MESSAGES}=en_US.ISO-8859-1 make -j1
                      make clean
                      env {LANG,LC_CTYPE,LC_MESSAGES}=en_US.utf8 make -j1

                      Bram, if you wish the lines “debugging stuff” removed from test{91,92}.in or any
                      other “final polish” … just let me know.

                      --
                      Regards
                      Roland Eggner
                    Your message has been successfully submitted and would be delivered to recipients shortly.