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