Inherit hydra faces from hydra-default-face

[eigengrau]

I don’t really know what I’m doing with my elisp, but maybe this is useful to you anyway, with some brushing up. I was a bit bummed out because my hydras were always using the same font as the buffer contents, whereas I like to use a different font for mode-line and echo area; so this adds proper faces for all the other parts of the hydra, not just the heads.

One corner that had to be cut is that the offsets of face-properties are not adjusted when padding is added in a string format. This works around the problem by setting the default-face only after the format is done. To not clobber any face-assignments done earlier (for the heads), it goes over the string and only propertizes the parts that are not already propertized.

[eigengrau]

I guess the latter point would complicate the tests somewhat, unless there’s some way of doing the propertization before the actual format at run-time.

[abo-abo]

Thanks a lot. But your change is quite large: I can accept only changes <=15 lines in total from a person without a on Emacs Copyright Assignment. I have an assignment, so that I'm allowed to have hydra in GNU ELPA. So you need to get one too if you want to contribute a large change to hydra (or any Emacs-related project, like org-mode or CEDET or Emacs itself). It's not hard, see description.

In case you already have an assignment, I can fix up the tests which are failing and merge.

[eigengrau]

Hi! Ah, I didn’t know ELPA had the copyright assignment thing. If the changes seem useful to you I can fill out the assignment no problem; my only concern was that they would make the tests too ugly for your tastes. It might take me a bit to get the forms posted, since I’m currently on travels; but if they snail mail it I suppose it takes a while anyway.

Thanks for making hydra!

[abo-abo]

If the changes seem useful to you

The changes are useful, and the tests can be updated to be less ugly, and no one except me looks at them anyway. Besides, it would be great to get more PR from you in the future, also for other ELPA projects.

Thanks for making hydra!

You're welcome.

[eigengrau]

I recently realized I completely lost track of the copyright assigment process I had started back then. So, with only 2 years of latency, I now went through with the remaining step and have my assignment. Not surprisingly, the patch doesn’t apply anymore, but if you’re still interested, I can look into that.

[abo-abo]

Thanks. Please rebase on top of master and update. I'll have a look.

[eigengrau]

I simplified things somewhat. For now, all this does is add hydra-default-face from which all other faces inherit, and also uses this face for all text apart from the heads.

As for the tests, do you have some sort of setup to diff expanded hydra definitions with the earlier ones? I reckon one could replace the target strings by up-to-date macroexpanded hydras, but without inspecting the diffs one could miss potential regressions.

[abo-abo]

Thanks. Could you also look into the failing tests?

[eigengrau]

Hello Oleh. I did glance over the test suite, hence my question. A lot of the tests hard-code the exact propertized string they expect. So, when anything about the UI is changed, those tests will break. One would need to regenerate the target strings completely. However, this runs the risk of bypassing the whole idea of testing. Hence I was wondering how you handled that in the past.

[abo-abo]

when anything about the UI is changed, those tests will break

That's the idea. The current UI is what most users expect now. So if any commit changes the UI, it's for a good reason, and is tracked by updated UI tests.

Unfortunately, there's manual work involved in rewriting the tests (easy), and checking that that they still make sense (harder).

The hard part isn't that hard though, just insert the old and the new version of the relevant string into a fundamental-mode buffer, and see if they still look similar enough.

When I rewrite the test, I point my cursor like this:

|(macroexpand

and call lispy-eval-and-insert. That command auto-formats the eval result into a consistent representation.

[eigengrau]

Unfortunately, there's manual work involved in rewriting the tests (easy), and checking that that they still make sense (harder).

Okay, thanks! My guess is the best way to validate new targets is to diff them with the old ones. I will look into this.

[eigengrau]

So I updated the failing tests. I couldn’t exactly reproduce your previous indentation though, even when tweaking lisp-indent-offset, so the commit adds whitespace changes. To make this less jarring, when touching a deftest, I reindented and staged the whole test. For the least amount of noise one probably needs to diff using --word-diff --ignore-all-space.

[abo-abo]

Looks wrong here. The doc part of the result should be data, not code. So this cl-loop should be evaluated in defhydra, not at top-level.

[eigengrau]

This is what I initially referred to as «corner that had to be cut». Hydras may contain something like %20`foo. Now consider this:

(with-current-buffer-window
 "baz"
 nil
 nil
 (insert
  (format
   (propertize "Magic number: %20s"
               'face
               '(:background "red"))
   42)))

Because of the padding added, 42 will not be propertized. This is why I came to believe that faces must be set at run-time, not when compiling the format string. Do you see another way to handle these cases?

[abo-abo]

Do you see another way to handle these cases?

The whole string can be propertized with some base face, excepting the keys.

Example:

(defhydra hydra-toggle (:color pink :hint nil)
  "
_a_ abbrev-mode:       %`abbrev-mode
_d_ debug-on-error:    %`debug-on-error
_f_ auto-fill-mode:    %`auto-fill-function
_h_ highlight          %`highlight-nonselected-windows
_t_ truncate-lines:    %`truncate-lines
_w_ whitespace-mode:   %`whitespace-mode
_l_ org link display:  %`org-descriptive-links
"
  ("a" abbrev-mode)
  ("d" toggle-debug-on-error)
  ("f" auto-fill-mode)
  ("h" (setq highlight-nonselected-windows (not highlight-nonselected-windows)))
  ("t" toggle-truncate-lines)
  ("w" whitespace-mode)
  ("l" org-toggle-link-display)
  ("q" nil "quit"))

Generates:

(set
 (defvar hydra-toggle/hint nil
   "Dynamic hint for hydra-toggle.")
 '(concat
   (format
    "%s abbrev-mode:       %S
%s debug-on-error:    %S
%s auto-fill-mode:    %S
%s highlight          %S
%s truncate-lines:    %S
%s whitespace-mode:   %S
%s org link display:  %S
"
    #("a"
      0 1 (face hydra-face-pink))
    abbrev-mode
    #("d"
      0 1 (face hydra-face-pink))
    debug-on-error
    #("f"
      0 1 (face hydra-face-pink))
    auto-fill-function
    #("h"
      0 1 (face hydra-face-pink))
    highlight-nonselected-windows
    #("t"
      0 1 (face hydra-face-pink))
    truncate-lines
    #("w"
      0 1 (face hydra-face-pink))
    whitespace-mode
    #("l"
      0 1 (face hydra-face-pink))
    org-descriptive-links)
   #("[q]: quit."
     1 2 (face hydra-face-blue))))

Two things can be done to hydra-toggle/hint:

• either replace format with some hydra--format which will add the appropriate face
• or fix the function that uses hydra-toggle/hint to add the appropriate face

I think the second approach should be preferred, the advantage being more flexibility later, and no need to update the tests.

[eigengrau]

I will look into hydra-toggle/hint. However, wouldn’t this equally require some code other than the format to be run at the Hydra’s run-time? The problem with %40`s is that the padding that is added bears neither the properties of the format-string, nor of the fillers, but is completely unpropertized. So at least there, we would have to generate some code (e.g., hydra--format, which wraps regular format and applies faces) — that okay with you? It would certainly be more elegant than running that loop each time though.

[eigengrau]

Actually, I’m not sure if that would be enough… Hm. If the %40`foo is part of the format-string, this is where the unpropertized padding is generated, so handling this would mean there must be some form that has scope over the format and is run at run-time. Handling this without looping over the eval’d format would entail reimplementing a string formatting function (not wrapping the existing one).

[eigengrau]

Note that

(format
   (propertize "Magic number: %20s"
               'face
               '(:background "red"))
   (propertize "42" 'face '(:background "blue")))

…generates:

  #("Magic number:                   42"
    0 14
    (face
     (:background "red"))
    32 34
    (face
     (:background "blue")))
  ))

With no face applied between indices 15 and 31.

[abo-abo]

With no face applied between indices 15 and 31.

Just add logic that takes a string, and replaces the 'default face with 'hydra-default-face.
I.e.:

• if the string region has a face applied to it, do nothing (that's a key's face)
• if the string region has no face applied to it, apply 'hydra-default-face.

This logic can be added to the function that eventually prints hydra-toggle/hint. The data of ``hydra-toggle/hint` doesn't need to change.

[eigengrau]

Just add logic that takes a string, and replaces the 'default face with 'hydra-default-face.

The stumbling block here is that this cannot be done at compile-time, at least not if the Hydra docstring contains something like %40`foo. Do you see what I mean or am I missing something? Whatever happens, this emits the form (format "%40S" …). So unless one generates code around the format, there will be unpropertized whitespace when the Hydra is displayed. In case the user chooses to add a background property to hydra-default-face, there will be parts of the Hydra window that look wrong.

Just add logic that takes a string, and replaces the 'default face with 'hydra-default-face.

This is what I went for by generating the cl-loop form that scopes over the format and applies the face to unpropertized spans. Only it occurs to me this cannot be done before the Hydra is actually shown.

This logic can be added to the function that eventually prints hydra-toggle/hint. The data of ``hydra-toggle/hint` doesn't need to change.

I don’t follow. Consider:

(defhydra foo nil "
_s_ %40(buffer-name)"
    ("s" nil))

The issue is that this will always generate (format "…%40S" …), and, once (and only once) the Hydra is shown, some amount of unpropertized whitespace. This is why the logic that propertizes the unpropertized spans must be invoked at run-time.

[abo-abo]

The stumbling block here is that this cannot be done at compile-time, at least not if the Hydra docstring contains something like %40`foo. Do you see what I mean or am I missing something?

I meant do it at runtime. See how it's done in example:

(defun hydra-toggle/body nil
  "Create a hydra with no body and the heads:

\"a\":    `abbrev-mode',
\"d\":    `toggle-debug-on-error',
\"f\":    `auto-fill-mode',
\"h\":    `(setq highlight-nonselected-windows (not highlight-nonselected-windows))',
\"t\":    `toggle-truncate-lines',
\"w\":    `whitespace-mode',
\"l\":    `org-toggle-link-display',
\"q\":    `nil'

The body can be accessed via `hydra-toggle/body'."
  (interactive)
  (hydra-default-pre)
  (let ((hydra--ignore nil))
    (hydra-keyboard-quit)
    (setq hydra-curr-body-fn
          'hydra-toggle/body))
  (hydra-show-hint
   hydra-toggle/hint
   'hydra-toggle)
  (hydra-set-transient-map
   hydra-toggle/keymap
   (lambda nil
     (hydra-keyboard-quit)
     nil)
   'run)
  (setq prefix-arg
        current-prefix-arg))

The relevant part is:

(hydra-show-hint hydra-toggle/hint 'hydra-toggle)

So hydra-toggle/hint should remain as it is now at compile time. But hydra-default-face should be added at run time by hydra-show-hint.