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

Improve g:ale_set_balloons default value #1565

Merged
merged 2 commits into from
May 15, 2018

Conversation

rhysd
Copy link
Contributor

@rhysd rhysd commented May 9, 2018

Vim help says:

m  *+balloon_eval*	|balloon-eval| support in the GUI. Included when
			compiling with supported GUI (Motif, GTK, GUI) and
			either Netbeans/Sun Workshop integration or |+eval|
			feature.
H  *+balloon_eval_term*	|balloon-eval| support in the terminal,
			'balloonevalterm'

It means balloon_eval is for GUI, and balloon_eval_term should be referred on CUI instead. I changed the default value of g:ale_set_balloons considering that.

w0rp
w0rp previously approved these changes May 9, 2018
@w0rp
Copy link
Member

w0rp commented May 9, 2018

I'll have to try running the tests for this myself. It might break some tests on Travis.

@rhysd
Copy link
Contributor Author

rhysd commented May 10, 2018

Build seemed not started. Could you restart the job?

https://travis-ci.org/w0rp/ale/builds/376853433

@rhysd rhysd force-pushed the improve-balloon-support-detection branch from 7f74db8 to 57f0454 Compare May 11, 2018 08:42
@rhysd
Copy link
Contributor Author

rhysd commented May 11, 2018

I force pushed 7f74db8 as 57f0454 for restarting CI job. Nothing was changed (only ammending).

@rhysd
Copy link
Contributor Author

rhysd commented May 11, 2018

I updated test cases also.

gagbo added a commit to gagbo/ale that referenced this pull request May 11, 2018
With the upcoming change in ale_set_balloons default value (see Pull
Request dense-analysis#1565), this check will be useless
@w0rp w0rp merged commit c23acb0 into dense-analysis:master May 15, 2018
@w0rp
Copy link
Member

w0rp commented May 15, 2018

Cheers! 🍻

w0rp pushed a commit that referenced this pull request May 16, 2018
* Guard the ballooneval settings

* Mark main objectives to do to get nice Hover

* Make tweaks to make the tooltip work - See " XXX: comments

* Guard balloon_show call

* Use return instead of finish for functions

* ale#hover#show : Add optional arguments to specify arbtirary position

This change is requested to be able to call the function with mouse
position to enable hover information in vim's balloon

* ale#ballon#Disable : Remove feature guards

* ale#balloon : Show 'ALEHover' output on balloon if no diagnostic found

* ale#hover#HandleLSPResponse : remove the check for cursor position

This check prevented the 'ALEHover in balloon' feature, since mouse
position is almost never cursor position.

* ale#balloon#MessageForPos : Change the return of balloonexpr

balloonexpr evaluation now works even without balloon_show for basic
diagnostics, leaving the balloon_show call to ale#hover#Show, which can
then feature guard the call to avoid errors

* ale#hover#Response : Feature guard balloon_show calls

* ale#hover : always display 'Hover' information in messages

Also add a small comment to warn readers the different outputs the
ale#hover#Show will write to

* {LSP,TS}Response : use only variables from the Response

It is clearer that we only rely on l:options to get the relevant data to
build the LSP Response string

* hover#ShowDetails : fix an issue where not having focus broke balloons

The issue was caused by not using a buffer-specific version of getline()
to cap the value of the column sent in the message to LSP. Therefore a
cursor on column 10 in an inactive window could send a message with
column=0, if the active window had a buffer with too few lines

* {LSP,TS}Response : Remove redundant checks for balloon_show call

With the upcoming change in ale_set_balloons default value (see Pull
Request #1565), this check will be useless

* balloonexpr? : Add a flag to separate hover#Show() calls

The goal of this flag is to make `:ALEHover` calls not pop a balloon
under the cursor, since the user has probably no interest in their
cursor while typing the command

The flag is a default argument which is overridden only in ballonexpr
call of ale#hover#Show, and stays set in the hover_map until the
callback for the LSP handles it.

There are no automated tests for this feature right now, and the nature
of the addition (one optional argument in the API) should make it
transparent to existing tests.

Since the differentiation is now possible, the check for moved cursor
has been put back in ale#hover#HandleLSPResponse

* ale#hover#hover_map : Protect accesses to hover_map

Using get() is safer than trying to access directly with ., as the tests
show.

* Raise timeout to try to get Appveyor happy

* Review : Fix comments

* Review : pass the optional argument 'called_from_balloonexpr' in a Dict

This optional dictionary has documentation just before the function
using it, ale#hover#Show, and allows easier extension in the future.
@w0rp
Copy link
Member

w0rp commented Jul 20, 2018

I unfortunately must disable balloon support for terminals by default, due to a Vim bug which causes very strange behavior when using balloonexpr in combination with ttymouse being set to the wrong setting. We might be able to enable terminal balloon support by default later in newer versions of Vim by checking the patch number with has().

@w0rp
Copy link
Member

w0rp commented Jul 20, 2018

See #1631

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.

2 participants