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

Tabs in diffs are not rendered correctly #8616

Open
vczh opened this issue Nov 12, 2019 · 10 comments
Open

Tabs in diffs are not rendered correctly #8616

vczh opened this issue Nov 12, 2019 · 10 comments
Labels
bug Confirmed bugs or reports that are very likely to be bugs priority-3 Bugs that affect small number of users and/or relatively cosmetic in nature

Comments

@vczh
Copy link

vczh commented Nov 12, 2019

Describe the bug

In both "Changes" and "History", when diffed lines has tab characters, the widths and positions are not correct.

Version & OS

Version 2.2.3
Windows 10 1903 (18362.418)

Steps to reproduce the behavior

  1. Modify anything, be sure that the modified line should begin with a tab character
  2. See diff in "Changes" (scenario 1)
  3. Submit the change
  4. See diff in "History" (scenario 2)

Expected behavior

scenario1: The first tab character has 4 spaces in width
scenario2: The first tab character has 4 spaces in width

Actual behavior

scenario1: The first tab character has 3 spaces in width
scenario2: The first tab character has 3 spaces in width
(if it is configured to be 8 spaces, then it it rendered in 7)

In this way, when the line has multiple tab characters, they all begin at (4n+3)-th character, instead of 4n.

I think it because the first "+" or "-" or " " takes 1 character space, which is not correct. These characters should not rendered like part of the text.

Screenshots

N/A

Logs

N/A

Additional context

N/A

@billygriffin
Copy link
Contributor

Hi @vczh, thanks for the issue. This seems to be an artifact of how Git produces unified diffs. I think the same problem also happens on the command line. Would you mind sharing how this is causing a problem for you and what you might able to do differently if there were 4 spaces after the + or -? It looks like copy/paste still works as expected from quickly testing that.

@billygriffin billygriffin added the more-info-needed The submitter needs to provide more information about the issue label Nov 12, 2019
@vczh
Copy link
Author

vczh commented Nov 13, 2019

@billygriffin Hi, I use tabs to format my code a lot. By not rendering tabs in correct positions and widths, it is hard to review before submitting the code.

Let me give you an example, I add following lines to my file

ABCD[tab]1234
ABC[tab][tab]1234

Both my editor and Github Desktop agree that tab equals to 4 spaces, and this is how it looks like in my editor (* == space)

ABCD****1234
ABC*****1234

When this is rendered in the diffs, it looks like this

+ABCD***1234
+ABC********1234

The issue makes well formatted code broken, because this is the correct result

+ABCD****1234
+ABC*****1234

By the way, I don't think Git cause Github Desktop to have problems. You can preprocess Git's output before pasting all of them to the app. More importantly, since Git command line has the same issue, if the app renders it correctly, it adds more value to the app.

One of the solution could be, render the first character and the rest separately, and do it line by line. When I select and copy something in diffs, the app don't actually give me those "+" and “-” characters. And when I just select the "+" character, and copy it, there is nothing in the clipboard. So it is not reason to put them in the same "text box" (I also believe that being able to select "+" and "-" is a bug because of such behavior), to let "+" and "-" affect how tabs are rendered.

@kuychaco
Copy link
Contributor

kuychaco commented Nov 13, 2019

@vczh thanks for the report. I'm trying to wrap my head around the issue you are seeing and could use some clarification.

I'm not seeing a discrepancy between my editor and GitHub Desktop:

README_md_—_desktop-tutorial_and_GitHub_Desktop

Could you please provide screen shots of what you are observing? That would help us understand your issue.

Version 2.2.3
MacOS High Sierra 10.13.6

lee-dohm added a commit to lee-dohm/test-repo that referenced this issue Nov 13, 2019
@lee-dohm
Copy link
Contributor

lee-dohm commented Nov 13, 2019

Steps to reproduce:

  1. In a git repository associated with GitHub Desktop, create a new file using VSCode (v1.40.1)
  2. Before entering any text, ensure that the indentation settings for the file are Tab Size: 4
  3. Enter the sample text:
    sdf
    foo
    bar
    baz
    
    ABCD[TAB]1234
    ABC[TAB][TAB]1234
    
  4. Save the file as test.txt
  5. Open the repository in GitHub Desktop (v2.2.3)
  6. View the diff

Screen Shot 2019-11-13 at 3 11 18 PM

The difference in behavior is "tabs as characters" and "tab stops". In VSCode, a Tab character is interpreted as advancing to the next multiple of the tab width (tab stop behavior). In GitHub Desktop, a Tab character is interpreted as some number of space characters, no matter where the Tab character appears on the line (tabs as characters behavior).

Reproduced using macOS 10.14.6.

@vczh
Copy link
Author

vczh commented Nov 13, 2019

@lee-dohm 's steps are right, but the reason is different. Maybe macOS and Windows version of Github Desktop has different bugs, but in my computer, the tab stop calculation is wrong because it counts the first "+"|"-"|" " character of each line, which should be avoided obviously.

@kuychaco I don't know why you cannot repro the issue since I can do this consistently, I guess the reason could be your editor replaces tab to spaces in that file. This happens in VSCode too due to different configurations for different file types.

lee-dohm added a commit to lee-dohm/test-repo that referenced this issue Nov 14, 2019
@lee-dohm
Copy link
Contributor

Yes, I apologize, my understanding of the GitHub Desktop logic is incomplete. I made incorrect assumptions about why the code was behaving the way it was. Let's focus on properly describing the behavior please @vczh. The more exact information you can give us, the better we'll be able to understand what it is you're suggesting we should fix.

I created a new file with better content to see the alignment:

sdf
foo
bar
baz

1234567890123456789012345678901234567890
ABCD[TAB]1234
ABC[TAB][TAB]1234

Which shows up like this:

Screen Shot 2019-11-13 at 4 27 56 PM

@tierninho tierninho added more-info-needed The submitter needs to provide more information about the issue and removed more-info-needed The submitter needs to provide more information about the issue labels Nov 19, 2019
@outofambit
Copy link
Contributor

Thanks for the discussion everyone. I think the bug is best summarized as "the diff in desktop doesn't match the text as shown in a text editor". (As a side note, Desktop's behavior here is indeed in line with the git CLI.) Marking this as a bug.

@outofambit outofambit added bug Confirmed bugs or reports that are very likely to be bugs priority-3 Bugs that affect small number of users and/or relatively cosmetic in nature and removed more-info-needed The submitter needs to provide more information about the issue labels Dec 16, 2019
@vczh
Copy link
Author

vczh commented Dec 17, 2019

What's the chance that a P3 bug get fixed?

@outofambit
Copy link
Contributor

@vczh Not likely in the near future. This can change if we learn more about the impact of this bug, but right now it seems primarily cosmetic and has an easy workaround of checking the original file (hence P3). There's potential for this to become a help-wanted issue in the future, but we're in the middle of revising our process for that.

I'm leaving this issue opened and have labelled it a bug because it is something that causes our users pain/confusion and is something that we can fix. I know this probably isn't the answer you're looking for, but I appreciate you asking and I want to clearly set expectations.

@ahmetsait
Copy link

This issue seems to be fixed in #17428 and can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bugs or reports that are very likely to be bugs priority-3 Bugs that affect small number of users and/or relatively cosmetic in nature
Projects
Development

Successfully merging a pull request may close this issue.

7 participants