inducer / pudb

Full-screen console debugger for Python
https://documen.tician.de/pudb/
Other
2.99k stars 230 forks source link

Fix the midnight theme #508

Closed asmeurer closed 2 years ago

asmeurer commented 2 years ago

This reverts the changes to the midnight theme that were made in #469 and also cleans it up a little bit.

CC @mvanderkamp I don't know if any of the changes in c7a8fbb0 were actually important.

inducer commented 2 years ago

It seems like this reintroduces a bunch of the redundancy that @mvanderkamp sought to eliminate. Maybe we can find a compromise that serves both goals?

asmeurer commented 2 years ago

I'm happy to do that. I basically just reverted his commit and started from there. I just didn't like the color changes that were made. I wasn't able to completely understand everything in the theming (some documentation here would be nice).

mvanderkamp commented 2 years ago

Yeah I guess I didn't really write this down anywhere inside the code.

When remastering the themes I was trying to achieve a handful of goals:

  1. Simplify and unify:
    • Give a consistent look to all UI elements.
    • A selectable item in one place should look similar to a selectable item elsewhere, and should look always sufficiently distinct from non-selectable items.
    • The currently selected item should always have similar styling regardless of which view you're in, especially the background color. (Hence getting rid of the special background highlights for various items when selected).
    • If possible, whichever color is used for the background of the "selected" item should not be used as the background anywhere else.
    • Using the base styles as much as possible, and letting the style hierarchy work, helps a lot here.
  2. Generalize
    • Specifically, I was trying to make it so that at least one 16-color theme was likely to be at least useable in the major common terminal themes. This might be what caused the changes in midnight you don't like. It's also not actually as big of a concern anymore since I wound up adding a mono theme which should always be a useable fallback. Just wanted to give a good "out of the box" experience though.
    • This involved testing the themes on terminals which were set to render bold text in bright colours, since this isn't always possible to turn off.
  3. Specialize
    • I tried to make each theme unique stand out in some way from the others
  4. Match external themes
    • If I was able to find an external theme of the same name that the theme seemed to be based on, I tried to align it as closely as possible.

I have no preference as to the actual colors that are used for any particular theme. Looks like specifically I wound up with a lot more green in the midnight theme than it was originally- (basically went with the sidebar selected item highlight over the source view's selected highlight then ran with it)- by all means switch that up.

A few specific thoughts currently to give examples of the above:

asmeurer commented 2 years ago

White on light blue for the line matching a search is quite hard to read, as is the bright yellow on light grey for the active header, and dark blue on black for the "class" (e.g. [MyClass]) highlight in the stack view.

Thanks. I figured I was missing some theming elements when playing around with this. I completely forgot there is a search function. And I still can't figure out what "active header" refers to.

When I say "documentation" what I really want is documentation on what each theme item actually refers to (for instance, it took me like half an hour of playing around to figure out what the "current line" selector was, partly because I wasn't expecting the yellow to actually be brown).

Need consistent "selectable"/"selected" background highlights

Do you mean that it is green for the sidebar and blue for the source view or something else? I think I switched to blue because I didn't like the green in the source view. I should note that this theme is designed to be used in a terminal where the text color is set to green. I set the source color to "" so it uses the default text color in your terminal (I could just use "dark green" here so that it's consistent). The text color to green is a bit of a change for the theme here, but it's what I actually intended in the first place (as is noted by the comment in the code), and it's what I use in other programs where I use a similar theme.

asmeurer commented 2 years ago

I pushed some further improvements, which make the midnight colors more consistent and readable. Let me know if you notice something that isn't readable or consistent (it's most likely some odd part of the UI I forgot about).

Two issues I noticed:

inducer commented 2 years ago

In case that's useful, here's the before and after (68e4465) in my terminal (Kitty with a theme based on "brewer"):

New:

image

Old:

image

Both versions feel "reasonable"/"usable" to me, and the existing version has way less code...

asmeurer commented 2 years ago

Your terminal does have slightly different ways of showing colors than mine, but it more or less looks the same (except I don't know why your AttributeError is showing in brown when it is supposed to be green).

Probably I can trim down some of what is here using the inheritance stuff.

mvanderkamp commented 2 years ago

And I still can't figure out what "active header" refers to.

Oh sorry, that's the "focused sidebar" style. Which I've just realized is unfortunately similar to the "focused sidebar {one,two,three}" styles, so in retrospect those should have had a different name.

When I say "documentation" what I really want is documentation on what each theme item actually refers to

Ah my mistake. Yes I agree this would be helpful, and would make it easier for users to write custom themes too!

I could just use "dark green" here so that it's consistent

This make sense to me. My (possibly controversial) opinion is that themes intended for a 16-color terminal palette shouldn't have any fallbacks to the default fore/background colours.

There doesn't seem to be a coloring for "focused hotkey", so the hotkey letter looks off in the focused pane.

I'm definitely in favour of adding this!

I used bold dark gray for "disabled current breakpoint", but for me at least it looks like bold white. I don't know if this is a bug in my terminal, urwid, pudb, or if they just do look the same.

I think it's a bug, they look correct for me.

mvanderkamp commented 2 years ago

I should clarify, in case my suggestions were a bit much: those really are just suggestions. It sounds like this is your theme so don't let me get in the way!

asmeurer commented 2 years ago

No worries, I just haven't had a chance to look over your suggestions yet.

asmeurer commented 2 years ago

I made some tweaks here based on review and some other things. I didn't do the link thing because I couldn't figure out exactly what you meant, but if you have some way to deduplicate the settings here, please do so. I didn't do the input/button thing because I'm not sure I agree with it (although tbh I can't say I'm completely satisfied with the colors in the dialogs, but they seem good enough).

As an aside, I found out that in is highlighted like an operator and not a keyword, which is a little annoying (most other editors would highlight it like a keyword).

Another thing I noticed is that the cursor is colored differently if it's the on the first character of an input box, meaning with my current decision to not highlight selected input, it's impossible to tell it's highlighted when the input is empty. Any idea why that happens? I noticed it also happens in some of the other themes, but not all of them.

asmeurer commented 2 years ago

Yeah I'll have to add documentation about how the hierarchy works and what linking styles does. There's definitely some deduping that can be done, did you want me to add suggestions?

If you want you can make suggestions or make a subsequent PR cleaning it up.

Interesting, the python docs list it under "keywords" but refer to it as an operator. Being a keyword which is an operator, I guess it's subjective which is more important. Anyway, this is handled by pygments: https://github.com/pygments/pygments/blob/master/pygments/lexers/python.py#L177

Ahh, that explains why some other code I'm using doesn't have this problem. It colors Operator.word separately from Operator. Maybe we should add this distinction to the pudb themes.

This probably depends on the terminal emulator. It might have specific colors set for your cursor instead of having it reverse the colours of the current cell. I'm not sure if urwid exposes anything for overriding that.

You're right. I'm using "smart cursor color" in iTerm2. The problem doesn't exist if I turn that off or use Terminal.app.

Still, it seems that there must be some difference in how it is reporting the colors for the first character and the subsequent ones, even though they appear the same. I guess it's possible this is an issue with iTerm2. It isn't really clear how the "smart cursor color" works. Here's an example (from the dialog that pops up when you press 'q'). In the first image the cursor is on a and in the second image the cursor is on b.

Screen Shot 2022-08-29 at 2 09 12 PMScreen Shot 2022-08-29 at 2 09 17 PM

asmeurer commented 2 years ago

Addressed the comments. Any objections to merging this?

mvanderkamp commented 2 years ago

None from me!

inducer commented 2 years ago

LGTM. Thanks for working on this!