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.

Sign up for GitHub

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

Jump to bottom

diff: alter foreground color as well #96

Merged
merged 1 commit into from
Sep 1, 2023
Merged

diff: alter foreground color as well #96

merged 1 commit into from
Sep 1, 2023

Conversation

NikolayXHD
Copy link
Contributor

to keep readable contrast between bg and fg

example:

viddy -d --no-title date

before

image

after

image

sachaos
sachaos requested changes Sep 1, 2023
View reviewed changes
Copy link
Owner

@sachaos sachaos left a comment

Choose a reason for hiding this comment

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

Thank you

snapshot.go
@@ -164,13 +164,7 @@ func DiffPrettyText(diffs []diffmatchpatch.Diff) string {

switch diff.Type {
case diffmatchpatch.DiffInsert:
for _, c := range text {
if unicode.IsSpace(c) {
Copy link
Owner

Choose a reason for hiding this comment

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

Don't delete this if branch.
Just set foreground color on L171.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@NikolayXHD
diff: alter foreground color as well
c59e27e 
to keep readable contrast between bg and fg
sachaos
sachaos approved these changes Sep 1, 2023
View reviewed changes
Copy link
Owner

@sachaos sachaos left a comment

Choose a reason for hiding this comment

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

LGTM
Thank you! :)

@sachaos sachaos merged commit 161a2f7 into sachaos:master Sep 1, 2023
1 of 2 checks passed
@NikolayXHD
Copy link
Contributor Author

@sachaos just discovered viddy today. I find it succint and elegant, and it solves very real problem. For me it's being able to scroll.

Than you for your work!

@NikolayXHD
Copy link
Contributor Author

@sachaos hi, how are you :)

Do you remember that loop, which highlights each individual character in the diff.

There is an issue with that, if there are already colors in the text, before coloring diff,
then for example '[\x1b40m' gets converted to green('[') + green('\x1b') + green('4') + ..., which visually looks like this:

image

so invisible '[\x1b40m' becomes visible and green, which is not supposed to happen.

Maybe we should remove such things from the string before applying green?

@sachaos
Copy link
Owner

sachaos commented Sep 7, 2023

@NikolayXHD Hi, thank you for your bug report.
Is this new bug of this PR or not?
I think we should dicuss on a new issue.

@NikolayXHD
Copy link
Contributor Author

Should not be caused by this MR, new discussion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sachaos sachaos sachaos approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@NikolayXHD @sachaos