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

New assignment of emph and non-emph styles #785

Merged
merged 7 commits into from
Nov 28, 2021
Merged

Conversation

dandavison
Copy link
Owner

@dandavison dandavison commented Nov 20, 2021

Fixes #776 cc @unphased @th1000s

This PR implements the suggestion #776 (comment) addressing @unphased's proposal in #776 to change the way emph and non-emph colors are applied.

To make this easier to think about and discuss, I've added a commit to the repo which gives a simple demo of the various emph and non-emph styles, fd9592e:

image

As per #776 (comment), what this branch is intended to do is allow lines where "nothing has happened" to be styled differently from a completely new or completely deleted line. In other words, allow the pink "dodo" line above to be styled differently than the emphasized "albatross" line, as GitHub does.

That is done by changing the code so that the pink dodo line gets minus-non-emph-style, whereas previously it was receiving minus-style. With delta's defaults that will have no effect, since the two styles are identical, but we can differentiate the two styles, e.g. with

[delta]
    features = github-theme

[delta "github-theme"]
    light = true
    pink-style = normal "#ffe0e0"
    dark-pink-style = normal "#ffc0c0"
    green-style = syntax "#d0ffd0"
    dark-green-style = syntax "#a0efa0"

    minus-emph-style = github-theme.dark-pink-style
    minus-non-emph-style = github-theme.pink-style
    plus-emph-style = github-theme.dark-green-style
    plus-non-emph-style = github-theme.green-style

    minus-style = minus-emph-style  # this is the difference;
    plus-style = plus-emph-style  # by default these are equal to *-non-emph

and that config gives rise to the middle panel here:

delta 0.9.1 delta [this branch] GitHub
image image image

So that looks promising.

Here's a more complex example (1973650). The new branch is noisier in this example, because it's emphasizing entirely deleted lines, but then using a non-emph color to fill right.

If anyone is able to offer thoughts on whether this branch is a good direction and what if anything more needs to be done, then that would be great!

delta 0.9.1 delta [this branch] GitHub
image image image

@unphased
Copy link

The new version, especially in the more involved example (thanks for that one, very nice complex example!) is far superior to the shown GitHub output! Kudos! I snagged both versions of your orphaned commits out of GitHub and ran a diff with my char-diff that I described in my thread, here it is for the purposes of edification. I tried a little bit to make a light background but it does not look very good, so I stuck with my usual dark theme...

image

@unphased
Copy link

unphased commented Nov 21, 2021

I do wonder, and let's not take my comment here as creeping the scope of your PR here. From my perspective I'm 100% for the changes.


I think I came up with a point of improvement, a potential deficiency (open debate!) for multi-line groups of changes. Let me illustrate with delta output:

a

I've got to spend a few seconds working out that I need to hop 4 lines downward to find the corresponding green line and see that no chars got added.

The reason for having to hop down by 4 lines instead of 1 line is because all minus-lines in a set of changes get grouped together.

I wonder if this separation is necessary. It sure makes it easier to read the previous code state. But I also worry that there are ambiguities if we try to interleave the lines. We could place the two purple underline lines right next to each other, but I do not know where to place the next one.
I love how my custom diff never has this ambiguity because it just jumbles it all in there, it shows both the begin state and end state and all common characters and you can mentally work out which is which as long as you can mentally block out one or the other bg-color and keep the non-bg-colored lines in. From my perspective by ensuring no duplication of unchanged characters ever appears, it sidesteps all ambiguities arising from attempting to generate some least confusing way to visually present these duplicated unchanged characters.

I worry that after using my solution for so long, I've spoiled myself, I worry that perhaps it simply looks like an incomprehensible jumble to most everyone, while to me it is the most efficient. What I am grateful for is that in writing this out I think I've begun to articulate a sort of proof for why my character diff colorization output is the tersest possible way to present a diff.

I haven't made up my mind yet on whether I want it to be a goal of mine to convince you that we should have this as a possible output. I think I still need to think about what the possibilities are when it comes to syntax highlighting done on the unchanged shared characters.

Base automatically changed from symbolic-style-names to master November 21, 2021 17:27
@dandavison
Copy link
Owner Author

dandavison commented Nov 21, 2021

Thanks @unphased! I think there are two or three different issues here. How about we use this ticket for resolving this question:

Do you think we need a fourth emphasis mode or is there maybe a cleaner way to address the issue I pointed out?

I.e. deciding changes we are going to make to delta's basic unified diff mode (where you have separate removed and added lines), and whether this branch should be merged as-is.

On that topic, I'm a little uncomfortable with the extra visual noise that is introduced when filling right with non-emph color. It's nice that the avocet and albatross lines are emphasized:

delta [this branch with plus-style=plus-emph-style] GitHub
image image

but on the other hand, this branch is producing the left panel when plus-style=plus-emph-style, whereas GitHub users are used to the right panel. Is this really what we want, or the change we want to make in this PR slightly more nuanced than what I have right now?

delta [this branch] GitHub
image image

Regarding your script, that's definitely interesting. So that's essentially a new type of output: neither unified, nor side-by-side, but "merged-line" or something, i.e. there's only one copy of each line. So in that respect it's reminiscent of git's --word-diff output. If you agree with that distinction, would you mind opening a separate issue to discuss the possibility of delta's supporting a "merged-line" output format? Or, shall we use

And then the possibility of re-ordering lines is again a distinct issue I think.

But my top priority right now is -- I'd love your help deciding what exactly is the correct change to unified diff (this branch) to release. As it is currently this branch doesn't make a visual change to the defaults, but is it going to upset some people who are already customizing emph styles?

@unphased
Copy link

unphased commented Nov 28, 2021

Sorry for my delay, I think that I may want to look into this specific question of yours @dandavison further by doing some experiments, but... I'm heavily leaning toward considering github's output a bug and your proposed output superior.

In particular what master-branch Delta and Github share is the main deficiency i highlight in the beginning of my #776 issue. There is one and the same style being applied by Github to the contextual portions of modified lines and wholly added or deleted lines, which is my pet peeve because it breaks the semantic meaning of the background color on a characterwise basis.

I think in the rendering what you've got on wholly new lines is plus-emph on all chars and non-emph till the beginning of the next line.

I could see somebody considering this style transition at the end of a line to be curious/weird/bad.

I think what's behind this is as I suggested earlier an introduction of a fourth style rather than to change all "pure" plus lines to be emph because at least that way it would be easy for a user to configure it to (or even to make Delta behave like this) the way Github looks.

My own aesthetic preference would be to have the darker bgcolor extend all the way to end of line on "pure" lines (the rationale is that the emph'd to-end-of-line maps one-to-one with newline insertion), so lacking the configurability of that is suboptimal. (edit: Not sure this needs configurability at all...)

@unphased
Copy link

unphased commented Nov 28, 2021

What I am unsure about, though, is whether there could be a smarter way to semantically break down the styles, still using only 3, and achieve all of our goals. Have not thought this through. Will give it a first pass:

Edit made later on:
I'm sorry for writing too much. I feel like I want to delete everything and think about it some more and get back to you on exactly what to change. Sorry about that. I think it is too difficult to work out for sure what the change is based on the available information, and I will make an effort to build and run this commit on my machine and get back to you on what's missing. Not actually deleting this post yet because I think there's some stuff in here I might want to refer to later.

begin recap feel free to ignore but I think I also want you to fact-check it?

I think these are the only possibilities (again, focusing on the plus- side of the symmetry without loss of generality) when it comes to types of lines

  • pure added line
  • partial added line (a corresponding removal line exists)

and only in the partial case will emphasis be provided onto the characters inside

  • emphasis applied on chars that got added
  • currently non-emph-style is applied to the unmodified context regions

These 3 cases (with the last 2 under the partial umbrella) correspond to the 3 configurable color styles (which should be set as background colors but not enforced)

This is all consistent with Github and no doubt that output was used as a guide to implement it.

To view it through my lens, we've got

  • added regions
  • unchanged regions

Pure added lines should look the same as portions of lines that were added. Unchanged regions I would want colored (lightly) to indicate whether it is part of a minus-line-context or plus-line-context region.
end recap

So after thinking about it more, I think filling right really should be done based on whether a newline is added there. I'm not sure how the logic needs to work, I think it's still inherently line based so I think this just means always fill right with whatever style is defined as plus rather than always as non-emph.

Side note: I think there are two things with your provided examples that make comparisons a little difficult to reason about:

  1. On the first line albatross -> avocet, Github treats it as a partial diff with the # being recognized as being the same, but Delta actually produces pure change lines.
  2. Your config has minus styles without syntax which serves to actually make the plus styles in this example visually muted, which impacts how the output gets perceived methinks

The following is egregiously off-topic feel free to ignore

Ideally what I'd like to see is if a change on a line is non-complex then it'd be good to have it viewed in squashed mode where no unchanged context regions are duplicated and they gain no highlight (as they are semantically identical to pure unchanged context lines, while more complex changed lines can be displayed as they usually are in Delta/Github, with the partial modification lines shown with one minus-line and one plus-line each.

The above italicized concept is a troublesome one. I wonder if it might be good to keep it in mind when thinking about architecture. The problem with the concept is that there's not an obvious notion or metric by which to measure complexity of change. It could be possible for us to define such a thing and allow configuration of thresholds. I would need to see it and use it for a while in order to determine whether it would be worthwhile at all or simply add to confusion...

The diff in this commit contains sections with all of the following semantics:

minus-style (old line 1)
plus-style (new line 1)
zero-style (line 2)
minus-non-emph-style (old line 4, and unchanged sections of old line 3)
minus-emph-style (deleted word "chaffinch" in old line 3)
plus-non-emph-style (new line 3, and unchanged sections of new line 4)
plus-emph-style (added word "dodo" of new line 4)
Fixes #776

Previously, when a paired plus line had no edits, it received
plus-style. With this commit such a line receives
plus-non-emph-style.

There's no change to unpaired lines (still plus-style) and paired
lines with edits (still a mosaic of plus-non-emph-style and plus-emph-style).

(The above statements hold for minus lines also).

Since *-non-emph-style defaults to *-emph-style, this commit does not
result in any change in output for users using the defaults.
- Factor out a new function update_diff_style_sections
- Use MinusPlus construct more
@dandavison dandavison merged commit a19945f into master Nov 28, 2021
@dandavison dandavison deleted the 776-non-emph-style branch November 28, 2021 23:28
@dandavison
Copy link
Owner Author

My own aesthetic preference would be to have the darker bgcolor extend all the way to end of line on "pure" lines (the rationale is that the emph'd to-end-of-line maps one-to-one with newline insertion)

Me too and I've changed the branch to do that.

@unphased I think I agree with all or nearly all that you wrote. I've merged this branch. Try out the master branch -- I believe that it makes most or all of the things you're proposing for unified diffs possible.

Pure added lines should look the same as portions of lines that were added. Unchanged regions I would want colored (lightly) to indicate whether it is part of a minus-line-context or plus-line-context region.

Right. So to do that basically all you have to do is

    minus-style = minus-emph-style
    plus-style = plus-emph-style

Below is an example theme that does it (I would really like to have examples of themes that work well on dark terminal backgrounds; please let me know if you have some!)

image
[delta]
    features = hoopoe2 consistent-color-semantics

[delta "consistent-color-semantics"]
    # All added/removed content is styled the same, regardless of
    # whether there is unchanged content on the same line.
    minus-style = minus-emph-style
    plus-style = plus-emph-style

[delta "hoopoe2"]
    light = true

    pink = "#ffe0e0"
    dark-pink = "#ffc0c0"
    green = "#d0ffd0"
    dark-green = "#a0efa0"

    minus-emph-style = normal hoopoe2.dark-pink
    minus-non-emph-style = normal hoopoe2.pink
    plus-emph-style = syntax hoopoe2.dark-green
    plus-non-emph-style = syntax hoopoe2.green

@unphased
Copy link

Thank you so much for your work on this project! I will be running master starting from now. Is it true that installation requires nothing other than placing delta into $PATH? How convenient.

@dandavison
Copy link
Owner Author

I will be running master starting from now. Is it true that installation requires nothing other than placing delta into $PATH? How convenient.

Great. Yes that's right, delta is just an executable. There are no man pages or anything else to install.

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.

Can't decide if actually a 🐛: Issue with semantics around emph- and non-emph- styles
2 participants