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

Re: [patch] Titles for quickfix / location list windows

Expand Messages
  • Lech Lorens
    ... Done. ... As a matter of fact, such behaviour is guaranteed by the C standard (C89 AFAIR). However, my feeling was that Vim s source code does not rely on
    Message 1 of 18 , Mar 28, 2009
      24-03-2009 Dominique Pelle <dominique.pelle@...>:
      >
      > Some remarks (besides what Andreas already mentioned):
      >
      > STRNCPY(NameBuff, bname, MAXPATHL);
      >
      > ... could be written safer as:
      >
      > vim_strncpy(NameBuff, bname, MAXPATHL - 1);

      Done.

      > 2/ I see several times things like this:
      >
      > if (bname)
      > vim_free(bname);
      >
      > The test for NULL pointer can safely be removed, i.e. just write:
      >
      > vim_free(bname);
      >
      > ... since vim_free(...) takes care of testing for NULL pointer.
      >
      > Even standard C function free() do nothing if pointer is NULL
      > by the way (at least on Linux, not sure how portable this is).
      > Quoting "man 3 free":
      >
      > "If ptr is NULL, no operation is performed."

      As a matter of fact, such behaviour is guaranteed by the C standard (C89
      AFAIR). However, my feeling was that Vim's source code does not rely on
      any compiler being standard compliant. Plus I did not have a look at
      what's inside vim_free(). My fault.

      Thank you for your suggestions + the documentation patch.

      Waiting for more comments.

      --
      Cheers,
      Lech

      --~--~---------~--~----~------------~-------~--~----~
      You received this message from the "vim_dev" maillist.
      For more information, visit http://www.vim.org/maillist.php
      -~----------~----~----~----~------~----~------~--~---
    • Yegappan Lakshmanan
      Hi, ... For maintainability reasons, it is better to assign the bname variable outside of this function call. ... Same here. It is better to assign the bame
      Message 2 of 18 , Mar 28, 2009
        Hi,

        On Sat, Mar 28, 2009 at 2:32 AM, Lech Lorens <lech.lorens@...> wrote:
        > Improved version of patch is attached. I took into consideration most of
        > the suggestions. I will be grateful for any more comments.
        >
        > --
        > Cheers,
        > Lech
        >
        >
        >
        > Index: src/hardcopy.c
        > ===================================================================
        > --- src/hardcopy.c      (revision 1425)
        > +++ src/hardcopy.c      (working copy)
        > @@ -568,6 +568,8 @@
        >     long_u             bytes_to_print = 0;
        >     int                        page_line;
        >     int                        jobsplit;
        > +    char_u             *bname = NULL;
        > +    int                        do_return = FALSE;
        >
        >     memset(&settings, 0, sizeof(prt_settings_T));
        >     settings.has_color = TRUE;
        > @@ -599,11 +601,15 @@
        >      */
        >     if (mch_print_init(&settings,
        >                        curbuf->b_fname == NULL
        > -                           ? (char_u *)buf_spname(curbuf)
        > +                           ? (bname = (char_u *)buf_spname(curbuf))

        For maintainability reasons, it is better to assign the bname variable
        outside of this
        function call.

        >                            : curbuf->b_sfname == NULL
        >                                ? curbuf->b_fname
        >                                : curbuf->b_sfname,
        >                        eap->forceit) == FAIL)
        > +       do_return = TRUE;
        > +
        > +    vim_free(bname);
        > +    if (do_return)
        >        return;
        >
        >  #ifdef FEAT_SYN_HL
        > Index: src/ex_docmd.c
        > ===================================================================
        > --- src/ex_docmd.c      (revision 1425)
        > +++ src/ex_docmd.c      (working copy)
        > @@ -7285,6 +7285,7 @@
        >     tabpage_T  *tp;
        >     win_T      *wp;
        >     int                tabcount = 1;
        > +    char_u     *bname;
        >
        >     msg_start();
        >     msg_scroll = TRUE;
        > @@ -7307,8 +7308,11 @@
        >            msg_putchar(' ');
        >            msg_putchar(bufIsChanged(wp->w_buffer) ? '+' : ' ');
        >            msg_putchar(' ');
        > -           if (buf_spname(wp->w_buffer) != NULL)
        > -               STRCPY(IObuff, buf_spname(wp->w_buffer));
        > +           if ((bname = (char_u *)buf_spname(wp->w_buffer)) != NULL)
        >

        Same here. It is better to assign the bame variable outside the if statement.
        This comment also applies to other places in this diff.

        - Yegappan

        > +           {
        > +               vim_strncpy(IOSIZE, bname, IOSIZE - 1);
        > +               vim_free(bname);
        > +           }
        >            else
        >                home_replace(wp->w_buffer, wp->w_buffer->b_fname,
        >                                                        IObuff, IOSIZE, TRUE);

        --~--~---------~--~----~------------~-------~--~----~
        You received this message from the "vim_dev" maillist.
        For more information, visit http://www.vim.org/maillist.php
        -~----------~----~----~----~------~----~------~--~---
      • Andreas Bernauer
        ... I m curious: what s the rationale to have the assignment outside the function call/if-condition? -- Andreas.
        Message 3 of 18 , Mar 29, 2009
          Yegappan Lakshmanan wrote:
          >> Index: src/hardcopy.c
          >> ===================================================================
          >> --- src/hardcopy.c (revision 1425)
          >> +++ src/hardcopy.c (working copy)
          >> @@ -568,6 +568,8 @@
          >> long_u bytes_to_print = 0;
          >> int page_line;
          >> int jobsplit;
          >> + char_u *bname = NULL;
          >> + int do_return = FALSE;
          >>
          >> memset(&settings, 0, sizeof(prt_settings_T));
          >> settings.has_color = TRUE;
          >> @@ -599,11 +601,15 @@
          >> */
          >> if (mch_print_init(&settings,
          >> curbuf->b_fname == NULL
          >> - ? (char_u *)buf_spname(curbuf)
          >> + ? (bname = (char_u *)buf_spname(curbuf))
          >
          > For maintainability reasons, it is better to assign the bname variable
          > outside of this
          > function call.

          I'm curious: what's the rationale to have the assignment outside the
          function call/if-condition?

          --
          Andreas.

          --~--~---------~--~----~------------~-------~--~----~
          You received this message from the "vim_dev" maillist.
          For more information, visit http://www.vim.org/maillist.php
          -~----------~----~----~----~------~----~------~--~---
        • Andreas Bernauer
          ... Yes. ... Great. ... Thanks for the patch. -- Andreas. --~--~---------~--~----~------------~-------~--~----~ You received this message from the vim_dev
          Message 4 of 18 , Mar 29, 2009
            Lech Lorens wrote:
            >> * The generated quickfix title seems to be saved somewhere. Is it difficult to
            >> add :csettitle! and :lsettitle! which switch back to the generated/default
            >> name?
            >
            > You mean to revert the effect of :csettitle "blah blah blah" you execute
            > :csettitle! ?

            Yes.

            >> * Loading the error list from a buffer with 'cbuffer' just mentions the buffer
            >> number ("cbuffer 24"), which is not very helpful. Could the title also
            >> display the buffer name?
            >
            > Done. Right now the buffer's name is appended inside parentheses to the
            > command name.

            Great.

            > Thank you for a scrupulous review!

            :-)

            Thanks for the patch.

            --
            Andreas.


            --~--~---------~--~----~------------~-------~--~----~
            You received this message from the "vim_dev" maillist.
            For more information, visit http://www.vim.org/maillist.php
            -~----------~----~----~----~------~----~------~--~---
          • Lech Lorens
            ... I found an invalid memory access while executing the :tabs command. The attached patch fixes the problem. -- Cheers, Lech
            Message 5 of 18 , Apr 16, 2009
              On 28-Mar-2009 Lech Lorens <lech.lorens@...> wrote:
              > Improved version of patch is attached. I took into consideration most of
              > the suggestions. I will be grateful for any more comments.

              I found an invalid memory access while executing the :tabs command. The
              attached patch fixes the problem.

              --
              Cheers,
              Lech

              --~--~---------~--~----~------------~-------~--~----~
              You received this message from the "vim_dev" maillist.
              For more information, visit http://www.vim.org/maillist.php
              -~----------~----~----~----~------~----~------~--~---
            • Lech Lorens
              Dear vim-dev subscribers, In message 20090322230448.GB7308@localdomain ( http://groups.google.com/group/vim_dev/browse_frm/thread/a44b2dff4a2ec379# ) I
              Message 6 of 18 , Oct 19, 2009
                Dear vim-dev subscribers,

                In message 20090322230448.GB7308@localdomain
                ( http://groups.google.com/group/vim_dev/browse_frm/thread/a44b2dff4a2ec379# )
                I informed vim-dev about the quickfix titles feature I developed and
                a few people expressed interest in the feature.

                I would like to inform those interested that thanks to the kindness of
                Markus Heidelberg the feature is now available as a part of the
                vim_extended repository available at repo.or.cz
                (git://repo.or.cz/vim_extended.git or
                http://repo.or.cz/r/vim_extended.git). The branch is called
                "feat/quickfix-title" but upon every change is merged into the "master"
                branch.

                The feature has been stable for me since the 17th of April when
                I removed the last bug so far. If anyone has any more comments and/or
                questions, I will be happy to answer.

                --
                Cheers,
                Lech

                --~--~---------~--~----~------------~-------~--~----~
                You received this message from the "vim_dev" maillist.
                For more information, visit http://www.vim.org/maillist.php
                -~----------~----~----~----~------~----~------~--~---
              • Bram Moolenaar
                ... I finally took some time to look at this patch. Unfortunately documentation is missing, I think it must be elsewhere. Would be a lot simpler to have it
                Message 7 of 18 , Jul 17 6:29 AM
                  Lech Lorens wrote (more than a year ago):

                  > On 28-Mar-2009 Lech Lorens <lech.lorens@...> wrote:
                  > > Improved version of patch is attached. I took into consideration most of
                  > > the suggestions. I will be grateful for any more comments.
                  >
                  > I found an invalid memory access while executing the :tabs command. The
                  > attached patch fixes the problem.

                  I finally took some time to look at this patch. Unfortunately
                  documentation is missing, I think it must be elsewhere. Would be a lot
                  simpler to have it as one patch.

                  Adding two commands to set the title is a bit too specific. Why not put
                  the values in window-local variables? Then these can be set from any
                  script that generates a quickfix or location list. The ":make" and
                  ":grep" commands can also do this. Advantage is that the values are
                  available to a custom status line as well. But perhaps there is a
                  disadvantage as well?

                  There is quite a bit of repeated code. E.g., buf_spname() is often
                  called to fill a buffer. Can easily make a separate function for this.

                  --
                  The question is: What do you do with your life?
                  The wrong answer is: Become the richest guy in the graveyard.
                  (billionaire and Oracle founder Larry Ellison)

                  /// Bram Moolenaar -- Bram@... -- http://www.Moolenaar.net \\\
                  /// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
                  \\\ download, build and distribute -- http://www.A-A-P.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
                • Lech Lorens
                  ... At that time the runtime files were not under version control like the source code was. That is why I split the patch into two files. Sorry for the
                  Message 8 of 18 , Jul 20 3:52 PM
                    On 17-Jul-2010 Bram Moolenaar <Bram@...> wrote:
                    >
                    > Lech Lorens wrote (more than a year ago):
                    >
                    > > On 28-Mar-2009 Lech Lorens <lech.lorens@...> wrote:
                    > > > Improved version of patch is attached. I took into consideration most of
                    > > > the suggestions. I will be grateful for any more comments.
                    > >
                    > > I found an invalid memory access while executing the :tabs command. The
                    > > attached patch fixes the problem.
                    >
                    > I finally took some time to look at this patch. Unfortunately
                    > documentation is missing, I think it must be elsewhere. Would be a lot
                    > simpler to have it as one patch.

                    At that time the runtime files were not under version control like the
                    source code was. That is why I split the patch into two files. Sorry for
                    the inconvenience.

                    > Adding two commands to set the title is a bit too specific. Why not put
                    > the values in window-local variables? Then these can be set from any
                    > script that generates a quickfix or location list. The ":make" and
                    > ":grep" commands can also do this. Advantage is that the values are
                    > available to a custom status line as well. But perhaps there is a
                    > disadvantage as well?

                    I like your idea very much, makes the whole mechanism much more
                    flexible. Modifying Vim to use w:quickfix_title variable for specifying
                    what is displayed in the status bar of a quickfix window proved to be
                    easy. I need some more time to clean up the code, though. I'll do it on
                    Thursday.

                    I can't see any disadvantages of this approach. However, one thing that
                    becomes obvious at this point, is that unless it can be done with
                    a clever trick that I can't think of right now (using the value of &bt,
                    but then what?), there is no way to make a custom status line display
                    "Quickfix List" or "Location List", much less their localised versions.

                    > There is quite a bit of repeated code. E.g., buf_spname() is often
                    > called to fill a buffer. Can easily make a separate function for this.

                    You mean I should prepare a function which will copy the value returned
                    by buf_spname() to e.g. IObuffer and free the allocated memory? Sounds
                    like a good idea.

                    Thanks for the enormous amount of work you had to put into Vim recently!

                    --
                    Cheers,
                    Lech

                    --
                    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
                  • Bram Moolenaar
                    ... It would be possible to add a statusline item for this, like %h. ... Note that I m including less changes now, and nothing that looks dangerous or needs
                    Message 9 of 18 , Jul 21 11:52 AM
                      Lech Lorens wrote:

                      > On 17-Jul-2010 Bram Moolenaar <Bram@...> wrote:
                      > >
                      > > Lech Lorens wrote (more than a year ago):
                      > >
                      > > > On 28-Mar-2009 Lech Lorens <lech.lorens@...> wrote:
                      > > > > Improved version of patch is attached. I took into consideration most of
                      > > > > the suggestions. I will be grateful for any more comments.
                      > > >
                      > > > I found an invalid memory access while executing the :tabs command. The
                      > > > attached patch fixes the problem.
                      > >
                      > > I finally took some time to look at this patch. Unfortunately
                      > > documentation is missing, I think it must be elsewhere. Would be a lot
                      > > simpler to have it as one patch.
                      >
                      > At that time the runtime files were not under version control like the
                      > source code was. That is why I split the patch into two files. Sorry for
                      > the inconvenience.
                      >
                      > > Adding two commands to set the title is a bit too specific. Why not put
                      > > the values in window-local variables? Then these can be set from any
                      > > script that generates a quickfix or location list. The ":make" and
                      > > ":grep" commands can also do this. Advantage is that the values are
                      > > available to a custom status line as well. But perhaps there is a
                      > > disadvantage as well?
                      >
                      > I like your idea very much, makes the whole mechanism much more
                      > flexible. Modifying Vim to use w:quickfix_title variable for specifying
                      > what is displayed in the status bar of a quickfix window proved to be
                      > easy. I need some more time to clean up the code, though. I'll do it on
                      > Thursday.
                      >
                      > I can't see any disadvantages of this approach. However, one thing that
                      > becomes obvious at this point, is that unless it can be done with
                      > a clever trick that I can't think of right now (using the value of &bt,
                      > but then what?), there is no way to make a custom status line display
                      > "Quickfix List" or "Location List", much less their localised versions.

                      It would be possible to add a statusline item for this, like %h.

                      > > There is quite a bit of repeated code. E.g., buf_spname() is often
                      > > called to fill a buffer. Can easily make a separate function for this.
                      >
                      > You mean I should prepare a function which will copy the value returned
                      > by buf_spname() to e.g. IObuffer and free the allocated memory? Sounds
                      > like a good idea.
                      >
                      > Thanks for the enormous amount of work you had to put into Vim recently!

                      Note that I'm including less changes now, and nothing that looks
                      "dangerous" or needs to be discussed first.

                      --
                      My girlfriend told me I should be more affectionate.
                      So I got TWO girlfriends.

                      /// Bram Moolenaar -- Bram@... -- http://www.Moolenaar.net \\\
                      /// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
                      \\\ download, build and distribute -- http://www.A-A-P.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
                    • Lech Lorens
                      ... [...] ... I followed your excellent advice with a window variable. This made the whole thing much simpler (the patch is almost 50% smaller than the
                      Message 10 of 18 , Jul 22 2:43 PM
                        On 21-Jul-2010 Bram Moolenaar <Bram@...> wrote:
                        >
                        > It would be possible to add a statusline item for this, like %h.
                        [...]
                        > Note that I'm including less changes now, and nothing that looks
                        > "dangerous" or needs to be discussed first.

                        I followed your excellent advice with a window variable. This made the
                        whole thing much simpler (the patch is almost 50% smaller than the
                        previous one).

                        Since you disliked the repeated code calling buf_spname(), I reverted
                        buf_spname() to the original shape, which means that no more memory
                        allocations are needed.

                        The only thing that is being done now is that the command which produces
                        a list of errors is remembered along with the quickfix list and when
                        a quickfix window is opened, the command name is copied to the
                        w:quickfix_title variable.

                        Since I also added a flag for the 'statusline' variable, I added a file
                        ftplugin/qf.vim, which sets 'statusline' for quickfix windows so that
                        the value of w:quickfix_title is displayed.

                        The patch is against changeset 878562053b.

                        --
                        Cheers,
                        Lech

                        --
                        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
                      • Bram Moolenaar
                        ... Thanks. I ll have a closer look soon. -- It is illegal for a driver to be blindfolded while operating a vehicle. [real standing law in Alabama, United
                        Message 11 of 18 , Jul 24 8:02 AM
                          Lech Lorens wrote:

                          > On 21-Jul-2010 Bram Moolenaar <Bram@...> wrote:
                          > >
                          > > It would be possible to add a statusline item for this, like %h.
                          > [...]
                          > > Note that I'm including less changes now, and nothing that looks
                          > > "dangerous" or needs to be discussed first.
                          >
                          > I followed your excellent advice with a window variable. This made the
                          > whole thing much simpler (the patch is almost 50% smaller than the
                          > previous one).
                          >
                          > Since you disliked the repeated code calling buf_spname(), I reverted
                          > buf_spname() to the original shape, which means that no more memory
                          > allocations are needed.
                          >
                          > The only thing that is being done now is that the command which produces
                          > a list of errors is remembered along with the quickfix list and when
                          > a quickfix window is opened, the command name is copied to the
                          > w:quickfix_title variable.
                          >
                          > Since I also added a flag for the 'statusline' variable, I added a file
                          > ftplugin/qf.vim, which sets 'statusline' for quickfix windows so that
                          > the value of w:quickfix_title is displayed.
                          >
                          > The patch is against changeset 878562053b.

                          Thanks. I'll have a closer look soon.

                          --
                          It is illegal for a driver to be blindfolded while operating a vehicle.
                          [real standing law in Alabama, United States of America]

                          /// Bram Moolenaar -- Bram@... -- http://www.Moolenaar.net \\\
                          /// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
                          \\\ download, build and distribute -- http://www.A-A-P.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
                        Your message has been successfully submitted and would be delivered to recipients shortly.