[patch] Titles for quickfix / location list windows
Lech Lorens
reflect the command that yielded the error list.
Additionally, two commands have been added:
:cse[ttitle]
:lse[ttitle]
The former allows for setting a custom title of the quickfix window. The
latter changes the title of the location list window.
An example can be found here:
http://i41.tinypic.com/nd8obr.png
The patch was made against Vim 7.2.147.
--
Cheers,
Lech
Bram Moolenaar
Lech Lorens wrote:
Thanks. I'll await comments.
--
hundred-and-one symptoms of being an internet addict:
259. When you enter your name in the AltaVista search engine, the top ten
matches do indeed refer to you.
/// Bram Moolenaar -- Br...@Moolenaar.net -- 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 ///
Andreas Bernauer
> Lech Lorens wrote:
>> The attached patch makes the quickfix / location list window's title
>> reflect the command that yielded the error list.
>>
>> Additionally, two commands have been added:
>> :cse[ttitle]
>> :lse[ttitle]
>>
>> The former allows for setting a custom title of the quickfix window. The
>> latter changes the title of the location list window.
>>
>> An example can be found here:
>> http://i41.tinypic.com/nd8obr.png
>>
>> The patch was made against Vim 7.2.147.
>
> Thanks. I'll await comments.
Seems to work.
Passes all tests.
Some remarks on qf_title_src.patch:
* Shouldn't the new qf_title argument to qf_init be commented? Same for
qf_init_ext.
* Without the patch, the quickfix buffer name is '[Quickfix List]', with
:csettitle it is '[Quickfix List] ' (note the extra space, visible via
':buffers'). Maybe we want to keep this consistently '[Quickfix List]'?
Ditto for lsettitle.
* Changes at the end of hunk src/quickfix.c @@ -257,12 +319,13 @@, and hunk
src/if_cscope.c @@ -1194,7 +1208,7 @@ are
only prettyfying the source/adding comments and have nothing to do with the
proposed feature.
(I don't know if this is ok or not, I just wanted to mention it.)
* I also want to point out that the patch changes the semantics of buf_spname in
src/buffer.c: buf_spname now returns an allocated string, which has to be
freed by the caller. The patch takes care that all (other) calls to buf_spname
free the returned string, but other patches might not be aware of this change,
or this change might be considered as an unwanted side effect.
* In hunk @@ -5101,9 +5121,30 @@, _(btname) is unnecessarily called twice.
Can't you assign btname = _("[Location List") right away?
* Could src/main.c, hunk @@ -662,10 +662,12 @@ use IObuff instead of introducing
own buffer 'qf_title_buf[IOSIZE]'?
* The patch adds a terminating NUL at strings created with STRNCPY, which is
fine. Maybe this could be done for other calls to STRNCPY, too, in a
separate patch? And do this in src/buffer.c @@ -2554,8 +2555,11 @@,
@@ -2974,8 +2979,11 @@, @@ -3659,8 +3672,11 @@,
too (or add short comment why this is not necessary)?
* src/screen.c @@ -9342,8 +9342,12 @@ is STRCPY(NameBuff, bname). Do we know
bname is shorter than NameBuff's size? At least, bname comes from some
arbitrary command line. src/memline.c @@ -1060,8 +1064,12 @@ uses STRNCPY.
Remarks for improvement:
* :[l]grepadd adds new matches to the location/error list. Should the title
show this? Ditto for caddfile, etc.
* 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?
* When the error list is loaded with 'cfile /tmp/errors', could the title just
say '/tmp/errors' or ':cfile /tmp/errors'. This would give a visual hint
that the error list was loaded with a vim command and not by a program called
'cfile'. Ditto for 'lfile', 'cgetfile', 'lgetfile'.
* 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?
--
Andreas.
Dominique Pelle
Thanks Lech. I tried it. It works well and I find it useful.
Some remarks (besides what Andreas already mentioned):
1/ The code contains several calls to STRNCPY(...).
It would be better I think to replace them with
vim_strncpy(...) to guarantee that the string is
always NUL terminated:
For example, in buffer.c:2560:
STRNCPY(NameBuff, bname, MAXPATHL);
... could be written safer as:
vim_strncpy(NameBuff, bname, MAXPATHL - 1);
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."
I'll keep testing when I have more time, but so far it works fine.
Thanks
-- Dominique
Dominique Pelle
> The attached patch makes the quickfix / location list window's title
> reflect the command that yielded the error list.
>
> Additionally, two commands have been added:
> :cse[ttitle]
> :lse[ttitle]
Additional attached patch adds documentation of
:csettitle and :lsettitle to doc/index.txt and doc/cmdline.txt.
-- Dominique
Lech Lorens
>
> Seems to work.
> Passes all tests.
All suggestions I do not relate to have been taken into account.
> * I also want to point out that the patch changes the semantics of buf_spname in
> src/buffer.c: buf_spname now returns an allocated string, which has to be
> freed by the caller. The patch takes care that all (other) calls to buf_spname
> free the returned string, but other patches might not be aware of this change,
> or this change might be considered as an unwanted side effect.
Agreed. Will wait for Bram's decision regarding this issue.
> Remarks for improvement:
> * :[l]grepadd adds new matches to the location/error list. Should the title
> show this? Ditto for caddfile, etc.
My feeling is that the original command is the more important one. This
is why I chose to keep its title and decided to ignore the .*add.*
commands. This was also the reason for introducing the :csettitle and
:lsettitle commands.
> * 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! ?
If so - done.
> * When the error list is loaded with 'cfile /tmp/errors', could the title just
> say '/tmp/errors' or ':cfile /tmp/errors'. This would give a visual hint
> that the error list was loaded with a vim command and not by a program called
> 'cfile'. Ditto for 'lfile', 'cgetfile', 'lgetfile'.
I'd say either all commands or none. Currently implemented it for all
commands that can produce an error list.
> * 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.
Thank you for a scrupulous review!
--
Cheers,
Lech
Lech Lorens
>
> Some remarks (besides what Andreas already mentioned):
>
>
> ... 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
Yegappan Lakshmanan
> 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);
Andreas Bernauer
>> 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.
Andreas Bernauer
>> * 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.
Lech Lorens
> 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
Lech Lorens
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
Bram Moolenaar
Lech Lorens wrote (more than a year ago):
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)
Lech Lorens
>
> Lech Lorens wrote (more than a year ago):
>
> > On 28-Mar-2009 Lech Lorens <lech....@gmail.com> 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