Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check for font-lock-defaults before enabling #55

Merged
merged 1 commit into from
Aug 3, 2020
Merged

Check for font-lock-defaults before enabling #55

merged 1 commit into from
Aug 3, 2020

Conversation

wasamasa
Copy link
Contributor

Currently there is an incompatibility between hl-todo-mode and enriched-mode, hl-todo-mode doesn't do a sufficient check whether font-lock-add-keywords can be used and wipes out the fontification done by enriched-mode, meaning no more pretty colors when opening etc/enriched.txt. This PR fixes this. See also https://debbugs.gnu.org/cgi/bugreport.cgi?bug=42138.

@tarsius
Copy link
Owner

tarsius commented Jul 26, 2020

I am currently trying to figure out why the test for font-lock-mode was there even before this. Unfortunately I didn't say so in respective commit message...

... let's not repeat that mistake. Please explain the why in the commit message.

I think turning the mode should not silently fail. The user should be informed about what's wrong and hl-todo-mode should not be t if the mode has not actually been enabled.

@wasamasa
Copy link
Contributor Author

I think turning the mode should not silently fail. The user should be informed about what's wrong and hl-todo-mode should not be t if the mode has not actually been enabled.

Fair enough, but how? Messages are easily overlooked, the warnings package is suboptimal and throwing an error, even the user-error kind gives you a file-specification error when used from a hook (I suspect it also prevents subsequent hook items or initialization code from being run).

@tarsius
Copy link
Owner

tarsius commented Jul 26, 2020

IMO it is okay to use user-error. Things might not work quite right but that is okay: the user has just been told that an error has occured, so that is to be expected.

@tarsius
Copy link
Owner

tarsius commented Jul 26, 2020

Ah you didn't notice that I did spin out the whitespace fix into a separate commit and then rebased the rest of your commit on top of that and pushed the result, and also added the hl-todo-mode: prefix to the commit message. Please add that prefix and rebase onto master to redo those things.

@wasamasa
Copy link
Contributor Author

OK, I think I finally figured the Git stuff out. Is the error message fine for you?

@tarsius
Copy link
Owner

tarsius commented Jul 29, 2020

Yes.

One last thing, please use unless.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
When using `enriched-mode` in addition to `text-mode` or alike,
`font-lock-mode` will be set, but `font-lock-defaults` will not be.
Therefore to detect whether font-lock can be used safely to add extra
keywords, it's necessary to check both `font-lock-mode` and
`font-lock-defaults`.  See also
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=42138 for extra context.
@wasamasa
Copy link
Contributor Author

wasamasa commented Aug 3, 2020

Forgot to mention I did that change as well.

@tarsius tarsius merged commit 9dc9b49 into tarsius:master Aug 3, 2020
@tarsius
Copy link
Owner

tarsius commented Aug 3, 2020

Thanks!

@tarsius
Copy link
Owner

tarsius commented Aug 4, 2020

I had to revert this because of #56.

@wasamasa
Copy link
Contributor Author

wasamasa commented Aug 4, 2020

Haha, so much about signalling a user error being the cleanest solution.

@wasamasa
Copy link
Contributor Author

wasamasa commented Aug 4, 2020

In any case, the error there is for the globalized mode, so a full solution would need to add some check for the globalized version to only be enabled when it makes sense to do so.

tarsius added a commit that referenced this pull request Aug 7, 2020
`hl-todo-mode' depends on `font-lock-mode'.  Enabling the former in
a buffer that does not use the latter likely removes text properties
that were put in place using some other mechanism.  So now we abort
if this is about to happen.

We cannot just check the value of `font-lock-mode' because apparently
that may be non-nil even though that mode is not actually in effect
or about to be put into effect.  It is unclear why that is so.  It
is also unclear why we already did check for `font-lock-mode' (but
seemingly too late).

At least `font-lock-specified-p' seems to serve our purpose, so use
that.

See #55 and https://debbugs.gnu.org/cgi/bugreport.cgi?bug=42138.
@tarsius
Copy link
Owner

tarsius commented Aug 7, 2020

Haha, so much about signalling a user error being the cleanest solution.

Yes, it worked very well! We immediately got notified about the regression, while I still knew what this is about.

In any case, the error there is ...

... that we both failed to test this properly.

Anyway, I (force) pushed a new version.

@wasamasa
Copy link
Contributor Author

wasamasa commented Aug 9, 2020

Thanks, that seems to do the trick (although I'm not happy with using font-lock-remove-keywords when there's no font-lock support, but eh).

@tarsius
Copy link
Owner

tarsius commented Aug 9, 2020

(although I'm not happy with using font-lock-remove-keywords when there's no font-lock support, but eh).

That cannot happen. It is not possible to turn on the mode and if the mode is never in the turned on state, then it can never be turned off. You can just try again to turn it on, which will fail again.

@wasamasa
Copy link
Contributor Author

Yeah, you're right. Personally I prefer to put something raising an error into a when and letting the code for the non-error case follow it to make this clearer.

@tarsius
Copy link
Owner

tarsius commented Aug 13, 2020

I had to revert my version as well.

I didn't remember this when you opened your pull-request but hl-todo is now actually supposed to be usable in combination with major-modes that don't use font-lock, such as text-mode. In such buffers font-lock is only used for hl-todo.

This does lead to conflicts such as the one with enriched-mode. I have updated hl-todo--turn-on-mode-if-desired accordingly and added a FAQ entry.

tarsius added a commit that referenced this pull request Aug 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants