cmake-font-lock icon indicating copy to clipboard operation
cmake-font-lock copied to clipboard

Conflict with package rainbow-delimiters

Open tpadioleau opened this issue 5 years ago • 14 comments

Hi, When I use cmake-font-lock along with rainbow-delimiters, cmake-font-lock does not have any visible effect on the buffer. When rainbow-delimiters is disabled I recover correct coloring.

Here is a minimal setup with use-package

(use-package rainbow-delimiters
  :ensure t
  :hook (prog-mode . rainbow-delimiters-mode))

(use-package cmake-mode
  :ensure t)

(use-package cmake-font-lock
  :ensure t
  :hook (cmake-mode . cmake-font-lock-activate))
  1. Would it be easy to fix ?
  2. Have you even considered pushing your changes to the kitware cmake-mode.el ? This is really enjoyable to have this coloring.

Thanks

tpadioleau avatar Apr 21 '20 10:04 tpadioleau

Hi!

Would it be easy to fix ?

I need to investigate what the problem is -- I don't use rainbow-delimiters myself.

Just a quick question: Have you tried to reverse to order? I.e. activate cmake-font-lock first and then activate rainbow-delimiters?

Have you even considered pushing your changes to the kitware cmake-mode.el ? This is really enjoyable to have this coloring.

Yes, I have considered it, but decided against it.

My package is much larger and more complex than that package, and I don't want "dump" that on someone else.

In addition, the cmake-mode.el package isn't that well written and I don't want to invest effort into it. For me, it would make more sense to start from scratch, but at the moment I don't have time for either...

-- Anders

On Tue, Apr 21, 2020 at 12:31 PM Thomas Padioleau [email protected] wrote:

Hi, When I use cmake-font-lock along with rainbow-delimiters, cmake-font-lock does not have any visible effect on the buffer. When rainbow-delimiters is disabled I recover correct coloring.

Here is a minimal setup with use-package

(use-package rainbow-delimiters :ensure t :hook (prog-mode . rainbow-delimiters-mode))

(use-package cmake-mode :ensure t)

(use-package cmake-font-lock :ensure t :hook (cmake-mode . cmake-font-lock-activate))

  1. Would it be easy to fix ?
  2. Have you even considered pushing your changes to the kitware cmake-mode.el ? This is really enjoyable to have this coloring.

Thanks

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/Lindydancer/cmake-font-lock/issues/13, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAYGGQHXHBJN7M5ANWFWTXTRNVYY3ANCNFSM4MNF7MJQ .

Lindydancer avatar Apr 21 '20 12:04 Lindydancer

I need to investigate what the problem is -- I don't use rainbow-delimiters myself. Just a quick question: Have you tried to reverse to order? I.e. activate cmake-font-lock first and then activate rainbow-delimiters?

If rainbow-delimiters-mode is not loaded using hook but manually in a buffer (so after the call of cmake-font-lock-activate function), it seems to work correctly for both packages.

If I do the reverse, load rainbow-delimiters-mode with hook and manually call cmake-font-lock-activate function, the latter seems to work but I loose rainbow coloring of delimiters.

So this suggets a fix like this:

(use-package rainbow-delimiters
  :ensure t
  :hook (prog-mode . (lambda () (unless (eq major-mode 'cmake-mode)
                                  (rainbow-delimiters-mode)))))

(use-package cmake-mode
  :ensure t)

(use-package cmake-font-lock
  :ensure t
  :hook (cmake-mode . (lambda () (progn (cmake-font-lock-activate)
                                        (rainbow-delimiters-mode)))))

tpadioleau avatar Apr 21 '20 13:04 tpadioleau

Hi!

This makes sense to me. cmake-font-lock-active replaces all existing font-lock rules with it's own, unfortunately, this includes the font-lock rules of minor modes like rainbow-delemiter-mode.

Are you satisfied with this? I might add a comment about this in future releases.

-- Anders

On Tue, Apr 21, 2020 at 3:20 PM Thomas Padioleau [email protected] wrote:

I need to investigate what the problem is -- I don't use rainbow-delimiters myself. Just a quick question: Have you tried to reverse to order? I.e. activate cmake-font-lock first and then activate rainbow-delimiters?

If rainbow-delimiters-mode is not loaded using hook but manually in a buffer (so after the call of cmake-font-lock-activate function), it seems to work correctly for both packages.

If I do the reverse, load rainbow-delimiters-mode with hook and manually call cmake-font-lock-activate function, the latter seems to work but I loose rainbow coloring of delimiters.

So this suggets a fix like this:

(use-package rainbow-delimiters :ensure t :hook (prog-mode . (lambda () (unless (eq major-mode 'cmake-mode) (rainbow-delimiters-mode)))))

(use-package cmake-mode :ensure t)

(use-package cmake-font-lock :ensure t :hook (cmake-mode . (lambda () (progn (cmake-font-lock-activate) (rainbow-delimiters-mode)))))

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Lindydancer/cmake-font-lock/issues/13#issuecomment-617175228, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAYGGQCJVLHJK5EYJW6NIPTRNWMSZANCNFSM4MNF7MJQ .

Lindydancer avatar Apr 22 '20 11:04 Lindydancer

Hi! This makes sense to me. cmake-font-lock-active replaces all existing font-lock rules with it's own, unfortunately, this includes the font-lock rules of minor modes like rainbow-delemiter-mode. Are you satisfied with this? I might add a comment about this in future releases.

I am satisfied. Of course if you have a better idea I will take it.

tpadioleau avatar Apr 22 '20 20:04 tpadioleau

Hello again, new question,

Could it be possible to remove this line defining a hook ?

It took me some time to understand why the following setup couldn't work.

(use-package cmake-font-lock
  :ensure t
  :init
  (defun my-cmake-font-lock-activate ()
    "Workaround wrapper of cmake-font-lock-activate."
    (rainbow-delimiters-mode-disable)
    (cmake-font-lock-activate)
    (rainbow-delimiters-mode-enable))
  :hook (cmake-mode . my-cmake-font-lock-activate))

It seems to be related to this hidden hook which runs after mine and is calling font-lock-refresh-defaults under the hood, hence removing effect of rainbow-delimiters-mode. Moreover, as the documentation advices, it should be of the responsibility of the user to add it in the init file.

Thanks

tpadioleau avatar Apr 30 '20 18:04 tpadioleau

Hi!

I see your point of view, but I would like to think about it for a while. Removing it will make it stop working for existing users, if they upgrade the package and haven't got a corresponding line in their init file.

In the meantime, you would use something like (remove-hook 'cmake-mode-hook #'cmake-font-lock-activate) in your init file.

-- Anders

On Thu, Apr 30, 2020 at 8:17 PM Thomas Padioleau [email protected] wrote:

Reopened #13 https://github.com/Lindydancer/cmake-font-lock/issues/13.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Lindydancer/cmake-font-lock/issues/13#event-3290761237, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAYGGQCF3IYHHUN4JLOL46LRPG6FNANCNFSM4MNF7MJQ .

Lindydancer avatar Apr 30 '20 18:04 Lindydancer

Hi! I see your point of view, but I would like to think about it for a while. Removing it will make it stop working for existing users, if they upgrade the package and haven't got a corresponding line in their init file.

Then could you consider adding a comment on this hook in the readme ?

In the meantime, you would use something like (remove-hook 'cmake-mode-hook #'cmake-font-lock-activate) in your init file. .

Indeed this is a simple workaround.

Thank you!

tpadioleau avatar Apr 30 '20 19:04 tpadioleau

This is related with #9. Stefan Monnier suggested that:

One option is to use font-lock-add-keywords instead of setting font-lock-defaults. Or otherwise it should be careful to refresh the keywords after this setting if font-lock-keywords was already set.

Ergus avatar Jun 05 '20 14:06 Ergus

This is a work workaround that fixes the issues commented above.

(use-package cmake-font-lock
  :defer t
  :preface
  (defun my/cmake-font-lock ()
    (let ((auto-refresh-defaults (boundp 'font-lock-keywords)))
      (cmake-font-lock-activate)
      (when auto-refresh-defaults
	(font-lock-refresh-defaults))))
  :init
  (add-hook 'cmake-mode-hook #'my/cmake-font-lock t))

Ergus avatar Jun 05 '20 16:06 Ergus

Hi!

In most font-lock packages I've written I've used font-lock-add-keywords and font-lock-remove-keywords, this allows me to model the packages as minor modes that can be enabled and disabled. In fact, I actually implemented the function font-lock-remove-keywords in Emacs back in 1999 when I wrote cwarn-mode.

However, the CMake case is different since I replace the font-lock keywords altogether. The best way I know how to do this is to set font-lock-defaults and call font-lock-refresh-defaults. Of course, if you or Stefan knows of a better way, please let me know.

-- Anders

On Fri, Jun 5, 2020 at 4:34 PM Jimmy Aguilar Mena [email protected] wrote:

This is related with #9 https://github.com/Lindydancer/cmake-font-lock/issues/9. Stefan Monnier suggested that:

One option is to use font-lock-add-keywords instead of setting font-lock-defaults. Or otherwise it should be careful to refresh the keywords after this setting if font-lock-keywords was already set.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Lindydancer/cmake-font-lock/issues/13#issuecomment-639534294, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAYGGQBUZPEFWKOOJX5QIIDRVD67JANCNFSM4MNF7MJQ .

Lindydancer avatar Jun 06 '20 06:06 Lindydancer

Hi!

First, I don't know what problem your code is trying to solve, so I don't know if it would work better that the original code.

I just spotted one thing, the (boundp 'font-lock-keywords) construct will always be true, since there is a global definition of the variable. So the only difference between your code and the original is that you check font-lock-mode before cmake-font-lock-setup is called. Since the function doesn't change the value of the variable, the code will behave exactly like to original code.

I guess that you intended to use something like (buffer-local-p 'font-lock-keywords)? Again, without knowing what problem you are trying to solve it's hard to know.

-- Anders

On Fri, Jun 5, 2020 at 6:45 PM Jimmy Aguilar Mena [email protected] wrote:

(defun cmake-font-lock-activate () "Activate advanced CMake colorization.

To activate this every time a CMake file is opened, use the following:

(add-hook 'cmake-mode-hook 'cmake-font-lock-activate)"

(interactive) (let ((auto-refresh-defaults (or (boundp 'font-lock-keywords) font-lock-mode))) (cmake-font-lock-setup) ;; If this function is called after font-lock is up and running, ;; refresh it. (This does not work on older Emacs versions.) (when (and auto-refresh-defaults (fboundp 'font-lock-refresh-defaults)) (font-lock-refresh-defaults))))

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Lindydancer/cmake-font-lock/issues/13#issuecomment-639622209, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAYGGQCJE2GUR2N5IOROY2LRVEOJ3ANCNFSM4MNF7MJQ .

Lindydancer avatar Jun 06 '20 06:06 Lindydancer

Hi:

The problem is actually that packages like rainbow-delimiters or highlight-numbers should be called before cmake-font-lock-mode because they are order dependent. But usually we enable those packages as prog-mode-hooks while cmake-font-lock is a cmake-mode-hook (which executes before).

According to Stefan: setting font-lock-defaults directly can only be done reliably by the major-mode function itself rather than from the mode hook.

The problem could be considered as being partly on highlight-numbers side as well. Because if cmake-font-lock refreshes the font-lock-keywords after highlight-numbers was enabled then there will be also an issue.

In my original code in #9 I proposed to extend the conditions to call font-lock-refresh-defaultsbecause in my case font-lock-mode was false but global-font-lock-mode was true so this somehow asserts that font-lock-refresh-defaults will be called before enabling other conflicting packages.

(when (and (or font-lock-mode  global-font-lock-mode)
           (fboundp 'font-lock-refresh-defaults))
      (font-lock-refresh-defaults)))

Alternatively the users can add a function to prog-mode-hook that adds rainbow-delimiters or highlight-numbers to font-lock-mode-hookwhich is in my opinion dirtier and looks more like a workaround.

Ergus avatar Jun 06 '20 14:06 Ergus

Hi again,

I see the problem. Of course, packages like highlight-numbers should just work, without the need for additional configuration for the user.

I will give it some thought and see if I come up with a better solution.

 -- Anders

On Sat, Jun 6, 2020 at 4:00 PM Jimmy Aguilar Mena [email protected] wrote:

Hi:

The problem is actually that packages like rainbow-delimiters or highlight-numbers should be called before cmake-font-lock-mode because they are order dependent. But usually we enable those packages as prog-mode-hooks while cmake-font-lock is a cmake-mode-hook (which executes before).

According to Stefan: setting font-lock-defaults directly can only be done reliably by the major-mode function itself rather than from the mode hook.

The problem could be considered as being partly on highlight-numbers side as well. Because if cmake-font-lock refreshes the font-lock-keywords after highlight-numbers was enabled then there will be also an issue.

In my original code in #9 https://github.com/Lindydancer/cmake-font-lock/issues/9 I proposed to extend the conditions to call font-lock-refresh-defaultsbecause in my case font-lock-mode was false but global-font-lock-mode was true so this somehow asserts that font-lock-refresh-defaults will be called before enabling other conflicting packages.

(when (and (or font-lock-mode global-font-lock-mode) (fboundp 'font-lock-refresh-defaults)) (font-lock-refresh-defaults)))

Alternatively the users must add a function to prog-mode-hook that adds rainbow-delimiters or highlight-numbers to font-lock-mode-hook

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Lindydancer/cmake-font-lock/issues/13#issuecomment-640065612, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAYGGQHIAKAG3VRHXKQIP7DRVJDZHANCNFSM4MNF7MJQ .

Lindydancer avatar Jun 07 '20 19:06 Lindydancer

Hi again!

I investigated the hooks that I had at my disposal and found change-major-mode-after-body-hook which is called before all normal mode hooks.

Can you give the attached (rough) patch a try?

One drawback is that it makes it hander to install this package manually, but I guess that very few people do that nowadays. (I haven't updated the documentation yet.)

-- Anders

On Sun, Jun 7, 2020 at 9:02 PM Anders Lindgren [email protected] wrote:

Hi again,

I see the problem. Of course, packages like highlight-numbers should just work, without the need for additional configuration for the user.

I will give it some thought and see if I come up with a better solution.

 -- Anders

On Sat, Jun 6, 2020 at 4:00 PM Jimmy Aguilar Mena < [email protected]> wrote:

Hi:

The problem is actually that packages like rainbow-delimiters or highlight-numbers should be called before cmake-font-lock-mode because they are order dependent. But usually we enable those packages as prog-mode-hooks while cmake-font-lock is a cmake-mode-hook (which executes before).

According to Stefan: setting font-lock-defaults directly can only be done reliably by the major-mode function itself rather than from the mode hook.

The problem could be considered as being partly on highlight-numbers side as well. Because if cmake-font-lock refreshes the font-lock-keywords after highlight-numbers was enabled then there will be also an issue.

In my original code in #9 https://github.com/Lindydancer/cmake-font-lock/issues/9 I proposed to extend the conditions to call font-lock-refresh-defaultsbecause in my case font-lock-mode was false but global-font-lock-mode was true so this somehow asserts that font-lock-refresh-defaults will be called before enabling other conflicting packages.

(when (and (or font-lock-mode global-font-lock-mode) (fboundp 'font-lock-refresh-defaults)) (font-lock-refresh-defaults)))

Alternatively the users must add a function to prog-mode-hook that adds rainbow-delimiters or highlight-numbers to font-lock-mode-hook

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Lindydancer/cmake-font-lock/issues/13#issuecomment-640065612, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAYGGQHIAKAG3VRHXKQIP7DRVJDZHANCNFSM4MNF7MJQ .

Lindydancer avatar Jun 07 '20 19:06 Lindydancer