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

Xresources updates #49

Closed
wants to merge 3 commits into from
Closed

Xresources updates #49

wants to merge 3 commits into from

Conversation

dgoodlad
Copy link

Support for light/dark switching, and a change to the bright-black color to make it visible against the background.

Simply #define SOLARIZED_LIGHT or SOLARIZED_DARK to choose which
one you want
Many tools expect bright black to be visible against the background.
For example, the backtraces printed by rspec are printed in bright
black, and were unreadable with the old color.
Missed this earlier!
@altercation
Copy link
Owner

@dgoodlad

I understand why you made this change regarding bright black, but the other terminal-color compliant ports of solarized utilize base03 in that slot. I'm not against a global change but it has to be all or nothing, so to speak. I.e. either this change is implemented across the board and all terminal themes in solarized will use it or it shouldn't be made in one port.

I'll outline why base03 is in that slot here and would like to examine this issue further.

There are a lot of CLI apps that can display background colors, but they can only display the ansi 0-7 as background values. This is generally true for Vim in terminal mode, Mutt (and I'm assuming most curses based apps), etc. I haven't dug deep into this (though perhaps I should) but my understanding is simply that bright ansi values cannot be used as background colors when in a 8/16 color terminal emulator.

I keep base02 and base2 in the 0/7 ansi slots so that I can use them in background values in those terminal emulators. Otherwise we're left without those as highlight values.

I'm not against having base03 in slot 0 and base02 in slot 8, but it throws a wrench into everything if the change isn't propagated to the other apps that make use of ansi values.

So those are the issues. I'm open to discussing.

@dgoodlad
Copy link
Author

I definitely see what you're trying to do there, but don't think that it's a good general solution.

I haven't tried the theme in either of the OSX terminals, but assume that it's setup the same way as in Xresources. I don't think it's wise to have the background color in any of ansi colors 8-15. Applications should be able to reasonably expect that they can use colors >=8 without worrying that the text will be invisible against the terminal's default background.

I'd definitely advise swapping the two colors throughout all the configs...

@altercation
Copy link
Owner

How would you handle background highlights in that case? I don't see any solution. There.

I agree that ideally apps do get to use 8-15 with impunity, but at the same time they shouldn't expect 8-15 to even be present. 0-7 should be prioritized given they are the immutable, always present values. In that case, I'd still want 0/7 to house base02/base2.

I'm taking this discussion seriously and am considering this change if there isn't a good reason not to do it. If it does change, it won't be before beta3 as I need to get all other devs coordinated on this. That being said, how do you feel about moving your alternative scheme to a dev branch on your repo and giving me one that is compatible with the current scheme to pull in? I prefer your conditional def method to a split light dark file.

@dgoodlad
Copy link
Author

Sure, I'm happy to split the branches and tidy things up a bit. Will have a look at it this evening.

@altercation
Copy link
Owner

Sounds good.

@altercation
Copy link
Owner

@dgoodlad - I just realized that I left the wrong base value in foreground in the old/original Xresources that you forked.

The correct values for foreground are:

dark mode: base0
light mode: base00

@dgoodlad
Copy link
Author

@altercation - I've opened a new pull request from a new branch. See #68

@dgoodlad dgoodlad closed this Apr 17, 2011
@lkraav
Copy link

lkraav commented Apr 23, 2011

@altercation: this base0 fix has not been commited as of today, has it? the only place it lives at is in that comment above?

@ghost
Copy link

ghost commented Aug 31, 2012

#68

xnox pushed a commit to xnox/solarized that referenced this pull request Sep 9, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants