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

Try to detect the current theme based on the terminal background color #2631

Closed
wants to merge 1 commit into from

Conversation

aykevl
Copy link

@aykevl aykevl commented Jul 28, 2023

This uses the termbg crate, as discussed here: #1746
Other than that, it uses the same logic as on macOS.

This behavior should probably be extended to macOS too, for cases like a terminal with a light background while the system theme is dark (or vice versa). But I didn't want to accidentally break anything on macOS.


This is my first Rust PR, so please be gentle 😄

I'm marking this a draft because cargo test is failing at the moment, I'll try to figure out why but any help would be appreciated.

This uses the termbg crate, as discussed here:
sharkdp#1746
Other than that, it uses the same logic as on macOS.

This behavior should probably be extended to macOS too, for cases like a
terminal with a light background while the system theme is dark (or vice
versa). But I didn't want to accidentally break anything on macOS.
aykevl added a commit to aykevl/termbg that referenced this pull request Jul 28, 2023
It doesn't make sense to try to set a terminal as a raw terminal if it
isn't actually a terminal. Worse, this may break tests that assume they
can simply provide a pipe to a subprocess and expect the terminal will
be unchanged.

See this PR, which needs this patch:
sharkdp/bat#2631
aykevl added a commit to aykevl/termbg that referenced this pull request Jul 28, 2023
It doesn't make sense to try to set a terminal as a raw terminal if it
isn't actually a terminal. Worse, this may break tests that assume they
can simply provide a pipe to a subprocess and expect the terminal will
be unchanged.

See this PR, which needs this patch:
sharkdp/bat#2631
@aykevl
Copy link
Author

aykevl commented Jul 28, 2023

Found the issue with the tests, I've made a termbg patch here: dalance/termbg#19

aykevl added a commit to aykevl/termbg that referenced this pull request Jul 28, 2023
It doesn't make sense to try to set a terminal as a raw terminal if it
isn't actually a terminal. Worse, this may break tests that assume they
can simply provide a pipe to a subprocess and expect the terminal will
be unchanged.

See this PR, which needs this patch:
sharkdp/bat#2631
@Enselic
Copy link
Collaborator

Enselic commented Oct 4, 2023

I hope you don't mind me closing this for now? I like when the PR inbox is clean :) Of course feel free to reopen this if you resume work on it!

@Enselic Enselic closed this Oct 4, 2023
@aykevl
Copy link
Author

aykevl commented Nov 23, 2023

I'm basically just waiting until dalance/termbg#19 is merged, but the author doesn't seem very active anymore.

aykevl added a commit to aykevl/termbg that referenced this pull request Dec 5, 2023
It doesn't make sense to try to set a terminal as a raw terminal if it
isn't actually a terminal. Worse, this may break tests that assume they
can simply provide a pipe to a subprocess and expect the terminal will
be unchanged.

See this PR, which needs this patch:
sharkdp/bat#2631
@aykevl
Copy link
Author

aykevl commented Dec 7, 2023

I've updated the branch (now with cargo test passing), but that isn't visible in the PR. Can you reopen this PR? (GitHub doesn't let me).

@Enselic
Copy link
Collaborator

Enselic commented Dec 7, 2023

I think that's a GitHub bug, you need to open a new pr after pushing stuff to a closed PR, unfortunately.

I can't reopen it either.

@aykevl
Copy link
Author

aykevl commented Dec 8, 2023

Ok, I'll make a new PR then.

EDIT: here it is: #2797

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

Successfully merging this pull request may close these issues.

2 participants