[vim/vim] Make sure QuickFixCmd autocommands are triggered by :ltag (#3001)
Matéo Zanibelli
Problem
Autocommands QuickFixCmdPre and QuickFixCmdPost are not triggered when performing :ltag.
Solution
Trigger these autocommands when performing :ltag.
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/3001
Commit Summary
- Make sure QuickFixCmd autocommands are triggered by :ltag
File Changes
- M runtime/doc/autocmd.txt (2)
- M src/tag.c (21)
- M src/testdir/test_quickfix.vim (19)
Patch Links:
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
Matéo Zanibelli
I have some trouble understanding why some of the checks don't pass. It's my first PR and first time tinkering with the codebase so let me know if I can improve the code.
Christian Brabandt
last patch has a missing define for os_unix.c. I'll post a fix.
Christian Brabandt
can you also include a test?
Matéo Zanibelli
Yegappan Lakshmanan
@yegappan commented on this pull request.
In src/tag.c:
> @@ -1028,6 +1040,15 @@ do_tag( */ i = jumpto_tag(matches[cur_match], forceit, type != DT_CSCOPE); + /* + * Trigger QuickFixCmdPost autocommand. + */ +#if defined(FEAT_QUICKFIX) + if (type == DT_LTAG) + apply_autocmds(EVENT_QUICKFIXCMDPOST, (char_u *)"ltag",
The QuickfixCmdPost autocmd is triggered before jumping to the error.
So this should be moved right after setting the list of tags to the
location list using set_errorlist().
In src/testdir/test_quickfix.vim:
> @@ -2038,6 +2038,16 @@ func QfAutoCmdHandler(loc, cmd) endfunc func Test_Autocmd() + + set tags=Xtags + call writefile(["!_TAG_FILE_ENCODING\tutf-8\t//", + \ "one\tXfile1\t/^one/;\"\tf\tfile:\tsignature:(void)"], + \ 'Xtags') + new Xfile1 + call setline(1, ['empty', 'one()', 'empty']) + write + bwipe!
The creation of Xfile1 can be simplified by using
call writefile(['empty', 'one()', 'empty'], 'Xfile1')
Matéo Zanibelli
@mzanibelli commented on this pull request.
In src/tag.c:
> @@ -1028,6 +1040,15 @@ do_tag( */ i = jumpto_tag(matches[cur_match], forceit, type != DT_CSCOPE); + /* + * Trigger QuickFixCmdPost autocommand. + */ +#if defined(FEAT_QUICKFIX) + if (type == DT_LTAG) + apply_autocmds(EVENT_QUICKFIXCMDPOST, (char_u *)"ltag",
That was indeed my first attempt (adding this chunk around the line 928). Unfortunately, and I don't know exactly why, this results in having the buffer to which the first error points to displayed inside the location lists's window. Would you mind helping me a bit with this one ?
Matéo Zanibelli
@mzanibelli commented on this pull request.
In src/tag.c:
> @@ -1028,6 +1040,15 @@ do_tag( */ i = jumpto_tag(matches[cur_match], forceit, type != DT_CSCOPE); + /* + * Trigger QuickFixCmdPost autocommand. + */ +#if defined(FEAT_QUICKFIX) + if (type == DT_LTAG) + apply_autocmds(EVENT_QUICKFIXCMDPOST, (char_u *)"ltag",
Sorry in unclear: assuming you have a QuickFixCmdPost autocommand to open the location list when non-empty.
Matéo Zanibelli
@mzanibelli pushed 2 commits.
—
You are receiving this because you are subscribed to this thread.
Codecov
Codecov Report
Merging #3001 into master will increase coverage by
0.02%.
The diff coverage is85.71%.
@@ Coverage Diff @@ ## master #3001 +/- ## ========================================== + Coverage 75.82% 75.84% +0.02% ========================================== Files 92 92 Lines 135297 135302 +5 ========================================== + Hits 102583 102624 +41 + Misses 32714 32678 -36
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/tag.c | 75.64% <85.71%> (+3.23%) |
⬆️ |
| src/gui_gtk_x11.c | 47.33% <0%> (-0.55%) |
⬇️ |
| src/if_xcmdsrv.c | 83.63% <0%> (-0.54%) |
⬇️ |
| src/ex_cmds.c | 79.53% <0%> (-0.23%) |
⬇️ |
| src/ex_cmds2.c | 80.2% <0%> (-0.1%) |
⬇️ |
| src/eval.c | 81.52% <0%> (-0.06%) |
⬇️ |
| src/os_unix.c | 54.29% <0%> (-0.05%) |
⬇️ |
| src/version.c | 82.38% <0%> (ø) |
⬆️ |
| src/channel.c | 83.2% <0%> (ø) |
⬆️ |
| src/normal.c | 73.93% <0%> (+0.01%) |
⬆️ |
| ... and 10 more |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered by Codecov. Last update d7a137f...4ee06e6. Read the comment docs.
Matéo Zanibelli
@mzanibelli commented on this pull request.
In src/tag.c:
> @@ -1028,6 +1040,15 @@ do_tag( */ i = jumpto_tag(matches[cur_match], forceit, type != DT_CSCOPE); + /* + * Trigger QuickFixCmdPost autocommand. + */ +#if defined(FEAT_QUICKFIX) + if (type == DT_LTAG) + apply_autocmds(EVENT_QUICKFIXCMDPOST, (char_u *)"ltag",
Actually I've been thinking about that case. I'd say there is a conflict between the meaning of two very different things:
- Jump to a tag
- Jump to a quickfix's location
As the documentation says:
:lt[ag][!] [name] Jump to tag [name] and add the matching tags to a new location list for the current window. [...] The location list showing the matching tags is independent of the tag stack.
As for QuickFixCmdPost:
QuickFixCmdPost Like QuickFixCmdPre, but after a quickfix
command is run, before jumping to the first
location. [...]
Actually at that point we did jump to the tag as in pushing to the tag stack. The location list was filled but we did not explicitely ask for jumping to the first location even if the result is similar.
That's why, instead of trying to change a behavior that could be tricky, I'd suggest considering to explain this particular behavior in the docs. @chrisbra @yegappan please let me know your opinion... :-)
Bram Moolenaar
What is the status if this PR? @yegappan are your comments taken care of?
Matéo Zanibelli
@brammool I tried to rebase the code against current master branch, but please have a look on this thread to decide what to do. I'm afraid I'll need some help implementing the mentioned behavior if it appears my understanding is wrong.
Bram Moolenaar
Perhaps @yegappan can help
Yegappan Lakshmanan
On Tue, Sep 24, 2019 at 10:21 PM Yegappan Lakshmanan
<yega...@gmail.com> wrote:
>
> Hi,
> On Tue, Sep 24, 2019 at 4:30 AM Matéo Zanibelli
> <vim-dev...@256bit.org> wrote:
> >
> > @brammool I tried to rebase the code against current master branch, but please have
> > a look on this thread to decide what to do. I'm afraid I'll need some help implementing the
> > mentioned behavior if it appears my understanding is wrong.
> >
>
>
Can you try the attached slightly updated version of your patch?
- Yegappan
K.Takata
@yegappan's comment to vim_dev is not forwarded here.
See this: https://groups.google.com/d/msg/vim_dev/z80ZpXnONiQ/o9ahN3kPBQAJ
Can you try the attached slightly updated version of your patch?
—
Matéo Zanibelli
@k-takata Thanks for notifying me. I'll try to have a look on the email thread from now on.
Unfortunately, the patch produces the exact same behavior than reordering my statements.
@yegappan Shall I keep it that way as it seems to be a bit more resilient anyway?
Moreover, here is the behavior that should maybe be considered:
- before jumping to the tag
- if current window is a location list window, jump to the corresponding standard window
- otherwise don't move
Bram Moolenaar
Closing this for lack of updates.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
Bram Moolenaar
Closed #3001.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.