pitkali / pos-tip

Show tooltip at point
37 stars 14 forks source link

Use the tooltip face #7

Closed wasamasa closed 9 years ago

wasamasa commented 9 years ago

This is a bit embarassing.

After my last pull request I've run into a weird Emacs bug. I'm using emacsclient which spawns an invisible, non-graphical server frame if it doesn't exist already, then a graphical one. Initialization of the package happens at the former step, this leads to pos-tip initializing with brightblue and brightblack as colours which however aren't valid in graphical emacs instances. Debugging this led to a suspicious amount of Emacs crashes. Making the theme set this variable wasn't helpful either.

I've therefore reconsidered my approach and went for something simpler. Considering it's only themes customizing these values and every popular Emacs theme out there sets these colors to the same values as the tooltip colors, it makes not much sense keeping them separate. That's why this pull request gets rid of the two customization variables and instead looks up the right colors from the tooltip face when needed.

pitkali commented 9 years ago

I'm rather hesitant about this change. For one, the 2 commits there should be one. That Whoops thing looks terribly in logs ;-)

The other thing is that curious thing I found in solarized theme, which does set tooltip to something, but notes that it does that for zencoding, as changing these values for OS tooltips does not work (which is indeed the case). I'm also wary of any users having set these values to something entirely separate.

I really think the correct fix for this is to revert default values of the 2 customisations to the ones previously there. If you really want to use tooltip, I suggest allowing user to set these to nil, which would then make the rest use tooltip face.

wasamasa commented 9 years ago

The commits can be still squashed, so that shouldn't be really a problem. In the worst case I'll just open a new pull request with a single commit.

To enable customized tooltip colors, one needs to deactivate the system tooltips (as these have to be customized outside Emacs for GTK at least). This can be done with (setq x-gtk-use-system-tooltips nil).

I assumed this pull request would be welcomed considering the premise of pos-tip, namely allowing one to position a tooltip (not changing tooltip colors, mind you). Reverting the initial colors would work, but be a bit silly considering they're the same colors as set by the tooltip face, so looking those up would make it just work for any theme, no matter whether it did customize the tooltip face or not.


As for the customization concerns, you can get a rough idea by using the Github search. Stuff I've found so far is either bundling pos-tip.el (as dependency), by people already using a theme that customizes the tooltip face appropriately and who customize it to match or by people customizing both to look the same.

This is of course just a rough metric, so beware. You could mark the variable as obsolete to make it display a warning and still let it do its thing if you consider this change as something people wouldn't discover themselves in the Changelog.

pitkali commented 9 years ago

Of course they can be squashed. It just won't work with automatic merging, which is to say, I'm not going to automatically merge it in a state like that using the temptingly handy "Merge pull request" button. Opening a new pull request seems redundant, though. I do know how to rebase, squash, and all that.

I'm not really interested in how to disable system tooltips, and not just because I'm actually using a mac port, and had nothing to do with GTK version for quite a while. It just serves as an example of the area where problems might arise. As a general rule I assume the users are dumb/unconcerned/do not want to know stuff. I assume that even emacs extension developers using pos-tip directly or extensions that depend on it do not want to be bothered with pos-tip API changes, and would rather spend time on their own extensions rather than update their emacs configuration to deal with this, for example.

Changing tooltip colours seems to be one of the core features of the library. Of course, I suppose when it is used by all those auto completion libraries, they provide their own faces anyway, rather than using defaults. And in case of using native tooltips, it might not work, depending on the platform port you're using. It works on mine, to the best of my memory.

However, the real issue I have with this is that removing these is an unnecessary backward-incompatible change. I don't think you have provided sufficient argument to surprise users of the library like that. So don't revert to default colours: use nil by default, which is indeed quite sensible, and move fetching tooltip face properties to point of use. That's what the crux of the issue is, isn't it? Fetching tooltip face properties at library initialisation causes failures in a certain use case related to using daemon mode. I can even do that myself, and perhaps this time I would be able to really do it over the weekend, rather than next month ;-)

By the way, I don't expect most users to read change logs. I don't think they should need to. Perhaps this is a consequence of spending most time on applications used by non-technical users, but I think a much better goal to aim for is to allow users to just keep updating the library, and using it as before. Of course, everything has its limits, but I don't think that in this case the cost of maintaining this backward compatibility is high enough to justify removal of those variables.

wasamasa commented 9 years ago

Do I understand you correctly that you'd prefer adding nil as customization value which would cause a runtime lookup of the tooltip face to happen and the question is just about whether to make nil or a color the default?

If yes, then I guess I'd be happy with that, no matter the outcome. The choice should only affect people who haven't customized these colors at all, but are relying on them.

pitkali commented 9 years ago

Yes. Add nil as customisation value, and make it default. I actually like the idea of using tooltip as default, I just don't like the idea of removing customisation point here.

wasamasa commented 9 years ago

Sounds good. Shall I push more commits on this branch or do you want to implement the necessary changes yourself?

pitkali commented 9 years ago

Feel free to add more commits. I don't feel the need to do it myself, or desire to do so today.

Thank you for your contribution anyway.

pitkali commented 9 years ago

Hmm, I was waiting for email notification about additional changes, while they were sitting here all this time. That's rather unfortunate. Sorry for the delay.

pitkali commented 9 years ago

Merged and pushed, with minor documentation, description improvements.

wasamasa commented 9 years ago

Nice, thank you!