search

larstvei / Focus

Dim the font color of text in surrounding paragraphs
485 stars 16 forks source link

Improve responsiveness #28

Closed florommel closed 5 months ago

florommel commented 5 months ago

I was experiencing performance issues with focus-mode, especially when scrolling. This PR consists of two mitigations:

  1. Cache the current thing to focus: This is what @alphapapa suggested in #23. Since there is still a lot of expensive stuff done in focus-move-focus, it doesn't bring that much of a performance improvement. Nevertheless, I think caching is the right thing to do here.

  2. Delay updating the focus via an idle timer: This improves responsiveness a lot. Of course, the downside is that the focus update is delayed until Emacs is idle. However, with the current default idle delay (focus-update-idle-delay) of 0.1s, this is barely noticeable in my opinion. Maybe with this, also #23 would run efficiently but I have not tested. If you do not want to use the idle timer by default, we could set focus-update-idle-delay to nil which restores the old behavior, but allows the user to optionally use the idle-timer feature.

larstvei commented 5 months ago

Hi, and thanks for contributing!

The caching looks fine and I'd be glad to merge it.

Having focus update when idle like a good feature for mitigate the performance issues. However, I prefer not to have it on by default. Though I don't use the package that much myself, I have never actually experienced any performance issues (that is not to say that there aren't any, or that they shouldn't be addressed). With the idle timer on, it feels a lot less responsive.

The type for focus-update-idle-delay has the wrong type (:type 'number) as it can also be nil. You can fix this by having the type be a choice, but maybe it's better to check if focus-update-idle-delay is zerop?

If you fix the type and change the default, then I'll merge it 👍

florommel commented 5 months ago

Thanks for the feedback!

I fixed the type of focus-update-idle-delay and the changed the default value to nil.

Regarding zerop: Currently, there is a small difference between 0 and nil for focus-update-idle-delay. A value of nil results in the default undelayed behavior. Currently, a value of 0 would still delay the update until Emacs is idle -- but execute it immediately when Emacs gets idle. So, if there are pending user inputs, they are handled first. I don't really know if that distinction is necessary or even perceivable and I do not have a strong opinion about it.

larstvei commented 5 months ago

Perfect, and thank you for the contribution! 🎉