search

alphapapa / magit-todos

Show source files' TODOs (and FIXMEs, etc) in Magit status buffer
GNU General Public License v3.0
727 stars 49 forks source link

XXX+ keyword from hl-note #101

Open arkoort opened 4 years ago

arkoort commented 4 years ago

hl-note considers XXX+ as regexp, so it highlights words XXX, XXXX, XXXXX ans so on. magit-todos considers hl-note's keywords as plain words, so it doesn't show words XXX, XXXX and so on, which are highlighted in code by hl-note, but shows XXX+, which is only partially highlited by hl-note.

alphapapa commented 4 years ago

This appears to be the result of a change in hl-todo (note the package's name). In the version I have installed, keywords are strings, not regexps.

arkoort commented 4 years ago

Of course hl-todo, sorry. It's my fault.

tlotze commented 2 years ago

I just ran into this issue and tried to fix it locally by reconfiguring hl-todo, adding XXX (without +) to the list of recognised keywords (hl-todo-keyword-faces). This didn't have any effect on magit-todos, though. Also, adding a keyword like YYY caused occurrences to be font-locked by hl-todo but didn't make them show up in the magit todos section.

alphapapa commented 2 years ago

@tlotze Please see https://github.com/alphapapa/magit-todos/blob/c5030cc27c7c1a48db52b0134bf2648a59a43176/magit-todos.el#L243

tlotze commented 2 years ago

Ah, ok. I wouldn't have thought to look into the customization of the ignored keywords. But it turns out to be a good starting point for investigating: The point of your "hack" is to re-evaluate magit-todos-keywords-list after its ingredients have been re-set, and it does achieve that for the list of ignored keywords and the description of what keywords to include. However, this description may be a reference to basically any list of strings, and it references hl-todos-keyword-faces in many cases, including mine. The logic of evaluating the actual contents of the resulting keywords list, magit-todos-keywords-list, isn't set up to run after that referenced list has changed. In my case, it was evaluated before the hl-todos keywords were customized, so it never picked up my XXX and YYY additions but always went with the package defaults. So unless there is a way to watch arbitrary variables using change hooks or something, which I am not aware of, the best way will be to remove your hack, move the logic for evaluating magit-todos-keywords-list out of the customization of magit-todos-keywords and into a separate function that is called each time a keyword search is performed, just before the regexes are constructed. This should be cheap enough compared to running the search itself, and look more straight-forward anyways. I just gave it a quick-and-dirty try and it did fix the issue for me.

tlotze commented 2 years ago

See PR #137.

alphapapa commented 2 years ago

Ah, ok. I wouldn't have thought to look into the customization of the ignored keywords.

The code I linked to doesn't customize ignored keywords--it's how the non-ignored keywords are customized.

But it turns out to be a good starting point for investigating: The point of your "hack" is to re-evaluate magit-todos-keywords-list after its ingredients have been re-set, and it does achieve that for the list of ignored keywords and the description of what keywords to include.

It's not a hack--it's a proper use of the Emacs customization system. Many users are unaware that customization options can have runtime setters and that they should therefore not use setq on such options.

However, this description may be a reference to basically any list of strings, and it references hl-todos-keyword-faces in many cases, including mine. The logic of evaluating the actual contents of the resulting keywords list, magit-todos-keywords-list, isn't set up to run after that referenced list has changed. In my case, it was evaluated before the hl-todos keywords were customized, so it never picked up my XXX and YYY additions but always went with the package defaults.

That is confusing, yes. A simple workaround for now would be to set the magit-todos option before the hl-todos option in your Emacs config. You could also call, e.g. customize-set-variable to cause the setter to run again.

So unless there is a way to watch arbitrary variables using change hooks or something, which I am not aware of,

Actually, Emacs does provide variable-change watchers, since a version or two ago, but they aren't intended for this kind of use, hence the customization setter.

the best way will be to remove your hack, move the logic for evaluating magit-todos-keywords-list out of the customization of magit-todos-keywords and into a separate function that is called each time a keyword search is performed, just before the regexes are constructed. This should be cheap enough compared to running the search itself, and look more straight-forward anyways. I just gave it a quick-and-dirty try and it did fix the issue for me.

Something like this may be worth considering, but I'm not sure if it's necessary. As I wrote earlier, the use of the hl-todos option here was just intended for convenience, since the packages seem to complement each other well. It might have been a mistake for me to do that in the first place; it might be better if it were left to users to write a couple lines of code in their config to customize both options similarly.

alphapapa commented 1 year ago

I appreciate the thoughtful discussion here and the PR in #137. I'm going to postpone that until v1.7 since it's a larger change. For now I just adjusted the documentation to mention this issue.