Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

syntastic closes an open location list on Write #637

Open
chrisbra opened this Issue May 2, 2013 · 29 comments

Comments

Projects
None yet
5 participants

chrisbra commented May 2, 2013

Hi,
I just noticed, that synastic closes an already open location list. It would be nice, if when no error is found, syntastic would reopen the last open location list (e.g. by using :lolder or something a like).

Thanks.

Collaborator

lcd047 commented May 2, 2013

No errors to display --> no loclist --> no error window. :) Coming up with an older list just to keep the window there doesn't make much sense as far as I can tell.

@lcd047 lcd047 closed this May 2, 2013

chrisbra commented May 2, 2013

That is wrong. If I have created a location list by e.g. using lvimgrep then syntastic shouldn't close it, just because there is no error when compiling that file. It should simply leave my location list alone.

@lcd047 lcd047 reopened this May 2, 2013

Collaborator

lcd047 commented May 2, 2013

Ah, I see. I don't think there is any easy way to achieve that. The way we handle opening and closing the error window is already pretty fragile.

chrisbra commented May 2, 2013

Here is a patch, that works for me. Please consider merging https://github.com/chrisbra/syntastic/commit/b8eb1d27dd92449ebf7278567fc64f4c02883387

@chrisbra chrisbra referenced this issue May 2, 2013

Closed

Fixes #637 #638

Collaborator

lcd047 commented May 2, 2013

That breaks in two ways: it opens the error window even if it was previous empty, and it populates the loclist with the wrong set of locations if there are errors, but g:syntastic_always_populate_loc_list and g:syntastic_auto_jump are both 0. There is also a more subtle problem: calling setloclist() after getloclist() can lose information, since syntastic-created loclists have non-standard attributes along with the standard ones. As I said, it isn't that obvious.

Collaborator

lcd047 commented May 2, 2013

One more thing: if all errors are fixed and the previous list of locations was generated by Syntastic, you don't want to resurrect the errors as zombies. I don't see how you could distinguish between pre-existing loclist entries and Syntastic-created ones based only on getloclist().

Collaborator

lcd047 commented May 2, 2013

The second patch addresses only the first problem I noted above, opening the error window even if it's empty. It doesn't seem to address the other ones.

chrisbra commented May 2, 2013

Indeed. I still find it disappointing, that this breaks several plugins, that create a location list for themselves. There must be a simpler way for syntastic to not mess with location lists, that have not been created by syntastic.

chrisbra commented May 2, 2013

I don't mind, having to create a new location list if syntax errors are found, but if not, it should still leave the old location list alone

Collaborator

lcd047 commented May 2, 2013

I agree, Syntastic should be able to coexist peacefully with pre-existing loclists. There might be a way to achieve that (we already do it for signs), but as far as I can tell it wouldn't be completely trivial.

Collaborator

lcd047 commented May 4, 2013

This is probably a step in the right direction: 003751a.

Collaborator

scrooloose commented May 6, 2013

Just so I understand, here is what is happening right?

  1. Open a file with no errors
  2. Add a loclist --> :call setloclist(0, [{'text': "example", 'lnum': 10, 'bufnr': bufnr("")}])
  3. :lopen
  4. :write

This causes the location list that was opened in step 3 to be closed by this: https://github.com/scrooloose/syntastic/blob/master/plugin/syntastic/autoloclist.vim#L33

This is a known problem that I have been pondering for a while. I have not found a nice solution and I'm pretty sure there isnt one.

Ideally we want to be able to tag an entire loclist (or its individual entries) as being created by syntastic. Then we would be able to tell if we were messing with someone else's loclist.

One hacky way could be to set one of the less critical items in each loclist item to a magic value. e.g. set the nr attribute to 99 so we know we created it. Then we could scan the loclist before closing it and ensure we dont close someone else's. Or we could change our approach and simply add/remove syntastic items to the existing loclist.

Im just floating the idea really, thoughts?

Collaborator

lcd047 commented May 6, 2013

@scrooloose: I think you essentially nailed it. The bigger issue is dealing in a meaningful way with loclists created by other plugins (or perhaps lvimgrep). Once this is solved, it should be trivial to decide when to close or open the loclist window.

As I see it, there are three possible approaches to dealing with the main problem:

  1. Make :lolder and :lnewer work with syntastic. A step towards that was my patch 003751a above, but that turned out to be problematic (see #641), and Vim makes it next to impossible to control exactly what loclist is active at any given time.
  2. Use "mixed" loclists, that is, make syntastic add / remove errors to the old loclist. This can be messy, since we use non-standard attributes for loclist errors. It can also be confusing to the user (personally I wouldn't want syntax errors mixed with, say, grep results in a single list).
  3. Save the previous loclist, let syntastic do its thing, then restore the old loclist when done. The main problem here is being able to distinguish syntastic loclists from external loclists.

Of these, 3. seems the least problematic approach. Inventing a magic attribute for tagging is really ugly, but it should work. I thought about this for a while, and I haven't been able to find anything better.

chrisbra commented May 6, 2013

Hi Martin!

Would it help, if we create a patch of the Vim source code, that adds an
additional parameter to the setqflist()/setloclist() functions, which
would set the w:quickfix_title variable accordingly?

This means, syntastic would call setloclist(0, [....], '', 'syntastic
locationlist') to create the locationlist and whenever it needs to work
with the quickfix list, it can simply query the w:quickfix_title
variable to find out, if this location list has been created by
syntastic.

(I have a patch ready, it just needs to be submitted to Bram for
inclusion. It might however take a while until Bram commits it, so this
isn't a solution, that will be available soon).

regards,
Christian

Collaborator

lcd047 commented May 6, 2013

@chrisbra: I'm not Martin, but I'd say such a patch would be useful but not enough. First, because we would still have to come up with some sensible behaviour for older versions of Vim, and second, because given a buffer, there is no way (at least not an obvious one) to find out which window corresponds to the associated loclist. Unlike quickfix windows, there can be multiple loclist windows.

If you're willing to go that way though, I'd really suggest taking your time to fix the real problem: managing error lists from VimL. I'd suggest at least these new features:

  • functions to read and edit the stack of saved error lists
  • functions to get the current position in that stack, and to move the stack pointer (like :lolder and :lnewer)
  • a function to get the owner window of an error list
  • options for :grep and friends to create a new error list or reuse an existing one (right now it always reuses an existing list)
  • options for :make and friends to create a new error list or reuse an existing one (right now it always creates a new list)
  • an option to add a tag to an error list, which could be used, as you suggest, for setting w:quickfix_title.

As far as I can tell, none of these should be particularly hard to implement. The functionality is already there, it just has to be exported to VimL. In my opinion it would make a huge difference: it would actually make error lists manageable from scripts. With the current interface, trying to do anything beyond the basics with error lists is pretty much hopeless.

Collaborator

lcd047 commented May 8, 2013

Ok, tagging would be something like this: a18a04b. The "tag" error is marked automatically as invalid, but that doesn't prevent it from being displayed. So this is not a viable idea. :(

Hi LCD!

I created some patches:

  1. set w:quickfix_title with setqflist():
    https://groups.google.com/d/msg/vim_dev/X7VVPd4Do5s/7j0TX6KJOp0J
  2. read stack of error lists (and get current position in error
    stack):
    https://groups.google.com/d/msg/vim_dev/51EmHJW6oSA/z556jnB0LLEJ
    (write is not necessary, can be done by using a combination of
    lolder/setqflist...)
  3. Get tdoesn't make much sense, to get the owner of an error list,
    because window numbers are not persistent they change too fast.
  4. an option to add/create new error list for make/grep etc.
    I am not sure, this makes much sense and I am afraid, this does not
    provide enough value to have it accepted (so I haven't created a
    patch for that yet).

regards,
Christian

Collaborator

lcd047 commented May 12, 2013

@chrisbra: That's great, thank you so much!

The patch that allows setting w:quickfix_title from setqflist() looks good, there isn't much to be said about it.

The patch adding getlocstack() and setlocstack() seems to work as intended, but I'm not sure about the interface. What if the stack gets extended to 15 items, then to 20? I'd suggest a simpler approach: just make getlocstack() return a list of loclists, make a separate getlocstackptr() return the index (1-based, to follow the VimL convention) of the current pointer into this list, then add a separate function to return the title of the loclist for a given index. A setlocstackptr() counterpart to getlocstackptr() would indeed be redundant, but it would still be nice to have, for symmetry. That would make a much cleaner interface in my opinion.

Getting the owner window of an error list is important, because without it there is no safe way to solve problems like the one described in #400. Window numbers being ephemeral should not be a problem, since VimL is single-threaded. In my opinion setloclist() taking a window argument is a design flaw, but we're stuck with it by now.

As for an option for :make and friends (not) to create a new loclist, that's what it takes to make :lolder and :lnewer work properly. Have you noticed that plugins using :grep (such as Ack, Ag, and many others) work fine with these functions, but none of the plugins using the :make mechanism do? Why do you think that is?

Hi LCD!

On So, 12 Mai 2013, LCD 47 wrote:

@chrisbra: That's great, thank you so much!

The patch that allows setting w:quickfix_title from setqflist() looks good, there isn't much to be said about it.

The patch adding getlocstack() and setlocstack() seems to work as intended, but I'm not sure about the interface. What if the stack gets extended to 15 items, then to 20?

Doesn't matter. It will still work correctly.

I'd suggest a simpler approach: just make getlocstack() return a list
of loclists, make a separate getlocstackptr() return the index
(1-based, to follow the VimL convention) of the current pointer into
this list, then add a separate function to return the title of the
loclist for a given index. A setlocstackptr() counterpart to
getlocstackptr() would indeed be redundant, but it would still be
nice to have, for symmetry. That would make a much cleaner interface
in my opinion.

Hm, I don't really mind. Please raise your voice in the mentioned
vim_dev thread. I just want to know, what others think about this. If we
generally agree on that, it would be great and I'll implement it but I
just want to make sure other developers have the chance to comment on
this.

Getting the owner window of an error list is important, because
without it there is no safe way to solve problems like the one
described in #400. Window numbers being ephemeral should not be a
problem, since VimL is single-threaded. In my opinion setloclist()
taking a window argument is a design flaw, but we're stuck with it by
now.

The api isn't really well defined for this. The window number is just a
counter going through the list of windows in the vim source. So
returning the window number for a location list doesn't help much.
Between the call of getlocstack() and restoring the stack, the windows
could have changed already (think of autocommands). I am not sure how to
approach this.

As for an option for :make and friends (not) to create a new
loclist, that's what it takes to make :lolder and :lnewer work
properly.
Have you noticed that plugins using :grep (such as Ack,
Ag, and many others) work fine with these functions, but none of the
plugins using the :make mechanism do? Why do you think that is?

I have no clue as I generally tend to not use many plugins besides my
own.

So how would you suggest to specify such an option? We can't use ! as
this attribute is already taken. I can only think of adding yet another
option or creating another command. But this issue should probably also
be raised at vim_dev.

regards,

Christian

Hi LCD!

As far as I can see, :grep and :make work exactly the same (only
[l]grepadd adds to an existing quickfix list all :make and :grep should
always create a new quickfix list). So what is really the problem with
:make and why doesn't it happen with :grep?
Do you know about any more detail (tickets/discussions) about this
issue?

regards,
Christian

Collaborator

lcd047 commented May 13, 2013

@chrisbra: Sure, I'll post a note to vim_dev about getlocstack().

Re: windows numbers: you're right, there is an even deeper design problem there, but I don't think it has any play in what I'm referring to. Basically, I'm asking for some way to convert between:

  1. the current window
  2. the associated loclist, when applicable
  3. the associated loclist window, when applicable.

Ephemeral window IDs should be fine for that. A possible problem might come up related to "orphaned" loclists, but that could be signaled by returning -1 for a window ID. Not being able to tell which loclist window belong to which buffer is a really annoying situation. :)

As for the :make and :grep, I'm referring to lines 2858-2862 in src/quickfix.c in the current HEAD:

res = qf_init(wp, fname, (eap->cmdidx != CMD_make
                         && eap->cmdidx != CMD_lmake) ? p_gefm : p_efm,
                                                  (eap->cmdidx != CMD_grepadd
                                                  && eap->cmdidx != CMD_lgrepadd),
                                                  *eap->cmdlinep);

The fourth parameter to qf_init() is newlist. I'm not aware of any discussion about it, since I stopped following the Vim lists some ~7 years ago (I re-subscribed recently). But I did some testing on my own a few days ago, related to this very issue, and the conclusion was :grep-based plugins have a somewhat easier life. :)

Hi LCD!

On Mo, 13 Mai 2013, LCD 47 wrote:

@chrisbra: Sure, I'll post a note to vim_dev about getlocstack().

Re: windows numbers: you're right, there is an even deeper design problem there, but I don't think it has any play in what I'm referring to. Basically, I'm asking for some way to convert between:

  1. the current window
  2. the associated loclist, when applicable
  3. the associated loclist window, when applicable.

Ephemeral window IDs should be fine for that. A possible problem might come up related to "orphaned" loclists, but that could be signaled by returning -1 for a window ID. Not being able to tell which loclist window belong to which buffer is a really annoying situation. :)

That sounds useful. I am just not sure how to accomplish that. I need to
think about it.

As for the :make and :grep, I'm referring to lines 2858-2862 in the current HEAD:

res = qf_init(wp, fname, (eap->cmdidx != CMD_make
                         && eap->cmdidx != CMD_lmake) ? p_gefm : p_efm,
                                                  (eap->cmdidx != CMD_grepadd
                                                  && eap->cmdidx != CMD_lgrepadd),
                                                  *eap->cmdlinep);

The fourth parameter to qf_init() is newlist.
Yeah exactly. And If you read this correctly, it means except for
(l)greapadd, Vim will always create a new location list/quickfix list.

Whatever problems some plugins have, needs to be detailed and is not a
problem for this issue here currently.

regards,

Christian

Collaborator

lcd047 commented May 13, 2013

@chrisbra: In that particular context, "except for (l)greapadd" translates to "(l)make" -- which is what I was claiming.

Basically, (l)make creates a stack of loclists (each invocation adds a new one on top), while (l)grep creates only one loclist (subsequent invocations reuse it). Consequently, running :lolder after a series of (l)grep gets you to whatever loclist was there before you started, and life is good, while running :lolder after a series of (l)make makes you go through the stack, which sucks. With (l)make there is no way to go directly to the loclist that was in place before you started, no way to tell where in the stack you are (not from VimL anyway), and even if you're careful to count your steps, there is an opportunity to intermix other things in the run chain which will throw off your count.

Now, from the point of view of usability, both behaviours (last shot vs. stack) make sense, which is why I'm suggesting a global option to control it.

Collaborator

lcd047 commented May 13, 2013

Oh, I see: (l)grep is not the same as (l)grepadd, and there is no such thing as (l)makeadd. Sorry about that.

Collaborator

lcd047 commented Aug 13, 2013

I committed a new branch loclist_count, that makes syntastic reuse loclists whenever it's reasonably safe to do it. This doesn't solve the initial problem, but it does make :lolder and :lnewer more useful in relation to syntastic.

Please note that this only works with Vim 7.4 or later. A relevant bug has been fixed in one of the last pre-releases of 7.4 (the bug could make Vim segfault when loclists are reused).

Contributor

blueyed commented May 2, 2015

@lcd047
Has the loclist_count branch been merged / used? (it does not exist anymore)
Is it https://github.com/scrooloose/syntastic/tree/loclist_state?

Collaborator

lcd047 commented May 3, 2015

@blueyed: Yes, it's controlled by g:syntastic_reuse_loc_lists. But it's off by default (and undocumented) because of #1127. I never got to the bottom of that problem either.

lifepillar pushed a commit to lifepillar/vimrc that referenced this issue Aug 20, 2015

Remove Syntastic 'cause it gets too much in the way.
It has a bad habit of automatically closing my location lists
(see vim-syntastic/syntastic#637).

lorin commented May 2, 2016

Note that this issue breaks a vim-go feature: fatih/vim-go#814

Collaborator

lcd047 commented May 2, 2016

@lorin Please see #1650 instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment