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

Add recursion-indicator recipe #7241

Merged
merged 1 commit into from Dec 9, 2020
Merged

Add recursion-indicator recipe #7241

merged 1 commit into from Dec 9, 2020

Conversation

minad
Copy link
Contributor

@minad minad commented Nov 21, 2020

Brief summary of what the package does

Show recursion-indicator in the mode-line.

Direct link to the package repository

https://github.com/minad/recursion-indicator

Your association with the package

Maintainer

Relevant communications with the upstream package maintainer

None needed

Checklist

Please confirm with x:

  • The package is released under a GPL-Compatible Free Software License.
  • I've read CONTRIBUTING.org
  • I've used the latest version of package-lint to check for packaging issues, and addressed its feedback
  • My elisp byte-compiles cleanly
  • M-x checkdoc is happy with my docstrings
  • I've built and installed the package using the instructions in CONTRIBUTING.org
  • I have confirmed some of these without doing them

@minad
Copy link
Contributor Author

minad commented Nov 23, 2020

Additional notes for reviewers: This is a really simple package which only marries two different builtin indicators, to show the recursion level (minibuffer-depth-indicate-mode and the brackets around the mode for recursive editing). I like it more to have these two indicators unified since both minibuffer and recursive-edit are recursion levels. Maybe there is also potential for more indicators, like for example indicating a debugger recursion level by showing a small 🪲. But for now the status of the package is the one I've used for a while as part of my init.el and extracted from the init.el as a separate package.

@riscy
Copy link
Member

riscy commented Dec 5, 2020

Hello, just letting you know I see this but will check it more closely on my next cycle since I just looked at one of your PRs. I can quickly mention you may want to revise

"Recursive edit, type C-M-c to get out"

Since I think it's possible someone could modify the global map and bind this chord to something else. M-x exit-recursive-edit could work. Maybe check the function where-is? I'll return to this.

@minad
Copy link
Contributor Author

minad commented Dec 5, 2020

"Recursive edit, type C-M-c to get out"

I think I copied this from how the mode-line-format-modes where this is used as tooltip. I think it is "messed-up" there too. I have to recheck. I agree that this could be improved. (Edit: I checked again and the help text is copied verbatim from here https://github.com/emacs-mirror/emacs/blob/master/lisp/bindings.el#L357).

Two other things you might find during your review and which I thought about improving:

  1. Better face definitions. The problem I had with this is that ideally I would like to inherit only the color from some of the default faces. I don't want to inherit more properties in order to not mess up of some of the other face definitions applied to the mode line.
  2. Maybe add support for customizable characters in order to configure for ascii only?

How do you feel about these points? Point 2 is not very important for me, but I am not happy with point 1.

@riscy
Copy link
Member

riscy commented Dec 5, 2020

Better face definitions. The problem I had with this is that ideally I would like to inherit only the color from some of the default faces. I don't want to inherit more properties in order to not mess up of some of the other face definitions applied to the mode line.

I'd suggest just inheriting the faces without modification (or using them directly). This makes it easy for themes to support the face, and it makes it easy for users to fix problems you might not anticipate with the colors.

I might be missing something, so do you have some examples of other face attributes affecting the mode line? (I can imagine modifications to the height of the face could matter, but that's probably rare.) I guess one attribute you would want to have an effect is :inverse-video.

@minad
Copy link
Contributor Author

minad commented Dec 5, 2020

I think some faces are applied to the whole modeline, so if I inherit from some font-lock face this could mess up some other face properties. I will make few tests if I find something satisfactory. Otherwise I can define my own colors, but I would like to avoid that if possible.

Since you mention height, I think that's one of the things which some of the themes adjust for the modeline.

Do you have more things you would like to have addressed? This is a pretty simple thing over all, so maybe there are no bigger issues.

@minad
Copy link
Contributor Author

minad commented Dec 5, 2020

@riscy I managed to fix the face issue I had by inheriting from both font-lock and mode-line faces. See here minad/recursion-indicator@37fd949

Edit: No I have not, now the background color is a problem (mode-line vs mode-line-inactive). I have to define my own colors it seems :( This is somehow the limitation of the inheritance system. It should be possible to inherit only certain properties.

Edit 2: @riscy There was some unnecessary confusion, I defined a custom face, which inherits from a font lock face. It works now as excpected. I tested multiple themes. See minad/recursion-indicator@f4f003a

@minad
Copy link
Contributor Author

minad commented Dec 7, 2020

@riscy From my side this is ready. Do you agree?

@riscy
Copy link
Member

riscy commented Dec 8, 2020

I'll take a look soon!

@riscy
Copy link
Member

riscy commented Dec 9, 2020

Great -- thanks for this.

@riscy riscy merged commit 2c8ad93 into melpa:master Dec 9, 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