inducer / pudb

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

Reorganize themes and add gray-light-256 and nord-dark-256 themes #612

Closed mvanderkamp closed 11 months ago

mvanderkamp commented 11 months ago

This PR adds two new themes: gray-light-256 and nord-dark-256. I also figured it was time to take a crack at splitting the themes out into their own files.

Oh, and there's a minor fix for an issue that was causing the debugger to crash when it couldn't find a custom theme file on startup.

Let me know what you think!

gray-light-256 Preview

Light grayscale 256 preview

nord-dark-256 Preview

Screenshot 2023-09-17 at 14 41 29

Closes #611 Closes #572 Closes #567 Closes #248

mvanderkamp commented 11 months ago

@jgarte Something like this?

jgarte commented 11 months ago

@jgarte Something like this?

Yep, that looks really great!

Please merge and ship it to PyPI 😸

mvanderkamp commented 11 months ago

This should probably not be merged before #572, because of the refactor around themes getting their own modules.

inducer commented 11 months ago

@mvanderkamp Could you look into the linker failure?

This should probably not be merged before https://github.com/inducer/pudb/pull/572, because of the refactor around themes getting their own modules.

Could you clarify how the two PRs would interact? Maybe @jgarte can give you permission to simply roll the nord-256 theme into this PR, to keep things simple?

jgarte commented 11 months ago

Could you clarify how the two PRs would interact? Maybe @jgarte can give you permission to simply roll the nord-256 theme into this PR, to keep things simple?

You mean for me to give write access to the nord branch? @inducer

If that helps, I can do that. Just let me know.

mvanderkamp commented 11 months ago

I think it would work quite easily for me to add the nord theme to this PR as just a copy-paste, I would have to do something roughly similar anyway to reconcile the two PRs. I'll take a go at it. @jgarte I'll see if there's some git shenanigans I can do to still make you the author of the theme in the commits.

mvanderkamp commented 11 months ago

@jgarte Okay it was actually easy enough to just pull your branch into mine, so git history should still be there as expected.

I'm getting some odd pylint errors:

************* Module pudb.debugger
pudb/debugger.py:2479:36: E1101: Module 'urwid.version' has no 'VERSION' member (no-member)
pudb/debugger.py:2807:25: E1133: Non-iterable value keys is used in an iterating context (not-an-iterable)
mvanderkamp commented 11 months ago

Ah yeah urwid has made some breaking changes for us: https://github.com/urwid/urwid/commit/8d8e4b678cc0f93149a4a964b26ee11fb136ea0f

Not sure why there's a "non-iterable" failure though

mvanderkamp commented 11 months ago

I added a restriction to not use the latest releases of urwid, that did the trick for this PR.

ThomasJRyan commented 11 months ago

Looking forward to this getting merged in. I've got an idea on how to expand it further now that the themes have been broken out into their own files

jgarte commented 11 months ago

I'm also looking forward to seeing this merged :)

ThomasJRyan commented 11 months ago

Looks like you'll also be able to close https://github.com/inducer/pudb/issues/248

mvanderkamp commented 11 months ago

Oh nice! I've reverted the urwid restriction, since the conflict is fixed.

inducer commented 11 months ago

At this point, all I can do is squash-merge this PR, but since multiple contributors are involved, that doesn't seem like the right choice. Could you clean up the history and rebase so that I can simply merge-rebase?

mvanderkamp commented 11 months ago

Whew that was more effort than I expected! Basically had to redo all the nord theme commits as though they were originally done in their own file. But it looks like it worked, and maintained attribution! :tada:

jgarte commented 11 months ago

Whew that was more effort than I expected! Basically had to redo all the nord theme commits as though they were originally done in their own file. But it looks like it worked, and maintained attribution! 🎉

Thanks for doing that. It is much appreciated!

inducer commented 11 months ago

LGTM. Thanks!