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 `show-whitespaces` feature #2073

Merged
merged 28 commits into from Jul 14, 2019

Conversation

Projects
None yet
10 participants
@notlmn
Copy link
Contributor

commented May 25, 2019

Epilogue

Before explaining how this feature works, I'd like to outline all the things I tried to implement this feature.

1. Custom Font

Just like @bfred-it in #588 (comment), I tried to create a custom font using FontForge adding glyps to the codepoints "Space" (U+0020), "New Line" (U+000A), and "Horizontal Tabulation" (U+0009).

As as expected it failed, the tab glyph was substitued by space glyph, and new line glyph was outright ignored. This looks like that the visual for control characters is handled by the application parsing the text and not by the font being used. That may be the reason no font in the world contains any glyph information placed in these codepoints, so the font-fallback ends up at the application handling that code point.

2. Font Ligatures

I have a feeling that this can be solved using font ligatures, providing glyphs information for how these whitespace characters interact with codepoints in the Basic Multilingual Plane (BMP).

I have no way to test this, I don't even know if this is possible, end of story.

3. Replacing Characters

The characters space and tab can be easily replaced with any of the codepoint in Unicode's Private Use Area plane. But as mentioned above, we loose semantics for control characters.

This PR

Closes #588.

With the above tests in consideration, I don't know if this feature can be implemented in any other way except for doing DOM manipulations. If there is I'm all ears.

This PR is a lot similar to #1989, doing a heck of lot DOM manipulations. At the start I had concerns about performance, but as it seems this feature does look a bit light that #1989.

Below are the list of characters and their substitution characters for reference.

White-space Characters

Entity Replacement character Unicode name Unicode number HTML-code
Space "Bullet" U+2022 •
Tab "Rightwards Arrow" U+2192 →
New Line (Disabled) ¬ "Not Sign" U+00AC ¬

Note: The part to enable "New Line" is disabled for now, as it has unusual behavior while displayed on empty lines (where it is the only child). And also because how it interacts when there is no new-line at the end of file. Yes, there is commented code committed to Git.

Testing this PR

Also works on all lazy-loaded and demand-loaded diff and file views.

Possible Future Additions

  • Show white-space character in code blocks inside comments.
    This is not a part of the current PR, as code blocks in comments are not separated into lines. Will see what I can do later.

  • Display every "invisible" (non-printable) character outside BMP using some method.
    Generalizing this feature to show-invisibles. Can be used especially to detect file corruptions. But this might be heavy on the browser (way too many DOM nodes).

  • To show or hide white-space characters per file.

Screenshots

image

notlmn added some commits May 22, 2019

@notlmn

This comment has been minimized.

Copy link
Contributor Author

commented May 25, 2019

One problem I see is that embedded code has empty spaces add to the start and end of each line.

image

A problem from GitHub's end, messes with copy and pasting code.

/cc @lukehefson @shayfrendt

Show resolved Hide resolved source/features/show-whitespaces.tsx Outdated
Show resolved Hide resolved source/features/show-whitespaces.tsx Outdated
Show resolved Hide resolved source/features/show-whitespaces.tsx Outdated
Show resolved Hide resolved source/features/show-whitespaces.tsx Outdated
Show resolved Hide resolved source/features/show-whitespaces.tsx Outdated
@bfred-it

This comment has been minimized.

Copy link
Collaborator

commented May 26, 2019

Your fork has an obsolete tag, do you mind deleting it?

git tag -d hotfix
git push --delete origin hotfix

notlmn added some commits May 27, 2019

@notlmn

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2019

With the latest commit, I managed to enable showing new-line characters. I can't decide this yet, but is this a bit distracting?

image

@bfred-it

This comment was marked as resolved.

Copy link
Collaborator

commented May 30, 2019

On hold. Let's merge it after #2085

@bfred-it bfred-it force-pushed the sindresorhus:master branch from 5df5c5f to 5f1a966 Jun 1, 2019

notlmn added some commits Jun 2, 2019

notlmn added some commits Jun 4, 2019

@notlmn notlmn changed the title Add `show-whitespaces` features Add `show-whitespaces` feature Jun 9, 2019

notlmn added some commits Jun 9, 2019

notlmn added some commits Jul 7, 2019

@bfred-it bfred-it merged commit a4173bd into sindresorhus:master Jul 14, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bfred-it

This comment has been minimized.

Copy link
Collaborator

commented Jul 14, 2019

Merging this because I want to see it in the wild and it's received enough reviews for now. These 2 suggestions would still be good though:

@bfred-it

This comment has been minimized.

Copy link
Collaborator

commented Jul 14, 2019

Thank you for this feature ^_^
I hope it's received well and doesn't need a disable toggle

@jesstelford

This comment has been minimized.

Copy link

commented Jul 15, 2019

This is a great feature, but it's frustrating that it's enabled by default.

I'm very used to reading code on GitHub, and right now don't have time to go hunting for what suddenly changed (but I did, and ended up finding it here after scrolling through the long list of GHR options one by one) to restore things to how they were.

Perhaps this could be disabled by default, or provide a toggle at the top of code views to "show whitespace characters" (like the "show whitespace" option in PRs)?

@jerone

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2019

I wanted this feature too, but I expected to see an toggle button. 🤔

@waldyrious

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2019

I'm personally fine with the whitespaces being made visible by default, but I'd rather retain the ability to select text including whitespace. With this feature, that's only possible if I start the selection in the non-whitespace part.

Even if I'm not particularly interested in selecting the whitespace per se, the disabling of selections makes it hard to select text that is surrounded by whitespace, since I need to place the cursor exactly at the word boundary (e.g. the start of the non-whitespace part of a line) for the text selection to be enabled, which is definitely cumbersome.

@wearhere

This comment has been minimized.

Copy link

commented Jul 15, 2019

I don't necessarily mind this feature being enabled by default and yet more of a heads-up would have been nice. For instance—should #1137 have been bumped?

@connorjclark

This comment has been minimized.

Copy link

commented Jul 15, 2019

I do mind this being enabled by default ... was very unexpected and imo it's just noise.

@bfred-it

This comment has been minimized.

Copy link
Collaborator

commented Jul 16, 2019

@wearhere we don’t update that page that often, once a month usually

@henrikno

This comment has been minimized.

Copy link

commented Jul 16, 2019

I also came here because I found this distracting while reviewing PRs. I first disabled Refined Github to check whether it was Github or Refined that added it. Then I spent 10 minutes scrolling through the options to find out which one I should disable.

Having a button and defaulting to off would be my choice #2240

@michaelficarra

This comment has been minimized.

Copy link

commented Jul 16, 2019

I'm surprised to see internal whitespace made visible here. Usually I only see leading/trailing whitespace. Because of this, I had to disable the option. It made it too difficult to read.

@notlmn

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2019

The author of this PR is surprised to see how many people find this feature distracting. The option to view whitespace per file was already mentioned in the original post of this PR, but was merged anyway because this PR had way too many reviews already and was already almost 2 months old, and assuming there would be future possible additions, and yes the author agrees of that.

Meanwhile the author of this PR is

  • tired after attending a very long hackathon
  • going through a slight identity crisis, kind of
  • feeling shitty everyday after returning from college recently

The author would like to mention the users that they are always welcome to disable this feature in the extension settings at any point, which is why this opiniated extension has added the ability to disable features in the first place because too many people had specific requirements.

The author would like to get back to this issue as soon as they can and would also like to link to this xkcd comic hoping that the users would calm down.

Feedback is always appreciated.

Peace 🖖.

@jerone

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2019

Appreciate all your work @notlmn 👍

Let's divert everyone to #2240.

When I find the time (hopefully this weekend); I'll write a toggle button. Because I love this feature, but only want it enabled when I need it (and don't want to enable/disable the whole feature and then reload the page again).

Repository owner locked and limited conversation to collaborators Jul 18, 2019

Repository owner unlocked this conversation Jul 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.