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

fix git annotations bug #9999

Merged
merged 1 commit into from
Sep 15, 2021
Merged

fix git annotations bug #9999

merged 1 commit into from
Sep 15, 2021

Conversation

shuyaqian
Copy link
Contributor

@shuyaqian shuyaqian commented Aug 31, 2021

Signed-off-by: shuyaqian 717749594@qq.com

What it does

fix #9998

How to test

  1. 'alt + b' to execute git.editor.toggle.annotations command
  2. when characters other than English exist,width calculation is incorrect

Review checklist

Reminder for reviewers

@colin-grant-work colin-grant-work self-requested a review September 1, 2021 15:08
Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an improvement over the old code, but I was still able to get a bad result when the message overflowed:

image

Since not all reviewers may be comfortable with non-Latin characters, you may want to include a test commit as part of this PR with a message that includes non-Latin characters, and add instructions in your PR description for where to find those messages.

@vince-fugnitto vince-fugnitto added the git issues related to git label Sep 1, 2021
@shuyaqian
Copy link
Contributor Author

This is an improvement over the old code, but I was still able to get a bad result when the message overflowed:

image

Since not all reviewers may be comfortable with non-Latin characters, you may want to include a test commit as part of this PR with a message that includes non-Latin characters, and add instructions in your PR description for where to find those messages.

done.

@colin-grant-work colin-grant-work self-requested a review September 7, 2021 12:57
@colin-grant-work
Copy link
Contributor

With the recent changes, this is looking good to me:
image

Please squash the commits, and I'll approve.

@msujew
Copy link
Member

msujew commented Sep 7, 2021

@colin-grant-work Even with the recent changes I'm still experiencing the issue you mentioned previously:

image

@colin-grant-work
Copy link
Contributor

@colin-grant-work Even with the recent changes I'm still experiencing the issue you mentioned previously:

Hm... thanks for checking. Gotta try more scripts... :-)

@colin-grant-work
Copy link
Contributor

colin-grant-work commented Sep 7, 2021

It looks like Japanese, Korean, and Arabic are all causing problems:

image

I used these messages:

わたしにほんごがすこししかはなせません
Courtesy of @msujew

한국어에서 Git 주석이 어떻게 보이는지 알고 싶습니다.
أريد أن أعرف كيف تبدو التعليقات التوضيحية باللغة الكورية.

The Arabic message is also displaying the end rather than the beginning - but handling RTL text correctly may be beyond the scope of this PR.

@shuyaqian
Copy link
Contributor Author

It looks like Japanese, Korean, and Arabic are all causing problems:

image

I used these messages:

わたしにほんごがすこししかはなせません
Courtesy of @msujew

한국어에서 Git 주석이 어떻게 보이는지 알고 싶습니다.
أريد أن أعرف كيف تبدو التعليقات التوضيحية باللغة الكورية.

The Arabic message is also displaying the end rather than the beginning - but handling RTL text correctly may be beyond the scope of this PR.

I have fixed it.

@shuyaqian
Copy link
Contributor Author

image

@colin-grant-work
Copy link
Contributor

colin-grant-work commented Sep 13, 2021

@shuyaqian, it looks like a problem with a reference to a non-existent namespace field is crashing the build.

@shuyaqian
Copy link
Contributor Author

@shuyaqian, it looks like a problem with a reference to a non-existent namespace field is crashing the build.

That's ok now.

Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The style specifications now look robust, and I can't identify any cases in which the time message overflows.

Signed-off-by: shuyaqian 717749594@qq.com
@shuyaqian
Copy link
Contributor Author

The style specifications now look robust, and I can't identify any cases in which the time message overflows.

You can test it like this
img

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me as well. Using different scripts/languages I could no longer reproduce the issue 👍

@colin-grant-work colin-grant-work merged commit 05f289b into eclipse-theia:master Sep 15, 2021
@shuyaqian shuyaqian deleted the fix-annotations branch September 16, 2021 01:03
@vince-fugnitto vince-fugnitto added this to the 1.18.0 milestone Sep 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
git issues related to git
Projects
None yet
Development

Successfully merging this pull request may close these issues.

git.editor.toggle.annotations‘ width calculation is incorrect when characters other than English exist.
4 participants