search

gonewest818 / dimmer.el

Interactively highlight which buffer is active by dimming the others.
GNU General Public License v3.0
267 stars 14 forks source link

Dimming and caching improvements #8

Closed purcell closed 6 years ago

purcell commented 6 years ago

These changes help the dimming work better after switching themes (still requires toggling the mode, though), and constructs dimmed colours differently, for better results.

More info in the commit messages.

gonewest818 commented 6 years ago

Thanks! What I think I’ll do (soon as I get a chance) is cherry-pick everything but the desaturation change so we can think a little more about the subjective choice of “dimming” algorithm a little more.

gonewest818 commented 6 years ago

I just cherry picked https://github.com/gonewest818/dimmer.el/pull/8/commits/4c2bf1c9a007905a2e053862ad5b6f7569560b17 and https://github.com/gonewest818/dimmer.el/pull/8/commits/c782b0490f011e87c9a2b1fe5f04d63ba1f27f8c to get those merged in.

gonewest818 commented 6 years ago

...and I think that merge commit just broke your branch. My fault: I hadn't tried the github "merge conflict" editor before, but it apparently stomped on your branch.

gonewest818 commented 6 years ago

So at this point, I've pulled everything in except for your algorithm changes to dimmer-invert-p and dimmer-compute-rgb. I'm so sorry I borked your branch, but at this point it might be cleaner to abandon this PR and then open a new one with only those changes.

gonewest818 commented 6 years ago

Steve, please check out this commit I just pushed on a feature branch. I left a reasonably detailed comment in there to explain.

https://github.com/gonewest818/dimmer.el/commit/31c2179028a9b06778d5fa9c9f03ae4f0834002f

purcell commented 6 years ago

No worries about the conflicts etc - thanks for cherry-picking. I should have just taken out the desaturation changes from this PR to keep things simple. :-)

The WIP changes in your branch look mostly good to me. It presumably breaks caching, though, in that dimmer-compute-rgb is now impure: it receives the foreground colour c, but then computes a result which depends on the background colour of the default face.

gonewest818 commented 6 years ago

Well, at least the cache key now looks like “fg-bg-frac” to avoid an obvious caching problem. It’s functional enough that I’m using that branch in my config to see how it feels.

I’ve tried running with no zenburn theme, using only the emacs default white bg, and other than I hate those colors it seems to work as advertised.

Tempted to bump up the fraction to 0.4 or 0.5 so that there is an obvious effect when you are first trying the mode.

If you’re willing to switch to the branch I’d appreciate any comments on how it’s working for you?

gonewest818 commented 6 years ago

I've been working with my personal Emacs config pointing at the dimming-in-cielab-space branch for the past 4 days, so far everything looks and works as I expect. Do you mind testing my branch to see if it works for you too? Thanks

purcell commented 6 years ago

Yep, that works pretty nicely for me too.

gonewest818 commented 6 years ago

I think we've got everything covered: your caching improvements from this PR mixed with the revised dimming algorithm in my branch.

I merged that branch to master. These features should be available on MELPA by now.

Thanks again for the contribution!

purcell commented 6 years ago

Great! I'm enjoying this library - thanks for a good job. Will no doubt suggest more tweaks if they occur to me.

gonewest818 commented 6 years ago

:thumbs up:

On Jan 23, 2018, at 12:26 AM, Steve Purcell notifications@github.com wrote:

Great! I'm enjoying this library - thanks for a good job. Will no doubt suggest more tweaks if they occur to me.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or mute the thread.