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! 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. regards,
    Message 1 of 16 , Feb 16, 2013
    • 0 Attachment
      Hi Roland!

      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.


      regards,
      Christian
      --
      Eine freie Seele, wie die seine, kommt in Gefahr, frech zu
      werden, wenn nicht ein edles Wohlwollen das sittliche Gleichgewicht
      herstellt.
      -- Goethe, Maximen und Reflektionen, Nr. 190

      --
      --
      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 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 2 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 3 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 4 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 5 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 6 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 7 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 8 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 9 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 10 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.