Skip to content
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

Prevent search results from leaking into the terminal #63

Merged

Conversation

lesguillemets
Copy link

Ref. mileszs#18

When vim is running on terminal, :Ag command works as expected, but meanwhile, all the search results are printed on the console. This is painful especially when handling large number of hits.

Adding 6 lines to Ag#ag, this commit hopefully fixes that by saving the user-set values of t_ti and t_te and setting both to temporally, just beforeagis called, then restoring those afterwards. It works the same way asAg#aghandles&grepprg currently.

@losingkeys
Copy link
Collaborator

Thanks! This seems to work well. Looking at the help for t_ti I couldn't really understand what it's doing. Could you explain it/link to an explanation? This isn't necessary for the merge, I'm just curious :).

The only bit of review I have for you is that the rest of the local variables are prefixed with l: (even though that's implied if it's not used). So if you don't mind adding that to the function-local variables in this pull request I'd be happy to merge it. Thanks again!

@lesguillemets
Copy link
Author

Hi, thanks for the comment.

About t_ti : I'm sorry, actually, I am not sure either.. I just followed @kassio 's comment on the issue mentioned above. Setting t_ti= and quitting vim leaves blank lines on the terminal, which seems a little relevant, but omitting the t_ti part in the pull req doesn't seem to change the behaviour. So perhaps that part should be removed. Could you wait for a few days while I try to figure out?

l: prefix: I missed something I should never have. I'll fix that when I've leaned whether t_ti is necessary or not. Thanks.

Speaking of l:, i noticed grepprg_bak and grepformat_bak on line 51-52 in autoload/ag.vimdidn't have prefixes. Is it all right as they are?

@losingkeys
Copy link
Collaborator

Yeah, no rush on accepting this. I usually leave my PRs open for a couple
of weeks for comments anyway.

As far as the l: prefixes.. They should either be used as much as possible
or as little as possible (for consistent style). I think for this project
they're being used as much as possible, so if they're missing it's probably
a bug. If you don't mind including those in your PR I'd appreciate it :)

On Wednesday, June 25, 2014, Yosh notifications@github.com wrote:

Hi, thanks for the comment.

About t_ti : I'm sorry, actually, I am not sure either.. I just followed
@kassio https://github.com/kassio 's comment on the issue mentioned
above. Setting t_ti= and quitting vim leaves blank lines on the terminal,
which seems a little relevant, but omitting the t_ti part in the pull req
doesn't seem to change the behaviour. So perhaps that part should be
removed. Could you wait for a few days while I try to figure out?

l: prefix: I missed something I should never have. I'll fix that when
I've leaned whether t_ti is necessary or not. Thanks.

Speaking of l:, i noticed grepprg_bak and grepformat_bak on line 51-52 in
autoload/ag.vimdidn't have prefixes. Is it all right as they are?


Reply to this email directly or view it on GitHub
#63 (comment).

@lesguillemets
Copy link
Author

OK, I'll include them then. Thank you :)

@lesguillemets
Copy link
Author

:h xterm-screens is related. But I still can't figure out whether t_ti is necessary or not, as I don't see changes in the behaviour of ag.vim. Sorry...

@brianpeiris
Copy link

+1 on this. I can't help but notice the flashing every time I :Ag not to mention the occasional run-away 1000+ result output.

@jsborjesson
Copy link

Any reason why this hasn't been merged yet?

@losingkeys
Copy link
Collaborator

I'm still unsure what t_ti does. I need to at least test it locally.. and hopefully add something to the docs about it.

@jsborjesson
Copy link

Alright! I'm also perplexed by it, but I think there might be some info in
the ack.vim issue about it.
On Nov 19, 2014 5:19 AM, "Josh Hoff" notifications@github.com wrote:

I'm still unsure what t_ti does. I need to at least test it locally.. and
hopefully add something to the docs about it.


Reply to this email directly or view it on GitHub
#63 (comment).

@jpalumickas
Copy link

Any news about this ?

@losingkeys
Copy link
Collaborator

It looks like those options just set different options in your terminal (and it's a terminfo thing?). I'll read it more closely and try it again and try to get this merged. @lesguillemets did you ever figure out for sure what those options are doing?

@Numkil
Copy link

Numkil commented Dec 18, 2014

I have been reading the vim manual because the results of ag leaking into my terminal annoys me haha. I believe the variables do this:

t_ti and t_te both contain a terminal specific code string which vim uses to signal the underlying terminal to go into and out of termcap mode respectively. Temporarily setting them to null will make vim/terminal to not go into and out of termcap mode, and thus not in a mode where it will print anything on the screen during a cmd. These variables exist only because not every implementation of a termcap able terminal is the same. Please correct me if I am understanding this completely wrong.

It seems like a hacky solution but hacky solutions are quite common in vim so I'm not too worried.
source: http://vimdoc.sourceforge.net/htmldoc/term.html

@losingkeys
Copy link
Collaborator

Ok, I still don't fully understand what "termcap mode" means (from the vim docs), but at least this discussion is here as a starting point in case someone finds a bug. I just tested it lightly and it works for me. Thanks for your patience!

losingkeys added a commit that referenced this pull request Dec 20, 2014
…o_terminal

Prevent search results from leaking into the terminal
@losingkeys losingkeys merged commit 8241b73 into rking:master Dec 20, 2014
@jsborjesson
Copy link

This works perfectly for me now, great! 👍

ches pushed a commit to ches/ag.vim that referenced this pull request Feb 7, 2015
Fix q mapping for location list commands
brock8503 added a commit to brock8503/ack.vim that referenced this pull request Oct 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants