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

Update theme preview scripts and screenshots #909

Merged
merged 4 commits into from
Jul 22, 2024

Conversation

NotTheDr01ds
Copy link
Contributor

@NotTheDr01ds NotTheDr01ds commented Jul 22, 2024

Lots of changes - Pretty every change needed to be made before updating the preview screenshots, so they all end up in the same PR here:

  • preview-generate-screenshots.nu now generates the previews and screenshots all in one pass rather than requiring a separate file.

  • Adds a new method for generating screenshots using PowerShell with no third-party application required. Created new function for choosing the method that is used to generate screenshots.

  • Fixes background color on nushell-dark and nushell-light themes.

  • Fixes bug where cursor setting (and perhaps one other) was getting dropped from the preview table (math ceil was needed rather than math floor)

  • Fixes cellpath setting - Was changed to cell-path a while back but never updated in the themes. All themes and their previews were regenerated. Custom themes were manually adjusted.

  • Important: I did not update make.nu to fix this (yet) since @amtoine has a open PR against it that has not been merged yet. Easier to either put that change in that PR or do it after that one has been merged.

  • Adds mod.nu in the root of the themes module. Moves some of the previous script commands such as preview_terminal, preview_theme_small, etc. over to the module. Makes for cleaner usage.

  • Note: The completion functions in the scripts have been broken for several months. I did not fix this at this point.

  • preview_theme_small (and others) renamed to use spaces in command name, so it is now preview theme small.

  • Fixes bug of my own making that prevented several custom themes from working.

  • Refactors the "list to columns" code to use group (will need to be updated to chunks in 0.96)

  • Suppresses the indices column in the preview table

  • Removes the column headers entirely since they don't serve any purpose in this type of "columnar table"

@NotTheDr01ds
Copy link
Contributor Author

Thought: I'm wondering if Asciinema could be used to create smaller, more consistent/reproducible theme previews.

@fdncred
Copy link
Collaborator

fdncred commented Jul 22, 2024

Nice work!

Adds a new method for generating screenshots using PowerShell with no third-party application required. Created new function for choosing the method that is used to generate screenshots.

I didn't know that was a thing. Does that work on all platforms too (mac/linux/windows)?

Thought: I'm wondering if Asciinema could be used to create smaller, more consistent/reproducible theme previews.

I'd be open to a more compact way of showing this. I once thought markdown with html tags indicating colors would do it, but I never pursued it. I'd only be open to asciinema you could view the preview inline without clicking on it or taking you to another site.

@NotTheDr01ds
Copy link
Contributor Author

NotTheDr01ds commented Jul 22, 2024

I didn't know that was a thing. Does that work on all platforms too (mac/linux/windows)?

That's a good question. I was thinking Windows-only, but since PowerShell and .Net are cross-platform, there's a chance it could work (with slight modifications).

That said, making both of those be dependencies for generating previews might be a bit overkill.

I'd only be open to asciinema you could view the preview inline without clicking on it or taking you to another site.

As far as I know, that's completely possible. I'll try a quick single-theme prototype and see how it looks.

@fdncred
Copy link
Collaborator

fdncred commented Jul 22, 2024

Also, I'm not clear on the API here. How does this stuff work?

I like the record view vs a table view in the screenshots.

@fdncred fdncred merged commit 995c1ea into nushell:main Jul 22, 2024
1 check passed
@fdncred
Copy link
Collaborator

fdncred commented Jul 22, 2024

it's better than before so let's go ahead and land it. if we want to do asciinema we can in another pr. thanks.

@fdncred
Copy link
Collaborator

fdncred commented Jul 22, 2024

Seems like background, foreground, and cursor are a little bit weird. I'm not sure they're right after looking at the screenshots.

So, we can't have background text be the color of the background or it would be invisible. I'm wondering how we can represent this better? How do other theme sites represent the background?

Also, this isn't a criticism of anything you've done but looking at the nushell value colors like int, float, string, etc. The themes all stink. In my theme, most of these are different colors so I can easily differentiate them in a table. I wonder if there's a certain pattern we could apply based on the input where we could color every nushell value with a slightly different color? or maybe just have a rainbow setting that does this. Not sure what the right direction is.

@NotTheDr01ds
Copy link
Contributor Author

NotTheDr01ds commented Jul 22, 2024

Also, I'm not clear on the API here. How does this stuff work?

If you mean the module, it's basically just the old preview-theme.nu that you used to source, but now it's moved to a module and exported (and renamed the command).

So:

> # Old
> source preview-screenshot-script.nu
> preview_screenshot_small

> # New
> use .. *
> # Or, if using Nupm or the nu_scripts is in the library path then
> use themes *
> preview screenshot small

preview_terminal.nu is also moved into the module.

I like the record view vs a table view in the screenshots.

Well I was going to say the same to you! It's still pretty much the same table that you came up with in #519 to bucket the record into three columns. It's just that I prettied it up by chopping off the header (manually, via regex) and suppressed the indices. I also think the group refactor is a bit cleaner (and fixes the bug to boot).

Seems like background, foreground, and cursor are a little bit weird. I'm not sure they're right after looking at the screenshots.

I was a bit surprised at red for a cursor color choice in the nushell-dark/light themes ... I assume that's what you are talking about?

So, we can't have background text be the color of the background or it would be invisible. I'm wondering how we can represent this better? How do other theme sites represent the background?

Ah yeah, forgot to mention that change.

Might could invert the foreground/background there? I'm flexible. My main goal was just to make the information visible, even if I wasn't representing the actual color.

looking at the nushell value colors like int, float, string

Agreed - I had the same thought while looking at it. The actual shapes do have independent colors, but the value representations do not.

I've often thought that jq pretty-prints JSON better than Nushell, and this may be the reason. It does differentiate types in the output.

@fdncred
Copy link
Collaborator

fdncred commented Jul 22, 2024

Seems like background, foreground, and cursor are a little bit weird. I'm not sure they're right after looking at the screenshots.

I was a bit surprised at red for a cursor color choice in the nushell-dark/light themes ... I assume that's what you are talking about?

Nope. When I created this preview, I was trying to find a way to represent the colors so that people know what it looks like without installing a theme. In my comment, I was specifically talking about fg, bg, cursor colors. Now that I think about it, fg is probably ok since it should be colored the same as the line. Like I said, we can't make bg text the color of the background because it would then be invisible. Maybe inverting the background would be ok. Not sure, I'd have to see a few. Or maybe draw the bg text with the fg as the bg color and the bg text as the fg color. (confusing for me to say, lol). Just trying to think of a way to make bg look more visible.

@fdncred
Copy link
Collaborator

fdncred commented Jul 24, 2024

I just noticed that the PNG files generated are more than 100k bigger as the previous ones. We should probably compress them better.
image

I'm wondering if we could implement this CI to make sure they're always compressed? https://github.com/nushell/nushell.github.io/blob/main/.github/workflows/image-actions.yml

@NotTheDr01ds
Copy link
Contributor Author

Ah, I didn't compare before-and-after. I did try with Jpeg, thinking those would be smaller, but the PowerShell/.Net representation from the clipboard capture was even larger!

I'm working on seeing how the Asciinema versions work now - It will auto-create a PNG that links to the recording, so let me see how large that is.

@fdncred
Copy link
Collaborator

fdncred commented Jul 25, 2024

I added the ci anyway

@NotTheDr01ds NotTheDr01ds deleted the theme-preview branch July 25, 2024 18:48
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.

2 participants