Feature/improvements

[cmccloud]

Two sets of changes to take a look at - the first extends the ways in which dimmer can be configured by the user, and should allow for better minibuffer support, and helm support, closing issues #10 and #15.

The second makes dimmer mostly asynchronous, which helped catch lots of instances where the selected buffer wasn't updating until the post-command-hook fired. Feel free to play with both, but they go a long way to improving how dimmer behaves on my system in most cases.

[alphapapa]

Haven't used dimmer before, but I just loaded up this PR's code, and it seems to work very well!

[alphapapa]

It would be nice to include some exclusion predicates for common packages.

By default, it should be safe to include active-minibuffer-window, as it's built in.

For helm, there is a variable, helm-active-p, but no function, so maybe there could also be a list of variables which act as predicates.

For the preds, it could use (when (fboundp pred)) in case the user doesn't have the package installed, and for the variables, it could simply do (bound-and-true-p var) for each one. This way we could support packages like Helm, Magit, etc, and it would work fine if the user didn't have them installed.

What do you think? :)

[alphapapa]

I just found what may be an issue in this PR: When I have the same buffer open in two windows, side-by-side, typing becomes very slow, using lots of CPU. As soon as I disable dimmer-mode, behavior returns to normal.

[Edit: Oops, I'm not sure this is what's causing it. Please disregard for now.]

[cmccloud]

@alphapapa Thanks for the feedback. The code which runs the predicates already uses fboundp, the branch is still active though so depending on when you pulled it you might not have the changes, but you can see it here.

I also like the idea of adding sane defaults to account for both built in and commonly used packages, but I thought that would be a decision better left to @gonewest818, given that there are so many places where one might want an exclusion (which-key popups, hydra popups, company-box, any kind of help window that isn't selected, etc). For the time being I am not convinced it's worth it to add another category of checks; adding (lambda () helm-alive-p) seems easy enough to me. Also, for what it's worth, helm--alive-p is an internal function defined in helm.el. Maybe it's new if you don't have it. It's not part of the public API, but I had mentioned it simply to show that dimmer was now working with helm.

Finally, regarding the possible performance problem you're seeing, let me know if you find out anything more.

Edit: I really don't mind adding a list of variables to check against either to be honest, but I would probably want to see at least a few more instances of where they would be useful. Just as an example, the logic in helm--alive-p is a bit more complicated than just checking the value of the variable.

[alphapapa]

I also like the idea of adding sane defaults to account for both built in and commonly used packages, but I thought that would be a decision better left to @gonewest818, given that there are so many places where one might want an exclusion (which-key popups, hydra popups, company-box, any kind of help window that isn't selected, etc).

Right, the problem is that, for users who have a lot of packages, that is a laborious process to figure out how to add a predicate for each one. It would be good if we could do that in one place. Of course that wouldn't be perfect, and might sometimes stop working if one of those packages changed, but I think it would be better than leaving each user to figure it out themselves for tens of packages. And many users aren't elisp programmers, anyway, so they wouldn't know where to start. If we do it well with the customize interface, it would be easy as checking boxes, and they could be enabled by default to "DTRT" automatically.

For the time being I am not convinced it's worth it to add another category of checks; adding (lambda () helm-alive-p) seems easy enough to me.

It is, but that can't be added in the customize UI, and it's generally not a good idea to use lambdas for hooks (which this basically is), because they can't be removed as easily.

Edit: I really don't mind adding a list of variables to check against either to be honest, but I would probably want to see at least a few more instances of where they would be useful. Just as an example, the logic in helm--alive-p is a bit more complicated than just checking the value of the variable.

You may be right. If we do need to, at least it will be very simple.

Also, for what it's worth, helm--alive-p is an internal function defined in helm.el. Maybe it's new if you don't have it. It's not part of the public API, but I had mentioned it simply to show that dimmer was now working with helm.

Thanks, that might work well, but looking at the docstring, I'm not sure if it would always do what I want. It probably will, but I'll have to test it...

Finally, regarding the possible performance problem you're seeing, let me know if you find out anything more.

Will do. I think it may have been a false alarm. I tested it by typing random letters quickly until the CPU spiked and Emacs seemed to hang, then used pkill -SIGUSR2 emacs to look at the backtrace, and I saw some smartparens functions in the list. Then I realized my version of smartparens was a few years old, so I upgraded it to the current version. I'm guessing it had nothing to do with dimmer or your PR.

Thanks.

[cmccloud]

@alphapapa If you're interested in putting together some of the useful defaults, please do! I've gone ahead and given you push access to my fork of the repo if you want to consolidate your efforts here.

I happen to think that dimmer is going to be one of those libraries that users have to customize to their own preferences, to go back to a very simple example, what should the default behavior be for the help window? It's easy enough when help-window-select is set to t, but otherwise it isn't clear to me what the 'right thing' to do is here. Should help windows always be left 'undimmed', or do we need more complicated logic than that?

In any event, I am happy to see someone else interested in improving this. Take care =)

Edit: I also hadn't really given much thought to how to best allow configuration from the customize menu, and if you're right about the difficulty in adding functions via the interface, then I would be much more inclined to add a list of variable values to check against.

[alphapapa]

what should the default behavior be for the help window? It's easy enough when help-window-select is set to t, but otherwise it isn't clear to me what the 'right thing' to do is here. Should help windows always be left 'undimmed', or do we need more complicated logic than that?

Hm, well, I would prefer that 1) help windows would not be dimmed, because I won't be typing into them, and I don't need any help distinguishing it from the other buffer; and 2) when the help window is selected after running such a command, the other window (I usually use 2, one for code and one for the help buffer) would also not be dimmed, because I'll likely be reading the code in that buffer while looking at the help buffer, and I'd rather it not be dimmed, so it's easier to read.

Does that make sense? I wouldn't want to impose that behavior on users if it's not commonly wanted, but if it is, I wouldn't want them to have to figure out what predicate to write and add to the list to make it behave that way.

If you're interested in putting together some of the useful defaults, please do! I've gone ahead and given you push access to my fork of the repo if you want to consolidate your efforts here.

Thanks, I'll accept the invitation, but I'm not sure when or if I'll add anything, since I have several other projects I'm working on right now, and this is pretty low on my list at the moment (I haven't actually started using it full-time yet). If I do, I would probably just stick to major and obvious ones, like Helm, Ivy/Counsel/Swiper...and I'm sure there are others, but that's all I can think of at the moment. :)

[azzamsa]

@cmccloud what do you think about this issue https://github.com/gonewest818/dimmer.el/issues/15#issuecomment-386820673 ?

I see both line exist in your fork. Don't you ecounter this https://github.com/gonewest818/dimmer.el/issues/15 issue ?

I opened pr https://github.com/gonewest818/dimmer.el/pull/18/files for that

[azzamsa]

oh looks like you already mentioned https://github.com/gonewest818/dimmer.el/issues/15. but you have anoter approach to solve, rather than remove both lines.

[azzamsa]

I checkout your PR and use :load-path in use-package. I still have Helm buffer dimmed. So I try to dig your config.

After reformat my config to match yours. Helm and minibuffer din't get dimmer again.

My old config:

(use-package dimmer
  :defer 1
  :config
  (setq dimmer-exclusion-regexp "^\*helm.*\\|^ \*Minibuf-.*\\|^ \*Echo.*")
  (setq dimmer-fraction 0.50)
  (dimmer-mode t))

My new config, mimicking yours:

(use-package dimmer
  :defer 1
  :load-path "~/emacs-packages/dimmer.el/"
  :config
  (setq dimmer-exclusion-predicates '(helm--alive-p window-minibuffer-p))
  (setq dimmer-exclusion-regexp-list
        '("^\\*[h|H]elm.*\\*" "^\\*Minibuf-[0-9]+\\*" "^.\\*which-key\\*$"))
  (setq dimmer-fraction 0.50)
  (dimmer-mode t))

Seems like this line is the key (setq dimmer-exclusion-predicates '(helm--alive-p window-minibuffer-p)). When I removed this, I get my minibuffer dimmed. But not Helm, maybe because helm ignored by dimmer-exclusion-regexp-list. But when I remove dimmer-exclusion-regexp-list and have only (setq dimmer-exclusion-predicates '(helm--alive-p window-minibuffer-p)) it works well, both helm and minibuffer. TIL, I didn't even know this expression (helm--alive-p window-minibuffer-p)

Turn out that the solution not to remove https://github.com/gonewest818/dimmer.el/pull/18 those lines. (setq dimmer-exclusion-predicates '(helm--alive-p window-minibuffer-p)) never mentioned in https://github.com/gonewest818/dimmer.el/issues/15.

Great thanks :tada:

[azzamsa]

Previously, I had problem with buffer switching, I suspect it comes from helm. I have some seconds lag when switching buffer, even I have to move my focus to other i3wm container, then to emacs again, to make it works. I don't know, I will try what you call "The second makes dimmer mostly asynchronous" hope that solve my long standing problem.

[azzamsa]

In i3wm, when you switch to other container then switch to emacs again, your minibuffer get dimmed too.

I stil need to remove these from your changes

(add-hook 'focus-in-hook 'dimmer-config-change-hook)
(add-hook 'focus-out-hook 'dimmer-dim-all)

[gonewest818]

@cmccloud and friends. I apologize for neglecting this PR but perhaps better late than never... are you still using these changes? Everything is reliable? thanks-

[azzamsa]

Yes, I use this patch for about a year without any problem:

with additional:

and this is my config:

(use-package dimmer
  :defer 1
  :load-path "~/emacs-packages/dimmer.el/"
  :config
  (setq dimmer-exclusion-predicates '(helm--alive-p window-minibuffer-p))
  (setq dimmer-exclusion-regexp-list
        '("^\\*[h|H]elm.*\\*" "^\\*Minibuf-[0-9]+\\*"
          "^.\\*which-key\\*$" "^*Messages*" "*LV*"
          "transient"))
  (setq dimmer-fraction 0.50)
  (dimmer-mode t))

[gonewest818]

Ok, what I'm doing right now is rebase and squash this pull request in two parts. First part is the reworking of the exclusion regexp list and predicate functions. Second part will be the asynchronous timers. I'm separating these for documentation purposes because (as you've seen), I may need to step away from this codebase and want things to be clear when I get back.

The regexp changes will break users of the MELPA release because the user configuration will have changed. So I want to let that stabilize and make sure people are happy before adding the second part of the PR.

[azzamsa]

I'm separating these for documentation purposes because (as you've seen), I may need to step away from this codebase and want things to be clear when I get back.

great.

So I want to let that stabilize and make sure people are happy before adding the second part of the PR.

Agree.

[gonewest818]

Stage one is done, and you should now see dimmer-exclusion-regexp-list and dimmer-exclusion-predicates in the latest build available from MELPA.

Because of the way I rebased and squashed those commits this pull request now conflicts with master. But please keep this branch as-is, so I can cherry pick the remaining commits I need.