Consider inheriting the default Magit faces from standard Emacs faces.

[tee3]

Would you consider providing faces by default that inherit from Vc, Diff, and other standard faces? I would imagine it would allow more themes to just work with Magit as long as the theme customized the standard Emacs faces nicely. I noticed when reviewing some of the Magit code that Magit uses this approach for some faces and not for others.

I've been using a theme I created to load after themes that do not support Magit to help Magit's faces blend in better with the current theme. It's called unobtrusive Magit theme and can be found at https://github.com/tee3/unobtrusive-magit-theme. I created this project after submitting pull requests to provide Magit support for several nice themes that did not support Magit already. The unobtrusive Magit theme just automates what I was doing manually in those projects.
This sort of thing is essential with themes built in to Emacs since they will likely not support an external package in a Emacs built-in theme. It works relatively well for my needs, though it does not properly override faces that have been customized by the current theme.

I was attempting to submit it to MELPA (melpa/melpa#6784) and they asked if I had raised an issue here, so here I am.

To be clear, I'm fine with whatever decision you make here. I can use it locally or from MELPA (if they accept it) or without any of that if you think this is a useful idea. If you like the idea, I could probably submit a pull request, but it might take me a bit to understand how all the defface works.

Finally, Magit is a great piece of software, thank you for your effort in continuing to maintain it.

[purcell]

This makes sense to me if practical, though I'm sure @tarsius has thought about it since some faces are already inherited. Perhaps it was just a lot of work to figure out a more extensive set of :inherit settings like you have, though... :-)

[tarsius]

Would you consider providing faces by default that inherit from Vc, Diff, and other standard faces?

other standard faces yes -- vc and diff not so much. The primary goal is for no information being lost due to bad/incomplete theming. Things looking good is of secondary importance.

The problem is that magit, unlike vc and diff, highlights the current file or hunk. If the author of a theme themes the diff faces then they might not check what effects that has on magit. They might not even realize that the situation is more complicated in this case.

For that reason magit's diff faces don't derive from diff and related faces: that way the theme author has to actively decide to theme magit and will look at the effects within magit. They might even read my essay on the horrors of theming magit faces: https://magit.vc/manual/magit/Theming-Faces.html.

[tee3]

I read through your document and I understand what you are saying. It is, however, a lot to take in and may be why so many themes do not customize Magit.

Do you think it is possible to implement your guidelines in code given the Vc and Diff values by doing some math on the colors provided? I tried to think through it, but it would take some time to work through the theming guidelines and turn that into code.

If you have serious objections of the "unobtrusive Magit theme" going into MELPA due to the concerns raised in the link above, could you raise them there in the MELPA ticket? I would rather not to contribute to the maintenance burden of Magit by making this widely available if you feel strongly that it will do so.

I will continue to use it for my own work either way as I rarely use the features of Magit that this simplistic theme does not expose. I will also ensure that I add documentation that reflects our discussion here so that anyone who finds it via GitHub will understand the risks of using it.

[tarsius]

I am not sure. Giving a definite answer would require quite a bit of research on my part, which is something I don't want to do right now. Sorry.

It's possible that it would actually be best to inherit the faces despite all I have said but again that isn't something I would want to rush.

[tarsius]

Maybe it would be best to add your package to Melpa, with a big fat warning that it might make things worse given some base themes but not others. That way users would at least have opted-in the potential breakage.

[tee3]

Sorry.

No need to apologize, I really appreciate you taking the time to discuss it a bit.

Maybe it would be best to add your package to Melpa, with a big fat warning that it might make things worse given some base themes but not others. That way users would at least have opted-in the potential breakage.

This is where I've been leaning as well. I did add a warning to the README referencing your theming document and this issue. You can see the commit with the warning here.

I feel like the unobtrusive Magit theme would be a great place to experiment and work out what that code might be and, if it works out and is solid, we could potentially work it into Magi. I will likely chip away at it further over time anyway.

[tarsius]

I feel like the unobtrusive Magit theme would be a great place to experiment and work out what that code might be and, if it works out and is solid, we could potentially work it into Magi. I will likely chip away at it further over time anyway.

Sounds good.