Add 256 color support to syntax highlighting #58

Merged
merged 3 commits into from Mar 22, 2013

2 participants

@GregIngelmo

This pull adds 256 color support to pudb through urwid. Authors of syntax files can simply use a foreground color > 16 in place of named colors.

The urwid issue and workaround around on https://github.com/kemist/pudb/blob/master/pudb/theme.py#L301 is caused by urwid attempting to shoehorn a color > 88 into an 88 color palette, which throws an exception :( (see https://github.com/wardi/urwid/blob/master/urwid/display_common.py#L824).

My own color palette as an example https://dl.dropbox.com/u/330001/pudb_ingelmo_theme.py

I love pudb!

vim_next_to_pudb

@inducer
Owner

Thank you for the patch! I really like the idea.

One thing I don't like is that this adds a hard dependency on curses. Can you please make it so that things still work if the curses module is not found?

Can you also please report the issue you found in urwid and add the URL to the workaround as a comment, so that I know when the extra cruft can be removed?

Thanks for the patch, and sorry to make you do more work!
Andreas

@GregIngelmo

Hey Andreas, thanks for following up so fast. Great feedback, I'm a bit pressed for time but I'll get to it this weekend. No worries about the extra work!

Greg

@GregIngelmo GregIngelmo referenced this pull request in urwid/urwid Mar 10, 2013
Closed

Exception when using color values > 87 #24

@GregIngelmo

Hey Andreas,

I opened an issue with author of urwid, we're talking about it now.

curses has been part of the standard libs for some time (at least from 2.4+). Is it necessary to test if the module isn't found?

I've got some small changes coming your way,

Gregorio

@inducer
Owner

curses is a Windows portability issue. That said, pudb has a few of those already. I'll leave that up to you.

@GregIngelmo

How do you feel about the curses dependency check? Would you rather place the the conditional import try/except inside of DebuggerUI?

@inducer
Owner

Hey, sorry. I hadn't spotted your updates at all. The patch (and, thus, the curses check) looks fine, I've merged it. Thanks for the contribution!

I do have one question: Why do the 256 color support check via curses at all? What breaks if you don't? Urwid's 256 color demo doesn't seem to do it...

@inducer inducer merged commit 8f511af into inducer:master Mar 22, 2013
@GregIngelmo

Just some defensive programming. Urwid throws an exception if you try to use a high color value without first telling it that your terminal is 256-color capable. For example, if you comment out set_terminal_properties in the 256 color demo it'll throw

...
File "/Users/greg/Dropbox/Documents/Projects/urwid/urwid/examples/urwid/display_common.py", line 574, in _set_foreground
    "in foreground (%s)") % (repr(part), repr(foreground)))

urwid.display_common.AttrSpecError: Unrecognised color specification 'h244'in foreground ('h244')

Proud to be part of pudb :)

@GregIngelmo

Quick reference for anyone that wants to create a 256 color palette. Created with Wardi's xterm_color_chart

256_color_chart

@inducer
Owner

Ok, makes sense. His palette test doesn't really solve the problem of detecting high-color terminals, right? If so, curses sounds like a reasonable solution.

Thanks again for your patch!

@GregIngelmo

Right on. And glad to help.

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