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

Modify mode-line lighter customization #86

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

sellout
Copy link
Contributor

@sellout sellout commented Aug 5, 2024

This adds a new customizable variable, envrc-lighter-function that sits between envrc-lighter and envrc-STATUS-lighter. This allows overriding envrc-lighter without relying on internal definitions (envrc--status).

The other benefit of this is that any customization is likely to want to set all three envrc-STATUS-lighter, with them being mostly the same, with a small conditional part (as you can see from both envrc--default-lighter and my lambda below). While this variable is a risky-local-variable, I believe that envrc-STATUS-lighter are also risky, because they can also contain :eval, no?

If envrc-lighter-function is not customized, the existing envrc-STATUS-lighter variables should continue to work as they have.

In future, I think this could be the only -lighter customizable variable.

I like my minor modes to be pretty minimal, so I use this like

(setq envrc-lighter-function
      (lambda (status)
        `(:propertize "🗁" ; looks like GH’s font doesn’t have a glyph for “open folder” 
                      face
                      ,(pcase status
                         ('on 'envrc-mode-line-on-face)
                         ('error 'envrc-mode-line-error-face)
                         (_ 'envrc-mode-line-none-face)))))

This adds a new customizable variable, `envrc-lighter-function` that sits
between `envrc-lighter` and `envrc-STATUS-lighter`. This allows overriding
`envrc-lighter` without relying on internal definitions (`envrc--status`).

The other benefit of this is that any customization is likely to want to set all
three `envrc-STATUS-lighter`, with them being mostly the same, with a small
conditional part (as you can see from both `envrc--default-lighter` and my
lambda below). While this variable is a `risky-local-variable`, I believe that
`envrc-STATUS-lighter` are also risky, because they can also contain `:eval`,
no?

If `envrc-lighter-function` is not customized, the existing
`envrc-STATUS-lighter` variables should continue to work as they have.

In future, I think this could be the _only_ `-lighter` customizable variable.

I like my minor modes to be pretty minimal, so I use this like
```elisp
(setq envrc-lighter-function
      (lambda (status)
        `(:propertize "🗁"
                      face
                      ,(pcase status
                         ('on 'envrc-mode-line-on-face)
                         ('error 'envrc-mode-line-error-face)
                         (_ 'envrc-mode-line-none-face)))))
```
Copy link
Owner

@purcell purcell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I figured people would just write their custom function and set envrc-lighter to (:eval (my-lighter-function). Part of me thinks there's no point changing custom vars in order to "unwrap" that function, but in practical terms I don't think anyone is going to set the lighter to a constant string, for example, so I'm +1 on this PR. If we're doing that, I'd also happy to just go ahead and remove envrc-lighter and mark it obsolete, plus remove the various envrc-STATUS-lighter vars. Would you like to amend the PR to do that too?

envrc.el Outdated Show resolved Hide resolved
@purcell
Copy link
Owner

purcell commented Aug 6, 2024

P.S. Sorry that CI is a bit broken currently, I still haven't figured out what's wrong with the failing tests.

Co-authored-by: Steve Purcell <[email protected]>
@sellout
Copy link
Contributor Author

sellout commented Aug 6, 2024

tl;dr: This is basically just rambling, no real information here other than “I’ll obsolete the oher vars”.

Thanks! I figured people would just write their custom function and set envrc-lighter to (:eval (my-lighter-function). Part of me thinks there's no point changing custom vars in order to "unwrap" that function, but in practical terms I don't think anyone is going to set the lighter to a constant string, for example, so I'm +1 on this PR.

An alternative I meant to mention in the description is just making envrc--status non-internal (envrc-status). And then setting envrc-lighter directly makes more sense. I also feel like exposing envrc--status is an API improvement, perhaps with an envrc-status-change-hook so users can make other decisions based on envrc (although I have no immediate use case for this).

If we're doing that, I'd also happy to just go ahead and remove envrc-lighter and mark it obsolete, plus remove the various envrc-STATUS-lighter vars.

I think removing envrc-STATUS-lighter is the real goal of this PR. Historically, I had configured the lighter like

(defun sellout--envrc-lighter (status)
  "Return a lighter for the provided envrc STATUS."
  `(:propertize "🗁"
                face
                ,(pcase status
                   ('error 'envrc-mode-line-error-face)
                   ('on 'envrc-mode-line-on-face)
                   (_ 'envrc-mode-line-none-face))))

(use-package envrc
  :custom
  (envrc-error-lighter (sellout--envrc-lighter 'error))
  (envrc-none-lighter (sellout--envrc-lighter 'none))
  (envrc-on-lighter (sellout--envrc-lighter 'on))
  :hook (after-init . envrc-global-mode))

Which I prefer over

(use-package envrc
  :custom
  (envrc-lighter '(:eval (sellout--envrc-lighter envrc--status)))
  :hook (after-init . envrc-global-mode))

because … I guess just because of envrc--status being internal. But, despite me doing that, I don’t see much of a case for wanting to set the envrc-STATUS-lighters individually. I guess I feel like by setting envrc-lighter directly, I would be bypassing part of system, making envrc-STATUS-lighter useless. So getting rid of envrc-STATUS-lighter just reduces my idiosyncratic mental stress 😅

Another possible benefit of envrc-lighter-function is that instead of having envrc-lighter use :eval, envrc--apply (or something around there) could (setq envrc-lighter (envrc-lighter-function envrc--status)), so that the lighter only gets updated when the environment does, rather than on every mode-line redraw.

Would you like to amend the PR to do that too?

Yeah, I’m happy to.

@sellout sellout marked this pull request as draft August 17, 2024 20:11
This commits to the new function-based lighter.
@purcell
Copy link
Owner

purcell commented Aug 27, 2024

Yeah, I guess I'm not super averse to making envrc--status public, ie. envrc-status. Or – potentially safer for the long run – exposing it via a corresponding function.

Regarding :eval or not, I'd probably be equally happy with the envrc-lighter-function path.

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.

2 participants