Feature/improvements - #16
Conversation
Change: dimmer-exclusion-regexp -> dimmer-exlusion-rexp-list -- Simplifies the process of adding new regexp conditions as new conditions on a list, rather than modifying single mono-regexp. Adds: new user option, dimmer-exclusion-predicates -- List of zero argument functions which are called in turn prior to executing dimmer-process-all. If any of these functions returns true, dimming/undimming of buffers does not occur. For improved mini-buffer support, add window-minibuffer-p to this list, along with appropriate regexp. For improved helm support, add helm--alive-p to this list, along with appropriate regexp.
None of these variables are actually treated as constants, nor is it obvious why we might want to deny other programs or users from binding these variables.
762b17a to
1b24243
Compare
Change: dimmer-config-change-hook and dimmer-command-hook now both call dimmer-process-all asynchronously. Additionally, before running they check for the existence of an unrealized but queued call to dimmer-process-all. This means that we are able to respond to operations that alter the window configuration multiple times with a single call to dimmer-process-all, greatly reducing the amount of work dimmer has to do. Change: on activation, dimmer mode makes a single eager call to dimmer-process-all, to correctly set the initial state. On deactivation, dimmer-restore-all is now called asynchronously in order to account for any unrealized calls to dimmer-process-all.
1b24243 to
275f200
Compare
|
Haven't used dimmer before, but I just loaded up this PR's code, and it seems to work very well! |
|
It would be nice to include some exclusion predicates for common packages. By default, it should be safe to include For helm, there is a variable, For the preds, it could use What do you think? :) |
|
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.] |
|
@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 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. |
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.
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.
You may be right. If we do need to, at least it will be very simple.
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...
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 Thanks. |
|
@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. |
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.
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. :) |
|
@cmccloud what do you think about this issue #15 (comment) ? I see both line exist in your fork. Don't you ecounter this #15 issue ? I opened pr https://github.com/gonewest818/dimmer.el/pull/18/files for that |
|
oh looks like you already mentioned #15. but you have anoter approach to solve, rather than remove both lines. |
|
I checkout your PR and use After reformat my config to match yours. Helm and minibuffer din't get dimmer again. My old config: My new config, mimicking yours: Seems like this line is the key Turn out that the solution not to remove #18 those lines. Great thanks 🎉 |
|
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. |
|
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 |
|
@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- |
|
Yes, I use this patch for about a year without any problem:
with additional:
and this is my config: |
|
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. |
great.
Agree. |
|
Stage one is done, and you should now see 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. |
5705a4c to
6d31ef5
Compare
4 participants
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.