highlight hunks without overriding semantic background colors #1380
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Make `magit-insert' and `magit-put-face-property' always use overlays. Previously it was possible to use text properties which theoretically gives a small performance boast. But the option to do the latter was more confusing than helpful and I doubt that anybody actually made use of it, especially because doing so would force the use of something other than the background color for highlighting the current section.
This option served to purposes: It allowed disabling highlighting of the current section. The ability to do this was added because of performance considerations, not as an aesthetic choice. The effect on performance is not noticeable and so it is okay to drop support for this. It is still possible to have the visual effect of not highlighting the current section by unsetting all face properties or setting these property to the same value it has in the `default' face. Once a theme has set a face property it is not possible for user to unset that property again, she can just change it to something else, but not nothing. This was a problem until recently, but now this can be worked around by setting that property to the same value it has in the `default' face.
Create just one section "Section Utilities" by absorbing the directly
preceding, one-definition section "Section Utilities" and moving some of
the definitions previously in "Section Actions" here. The latter is the
reason we are doing this; we have to move these things up so that we can
use the `magit-section-case' macro in code which is defined earlier than
where it used to be defined. The `magit-{*}-at-point' functions are
moved elsewhere, putting them below "Section Api" was a mistake in the
first place.
For color names that consist of a single word use lowercase. For color names that consist of multiple words use CamelCase.
Rename `magit-diff-file-header' to `magit-file-heading', and rename `magit-diff-hunk-header' to `magit-hunk-heading'. Drop the "diff" from the name of "file" face because it is also used outside of diffs, i.e. for untracked files. Drop it from the name of the "hunk" face too for consistency. Replace "header" with "heading" because we only use the term "header" for those one-line headers that appear at the top of some buffers. For the part of a section that remains visible even in collapsed state we use "heading". Consistently using distinct terms helps avoid confusion.
For most headings the heading face is only used for the actual text but not for the following newline. This makes a difference when the background color is set for the heading face. For hunk headings we also propertize the newline and by default also set the background color, which makes diffs more readable because this clearly separates different hunks from one another. Until now we also did that for files because a diff may contain several files and separating them clearly makes sense the same way it does for hunks. We did not actually set the background color by default, but users who wanted to visually separate files the same way that hunks are, just had to do so. However if the background color actually is specified and spans the hole line then the region is not visible when selecting multiple files. This would be very confusing so remove the ability to set the background for the complete file heading line.
Hashes should not stand out because, from a human perspective, they are usually not very interesting. So use some shade of grey instead, to make them *less* visible. Previously some red was used which was also semantically wrong; red is the color used for e.g. *removed* lines, while the main even in a commit's life is its *creation*.
So that is different from magit-log-author.
Previously `magit-section-highlight' inherited from `secondary-selection'. Doing so had the advantage that for most themes this face is sufficiently distinct form other faces without requiring any further customization. The default background color for `secondary-selection' was selected so that it works regardless of what changes were made to other faces. This is done by using a background color which unlikely to be used as background or foreground color for any face in most reasonable themes. The disadvantage, and result, of this is that this face always looks very out of place. For `secondary-selection' this works (not the "secondary"). But for `magit-section-highlight' it doesn't; we use that face all the time, and we want Magit to look okay when using the default faces (dark or light).
No longer inherit from `diff-{added,removed,context}' because these
faces only define the background color. It is better to also define the
foreground color because other faces that overlay the text propertized
with these faces might also set the background color (which would shadow
the background colors of the diff faces, but not their foreground colors).
This is specially problematic because in Magit the current section is,
by default, highlighted using the background color. That will be fixed
in the next commit, but the same applies to the region.
Generally speaking, highlighting the current section using the background color means that any text in that section which also sets a background color different from that of `default' loses its special appearance. For things like ref names we work around this by using overlays. But we didn't do that for added, removed, and context lines in diffs, even though the default values of the respective faces did only set the background color but not the foreground color. This was fixed by some third-party color themes by instead using the foreground color instead of the background color to differentiate lines in diffs. This commit makes such kludges unnecessary. It is still possible to do it the old way, but it is not recommended, because doing so makes it much harder to come up with faces for hunk refinement which both look good and are sufficiently visible. (Using bold is not a good option; it's easy to overlook, and for whitespace it doesn't have any effect at all.) So hunks are now treated as a special case when highlighting; instead of `magit-section-highlight' the new faces `magit-hunk-heading-highlight', `magit-diff-added-highlight', `magit-diff-removed-highlight', and `magit-diff-context-highlight' are used. Furthermore we don't use overlays to apply these faces but the face text property. Doing it this way avoids problems with `diff-refine-hunk'. If we did use overlays these overlays would in some cases override the refinement overlays.
|
See #1220. |
This was referenced Aug 8, 2014
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Generally speaking, highlighting the current section using the
background color means that any text in that section which also sets
a background color different from that of
defaultloses its specialappearance. For things like ref names we work around this by using
overlays.
But we didn't do that for added, removed, and context lines in diffs,
even though the default values of the respective faces did only set the
background color but not the foreground color. This was fixed by some
third-party color themes by using the foreground color instead
of the background color to differentiate lines in diffs.
This commit makes such kludges unnecessary. It is still possible to do
it the old way, but it is not recommended, because doing so makes it
much harder to come up with faces for hunk refinement which both look
good and are sufficiently visible. (Using bold is not a good option;
it
s easy to overlook, and for whitespace it doesnt have any effect atall.)
So hunks are now treated as a special case when highlighting; instead of
magit-section-highlightthe new facesmagit-hunk-heading-highlight,magit-diff-added-highlight,magit-diff-removed-highlight, andmagit-diff-context-highlightare used.Furthermore we don't use overlays to apply these faces but the face text
property. Doing it this way avoids problems with
diff-refine-hunk.If we did use overlays these overlays would in some cases override the
refinement overlays.
Unfortunately this means that existing themes and user customizations break. I hope that be the time
nextis merged intomastermost popular themes have been adjusted. Early adopters can help with that by adjusting their favorite theme now. I recommend you don't submit your changes just yet, as I might some other adjustments to faces before the merge happens. I have already started to adjust the zenburn theme. There are two variants: https://github.com/tarsius/zenburn-emacs/tree/magit-next and https://github.com/tarsius/zenburn-emacs/tree/magit-next-oldschool. The later shows how to make a theme again look as it used to; doing that is not recommended. Treat the "light default theme" as reference implementation:I think that looks pretty good. The "dark default theme" is a bit ugly: