flycheck / flycheck-pos-tip

Flycheck errors display in tooltip
GNU General Public License v3.0
122 stars 10 forks source link

Use echo area in nongraphical sessions #14

Closed cpitclaudel closed 8 years ago

cpitclaudel commented 8 years ago

Here's an tentative implementation for #11. It adds two fallback functions -tty-show and -tty-hide that are used on TTYs instead of the normal show and hide.

I wonder whether adding these two fallback functions is the best though. Since the show and hide functions are already customizable, we could instead just make the defaults for these two a bit smarter (this would also be a bit more backwards compatible).

Also worthy of discussion: how much trouble do we want to go through to make tty tooltips disappear after the right amount of time? We could save the previous message (in the style of with-temp-message) and restore it from a timer, but is that complexity warranted in the tty case?

Side note: the flycheck-pos-tip-timeout setting is a bit surprising, as it's not enforced independently of the value of flycheck-pos-tip-show-function (the default -show function respects it, of course, but we could enforce it by calling -hide in a timer after flycheck-pos-tip-timeout).

@lunaryorn, opinions? :)

swsnr commented 8 years ago

@cpitclaudel I think we should keep the fallback as simple as possible. I presume anyone who adds this extension has a preference for GUI Emacs anyway; I guess we can live with a less perfect fallback.

Specifically, I'd not bother to restore the old message (the standard error display doesn't either) nor enforce the timeout for the fallback function. If a user wants that they can write their own more sophisticated fallback.

We need a separate fallback function, however; a global default won't cater for the type of the individual frame, which is inherently a frame-local state. Just consider a (frame-less) Emacs daemon that gets a GUI frame and a TTY frame attached: The former will be able to and should use pos-tip, while the latter needs the fallback. We have to make the choice individually for the current frame.

As for flycheck-pos-tip-timeout, I think we should just document (i.e. in the docstring) that the variable only has effect with the default show function, and only for the non-fallback case.

cpitclaudel commented 8 years ago

I've implemented the changes you suggested, thanks for the review :)

We need a separate fallback function, however; a global default won't cater for the type of the individual frame, which is inherently a frame-local state. Just consider a (frame-less) Emacs daemon that gets a GUI frame and a TTY frame attached: The former will be able to and should use pos-tip, while the latter needs the fallback. We have to make the choice individually for the current frame.

Indeed. My thought was just that instead of providing two configurable show functions (one for GUI and one for TTY, defaulting respectively to pos-tip and message), we could provide only one, the default implementation of which would check (display-graphic-p).

That is, instead of putting

(funcall (if (display-graphic-p)
                   flycheck-pos-tip-show-function
                 flycheck-pos-tip-tty-show-function)
               messages)

in flycheck-pos-tip-error-messages, we could put the check in flycheck-pos-tip-show-function.

I don't think it matters too much, however.

swsnr commented 8 years ago

@cpitclaudel Ah, so you mean we wouldn't have a separate option for the TTY fallback, but just a single function that dispatches accordingly, right?

That's a nice idea, too, I didn't think of that. I guess that'd be much simpler.

cpitclaudel commented 8 years ago

@cpitclaudel Ah, so you mean we wouldn't have a separate option for the TTY fallback, but just a single function that dispatches accordingly, right?

Yes, exactly. I'll update the patch accordingly :)

cpitclaudel commented 8 years ago

Patch updated :) I'll squash everything before merging, but I thought it best to keep both commits for now.

swsnr commented 8 years ago

@cpitclaudel Merged, thank you :relaxed:

I've refactored things a bit, though. I removed the customisation options for the show/hide functions. It was a bad idea of mine to introduce these in the first place; if anyone wants to use a different library to show popups they are better of with their own library and minor mode anyway, because this one has many assumptions about the behaviour of pos-tip and X tooltips baked in that'd sooner or later conflict with other popup libraries anyway.

However, I introduced an option to customise the fallback function. I think that this is actually needed, particularly users which use the error list a lot would probably rather use flycheck-display-error-messages-unless-error-list as fallback instead.

cpitclaudel commented 8 years ago

Awesome, that looks great!