-
-
Notifications
You must be signed in to change notification settings - Fork 130
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Report Success/Failure in the quickfix title #313
base: master
Are you sure you want to change the base?
Conversation
Been using this for a while with good results. I would have merged it some time ago, but |
Just to be clear, are you thinking:
|
Undecided. My initial instinct was something like |
I am okay to rethink the |
What's the use case for showing handler and pid information in the title after the process has completed? Programmatically it is used to continue making sure that we uniquely identify lists in the quickfix history so that we can properly update them, but what does a user do with that information at that point? If it's just for uniquely identifying quickfix lists, we could use the id of the list or set the context to achieve the same thing, and just show the exit status in the title instead. |
PID and adapter are in the title for continuity with the start message, and they're in the start message because they're useful for understanding what's going on. Continuity could be fulfilled by the PID alone though, so that opens things up a bit. I wouldn't even say continuity is a hard requirement, but I do like that if you run the same command several times in a row, each title is unique, helping to reduce confusion. We're currently in the process of phasing out Vim 7 support, but even when we get to an 8.0 minimum version, that's not enough to guarantee support for the quickfix ID or context. |
Okay, that all makes sense, thank you! What do you think of a format like:
Also: should this be how the postfix is always formatted, or just for the quickfix title? |
Ngl I hate that One thing I will say is that if we stick with |
I'm not attached to the exit code, I just thought it would be useful- I don't mind ditching it.
Haha good call, of course it is! |
For example, instead of (tmux/12345), show (12345/Success), or (12345/Running) if the command hasn't finished yet. This is helpful in cases where it is difficult at a glance for a user to tell whether their command succeeded just from reading the output. This is updated in all cases where a postfix is added to command in messages to the user, including the quickfix title. For updating quickfix lists, the title up to the '/' character in the postfix is used.
fc9747b
to
1d891fc
Compare
Sorry I didn't notice there was a merge conflict! I think I've resolved it correctly (removed the added space before |
Still not ready to act on this, but I can assure you I consider this mandatory for the next stable release, as it's impossible to tell if the job adapter is finished without it, assuming you miss the status message. A merge conflict won't block this. |
I'll just drop by for a +1 |
I still use the tmux adapter the majority of the time, but rest assured whenever I use the job adapter this continues to bother me!
One advantage of the long delay is enough time has passed that I think I'm willing to cave on this. I'm pushing a change to require the quickfix id for asynchronous dispatch to see if anyone complains. |
Per this comment, I've added the status label part of the message to the quickfix title. This label is only added when the command completes, and when updating the quickfix list only the initial part of the title (before the status label) is used to check against the current quickfix list, so updates to the current quickfix list should still work as expected.
The status label is currently only added under the same circumstances that the normal "Success"/"Failure" message is shown.