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

Readability issues in recently added features, +config file? #47

Closed
0x5c opened this issue Aug 16, 2023 · 16 comments
Closed

Readability issues in recently added features, +config file? #47

0x5c opened this issue Aug 16, 2023 · 16 comments

Comments

@0x5c
Copy link
Contributor

0x5c commented Aug 16, 2023

Some of the new highlight features in recent-ish versions are creating readability issues for me (details further down). I see the value in those features, so I wouldn't advise removing them. I think the best approach is to make them configurable, either through enabled/disabled switches, or through the ability to specify the style to use for each "token" (for lack of a better word). This last approach would also double as a way to customise colours for colourblindness accessibility and personal preference. Configuration could be done using a config file and/or environment variables, the former being more user-friendly.

Renamed files highlighting

I like this way of highlighting renames, but dark grey on red/green is very hard to read. This one seems to simply be a bug; the foreground black being turned into "bright black" by the bright/bold modifier on the whole filename. Should be fixable by setting the foreground of the highlight area to no-bold. I'll try to make a fix for that one.
image

"Only additions" highlight

With this highlight, it becomes very hard to read the before line (especially with partial red-green colourblindness), which for me means that I can't easily parse the diff since I need both the + and - lines.
image

Added/removed files lowlighting

Similar to the problem with "only additions" highlight, this one makes it harder for me to process that a file has been deleted/added since the lack of the other line removes the comparison point. It's slightly less pronounced than "only additions" because of the colour, but still causes difficulty parsing.
image

@walles
Copy link
Owner

walles commented Aug 16, 2023

Renamed files highlighting

I think your terminal mistreats the ANSI SGR codes.

The ANSI SGR codes say "Bold, then invert, then set green foreground color". It says nothing about grey. And green-on-black inverted should IMO become black-on-green, not grey-on-green (which is really ugly just as you point out).

To verify this, I tried before-after.txt in both iTerm2 and the macOS terminal and with hexdump -C.

If we look at the first line of the hexdump -C output...

/tmp $ hexdump -C before-after.txt | head -2
00000000  1b 5b 31 6d 2d 2d 2d 20  2f 74 6d 70 2f 1b 5b 37  |.[1m--- /tmp/.[7|
00000010  6d 1b 5b 33 31 6d 62 65  66 6f 72 65 1b 5b 32 37  |m.[31mbefore.[27|
/tmp $

... we can see that the ANSI SGR codes are 1, 7 and 31. Wikipedia tells us these are bold, inverse and red respectively, and here's what this looks like in iTerm2.

Note how the foreground becomes black when inverting, not grey as in your terminal.

before-after-iterm

For this particular issue, I think you should report it to your terminal vendor.

@0x5c
Copy link
Contributor Author

0x5c commented Aug 16, 2023

I think your terminal mistreats the ANSI SGR codes.

The ANSI SGR codes say "Bold, then invert, then set green foreground color". It says nothing about grey. And green-on-black inverted should IMO become black-on-green, not grey-on-green (which is really ugly just as you point out).

"Bold" is font bold and/or brighter colour depending on the terminal implementation and there is no actual standard as to which one to use so every terminal is going to do something different.

... we can see that the ANSI SGR codes are 1, 7 and 31. Wikipedia tells us these are bold, inverse and red respectively, and here's what this looks like in iTerm2.

There's also no standard saying the bold attribute doesn't stay on the foreground after reverse video is applied, it is not defined as a singular "event" but an effect that applied to any text between it and the next bold-resetting code (0, 22, 21). Some terminals consider that reverse video switches the bold attribute over to the background, others keep it on both foreground and background.

The only explicit and somewhat widely (but not universally) supported way to specify foregrounds and backgrounds with any mix of bright (not bold) and normal colours is the so-called "aixterm bright colours"; range 9x for bright foreground and range 10x for bright background.

@walles
Copy link
Owner

walles commented Aug 17, 2023

Makes sense, wanna do a PR for having the highlighted file name parts not being bold?

0x5c added a commit to 0x5c/riff that referenced this issue Aug 18, 2023
@walles
Copy link
Owner

walles commented Aug 18, 2023

"Only additions" highlight

In this case...

line-line2

... the -line line contains no information. Nothing was removed, and the added 2 is highlighted on the next line.

Since the -line line contains no information it's being lowlighted to help the user ignore it.

Color blindness

Since I don't have any color blindness, I can't judge the decorations from that perspective.

How obvious (or not) is it to you that the 2 is highlighted in inverse green? The rest of the line has "default" color.

@walles
Copy link
Owner

walles commented Aug 18, 2023

And on the topic of color blindness, if I need:

  • One default color
  • One added color
  • One removed color

Do you have any suggestions for which colors I could use for added / removed?

@0x5c
Copy link
Contributor Author

0x5c commented Aug 18, 2023

"Only additions" highlight

In this case...

line-line2 ... the `-line` line contains no information. Nothing was removed, and the added ` 2` is highlighted on the next line.

Since the -line line contains no information it's being lowlighted to help the user ignore it.

This is mostly by habit, since I am used to reading diffs with red - lines and green + lines, in the same way one gets to reading text in a given language (ie: english). I use riff to get highlighting for that language but when things like -+ lines are highlighted in ways that hide some of that information, I'm faced with a different "language".

In detail, this is what happens when glancing at a "only additions" line:

  • I don't grok I'm looking at a change since the - line is effectively invisible (this is the partial colourblindness part)
  • The impression that the line is context is reinforced by the lack of the usual + line green colouration (and the little + at the start is completely missed since that's not what I normally look for with the near-ubiquitous red/green syntax highlight)
  • And then complete confusion sets in when the "context line" has an highlighted new segment

Color blindness

Since I don't have any color blindness, I can't judge the decorations from that perspective.

How obvious (or not) is it to you that the 2 is highlighted in inverse green? The rest of the line has "default" color.

Colour blindness is very varied; it can affect different cone types on the retina, and can be anywhere from mildly degraded perception/differentiation of some colours all the way to not being able to perceive an entire segment of the rainbow.

What I have is partial degradation of red cones, which makes dark red almost identical to black, very bright (near hex FF) yellow and green hard to distinguish, and teal essentially grey. Most of these are not an issue on the terminal since I can just adjust the colours, but red with the faint attribute is unavoidably dark red*.

The line is very clearly normal colour with a black on green reverse-video segment

*My terminal allows customising faint colours, but it can't help much unless I make it very close to normal red

And on the topic of color blindness, if I need:

  • One default color
  • One added color
  • One removed color

Do you have any suggestions for which colors I could use for added / removed?

With what I have red/green highlighting for diffs still makes more sense so It's hard to personally recommend alternative colours. But a place to look is Github's colourblind themes.
image

@walles
Copy link
Owner

walles commented Aug 19, 2023

Added/removed files lowlighting

I'm unsure how to best illustrate that a file has been added.

There are two lines for this, the +++ and the --- /dev/null lines. They should be formatted in a way that makes it obvious that:

  • This is a file header
  • The file was just added

Since all other file headers come with a highlighted +++ part, it makes sense for this one to do that as well.

For the --- /dev/null part, I see some options. More suggestions welcome:

  1. Lowlight the whole line. That's what's being done now. Indicates nothing was removed, but removes the --- and +++ headers combo indicating that "here comes a file".
  2. Lowlight only /dev/null. Indicates nothing was removed, by lowlighting /dev/null, which effectively means "nothing", but keeps the highlighted --- header indicating together with +++ that "here comes a file".
  3. Just highlight the whole line like git does natively. Makes "here comes a file" obvious, but less obvious that the file is new.

Do you have any other suggestions? Out if these three I think I might like 2 the best, followed by 1.

lowlight--- lowlight-dev-null highlight-all

@0x5c
Copy link
Contributor Author

0x5c commented Sep 12, 2023

2. Lowlight only /dev/null. Indicates nothing was removed, by lowlighting /dev/null, which effectively means "nothing", but keeps the highlighted --- header indicating together with +++ that "here comes a file".

Of these this seems to me like the best option, but considering there's already a highlight for filename changed, maybe it could be just that, or combined with 2 (keep ---/+++ bold, lowlight /dev/null filename, with red/green highlight filename)

@0x5c
Copy link
Contributor Author

0x5c commented Sep 12, 2023

If a style system is added, I could see this being solvable with a style for

  • ---/+++ (combined or separate)
  • old filename
  • new filename
  • /dev/null

@walles
Copy link
Owner

walles commented Sep 27, 2023

  1. Lowlight only /dev/null. Indicates nothing was removed, by lowlighting /dev/null, which effectively means "nothing", but keeps the highlighted --- header indicating together with +++ that "here comes a file".

Of these this seems to me like the best option, but considering there's already a highlight for filename changed, maybe it could be just that, or combined with 2 (keep ---/+++ bold, lowlight /dev/null filename, with red/green highlight filename)

Wanna make a lowlight-/dev/null-only PR?

I feel that having red/green in this case might be a bit misleading since the file name hasn't actually changed.

@0x5c
Copy link
Contributor Author

0x5c commented Sep 27, 2023

Wanna make a lowlight-/dev/null-only PR?

Sure I can have a look

I feel that having red/green in this case might be a bit misleading since the file name hasn't actually changed.

Diffs are traditionally highlighting removed lines as red and added lines as green even when not part of a modification pair, just like riff does, so I don't feel like it'd be much different with file names

@classabbyamp
Copy link

@walles would you be interested in a PR that adds a way (environment var like LS_COLORS and/or a file of some kind) to configure the colours used by riff?

I think that could help with the colourblindness issue that 0x5c brings up, and allow users to choose colours that fit their preferences.

Also, I think the only way to get decent highlighting on git combined diffs (other than just interpreting both + and + as green etc) would be to use more than 16 colours, and unless you want to expand the required terminal palette that would need to be left to the user.

@sergeevabc

This comment was marked as off-topic.

@walles

This comment was marked as off-topic.

walles added a commit that referenced this issue Jan 31, 2024
With this change in place, +++ and --- are now always bold so that
people who are used to reading diffs can quickly spot the added and
removed files.

/dev/null is still lowlighted.

Added files have their names prepended with the word "NEW". Removed
files have their names prepended with the word "DELETED".

Since adds / deletions aren't really name changes, I didn't want to use
colors for highlighting new / deleted files.

Let's see how this feels.

Relates to #47.
@walles
Copy link
Owner

walles commented Jan 31, 2024

Added/removed files highlighting

Here's the updated flavor from the latest commit. I'm still unsure what would be the best thing here, but I do think this is an improvement. Inspired by (but not fully following) this comment:
new-added-file
new-removed-file

Both prefixes are now highlighted to aid vision (muscle?) memory.

/dev/null is still lowlighted since it just means "nothing".

New files have their names prefixed with NEW, and deleted files with DELETED.

I opted for not using red / green since these aren't really changes.

walles added a commit that referenced this issue Jan 31, 2024
walles added a commit that referenced this issue Feb 1, 2024
@walles
Copy link
Owner

walles commented Feb 1, 2024

"Only additions" highlight

I have now added a new command line option for this.

And, riff will now read command line options from the RIFF environment variable, so this behavior can now be set as the default.

Sorry for the long lead time @0x5c!

I will close this issue now since it's become too much of a kitchen sink and the initial requests have been remedied to some extent.

Do open new issues for things that still need fixing or things I forgot!

@walles walles closed this as completed Feb 1, 2024
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

No branches or pull requests

4 participants