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

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

Merged
merged 13 commits into from
Oct 6, 2023

Conversation

mvanderkamp
Copy link
Contributor

@mvanderkamp mvanderkamp commented Sep 18, 2023

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
Copy link
Contributor Author

@jgarte Something like this?

@jgarte
Copy link
Contributor

jgarte commented Sep 18, 2023

@jgarte Something like this?

Yep, that looks really great!

Please merge and ship it to PyPI 😸

@mvanderkamp
Copy link
Contributor Author

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

@inducer
Copy link
Owner

inducer commented Sep 20, 2023

@mvanderkamp Could you look into the linker failure?

This should probably not be merged before #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
Copy link
Contributor

jgarte commented Sep 21, 2023

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
Copy link
Contributor Author

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 mvanderkamp changed the title Reorganize themes and add a light gray 256 theme Reorganize themes and add gray-light-256 and nord-dark-256 themes Sep 22, 2023
@mvanderkamp
Copy link
Contributor Author

@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
Copy link
Contributor Author

Ah yeah urwid has made some breaking changes for us: urwid/urwid@8d8e4b6

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

@mvanderkamp
Copy link
Contributor Author

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

@ThomasJRyan
Copy link

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
Copy link
Contributor

jgarte commented Sep 29, 2023

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

@ThomasJRyan
Copy link

Looks like you'll also be able to close #248

@mvanderkamp
Copy link
Contributor Author

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

@inducer
Copy link
Owner

inducer commented Oct 1, 2023

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
Copy link
Contributor Author

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! 🎉

@jgarte
Copy link
Contributor

jgarte commented Oct 2, 2023

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 inducer merged commit 5f5a235 into inducer:main Oct 6, 2023
8 checks passed
@inducer
Copy link
Owner

inducer commented Oct 6, 2023

LGTM. Thanks!

@jgarte jgarte mentioned this pull request Oct 6, 2023
@mvanderkamp mvanderkamp deleted the grayscale branch June 10, 2024 03:34
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.

Greyscale Theme Nord Theme Move themes out to separate files
4 participants