search

nushell / nu_scripts

A place to share Nushell scripts with each other
MIT License
743 stars 227 forks source link

Simplify theme usage #896

Closed NotTheDr01ds closed 3 months ago

NotTheDr01ds commented 3 months ago
fdncred commented 3 months ago

Nice! Do all the preview-*.nu scripts still work?

NotTheDr01ds commented 3 months ago

Nice! Do all the preview-*.nu scripts still work?

I'm ... uh, not sure! What and where are they?

NotTheDr01ds commented 3 months ago

Also not sure why CI is failing. Is toolkit check pr really working? I took a look at it yesterday and it looks fairly broken, but I haven't gone too far into it. In this case, it looks like it's expecting make.nu to be a module, but it fails because it's a script.

fdncred commented 3 months ago

What and where are they?

In the themes directory. It's what I used to generate all the screenshots.

fdncred commented 3 months ago

I'm not worried by the CI, it doesn't work very well.

NotTheDr01ds commented 3 months ago

What and where are they?

In the themes directory. It's what I used to generate all the screenshots.

Ah yeah - Just found theme a few seconds ago. Probably needs updating - Brb ;-)

NotTheDr01ds commented 3 months ago

I think it's a simple change, but:

  1. I can't test it at the moment under Linux where I'm working on the PR (I'll get something set up)
  2. It looks like there's at least one issue that predates this PR -- Your preview_theme_* assumes the directory is named themes/themes, but the repo directory is themes/nu-themes.

As I work on this, do you have a preference on what the directory should actually be named? I'm good with either one, but it seems like it's been changed at some point.

fdncred commented 3 months ago

I'm fine with the current name for folders but I've always thought the preview scripts belonged in a different folder since they're not themes. I just left them in there because it was just easier to generate the theme images.

I wrote those on Windows, but I wouldn't be opposed to having a version that works on Linux or MacOS.

NotTheDr01ds commented 3 months ago

I'm going to work on updating it so it will at least run on WSL and Windows native, but I don't have a Mac or pure-Linux (even virtual machine) at the moment.

I'm thinking we should at least move the screenshot code to a closure that can be run based on the host system. Then people can add screenshot code in the future for other platforms.

Anyway, I'm going to do some refactoring on it, and I'll commit more later - Probably tomorrow.

fdncred commented 3 months ago

I was playing with this PR this morning. I'm not a super fan of having double the files with the *_colors.nu files. I also couldn't try any of the themes without being in the nu-themes folder.

Maybe I'm just looking at it too early and it's not done yet?

NotTheDr01ds commented 3 months ago

As I was working through your preview scripts, I noticed that the main README and process for setting a theme has always been ... very wrong.

The modules here only set the $env.config.color_config to the theme colors. That's great, but the modules failed to set the terminal foreground, background, and cursor color. You do that correctly in your preview scripts, of course.

The latest commits fix this. While I could, of course, set the terminal colors and color_config in the same module, there's benefit to having this separated. I personally prefer the color_config but don't typically want to use the terminal colors. The new scheme creates two modules for each theme - One which has the definitions and no other side-effects, and the other (main) that actually changes the colors for both Nushell and the terminal.

Okay, now back to working on the preview scripts - They should be much easier now.

NotTheDr01ds commented 3 months ago

Maybe I'm just looking at it too early and it's not done yet?

Ah yes, definitely wasn't done yet (and still isn't). But it should have been "workable" in that state.

I was playing with this PR this morning. I'm not a super fan of having double the files with the *_colors.nu files. I also couldn't try any of the themes without being in the nu-themes folder.

I've tried every which way that I can to have a module that has the color definitions and can be use'd without also setting the colors, but I don't think it's possible. Either a module export-env's or not - There's no way to "optionally" import the module and have just the definitions loaded.

For most users, they'll just use nu-themes/<theme>.nu like always. That will now set the colors of the terminal and Nushell.

Users who prefer the older scheme (which I do) can load the definitions separately with use nu-themes/theme-colors/<theme>.nu * and have access to the definitions.

fdncred commented 3 months ago

I really don't want to have double the files for themes. It just makes them way more cumbersome.

NotTheDr01ds commented 3 months ago

Does it help that the files are now in a separate folder?

Otherwise, I think we're going to have to just throw away the PR (and that's okay), since we can't obtain the color definitions any other way without also changing the colors. That's not how it worked before, and I really don't want to lose the ability to set the $env.config.color_config without also changing the terminal colors.

I can put in an enhancement request for a way to do this in a single module, and then we can revisit if that gets implemented.

Or, we can go with the second module and fix it if that enhancement gets implemented.

I'm okay with any path.

NotTheDr01ds commented 3 months ago

Hmmm - One more thing to try to get them in a single file ... Probably won't work, but crossing my fingers.

NotTheDr01ds commented 3 months ago

Hmmm - One more thing to try to get them in a single file ... Probably won't work, but crossing my fingers.

Well, it didn't, but eventually I came up with something that allowed me to get rid of the duplicate module files.

@fdncred Thanks for pushing back on this. I didn't like it either, but I just hadn't found a good alternative. I think I'm happy with the latest iteration, and I hope you will as well.

The interesting thing about where I ended up is that it is fully backwards-compatible with the older version. In other words, this still works exactly the same:

> use ./themes/nu-themes/<theme_name>.nu
> $env.config.color_config = (<theme_name>)

But that still leaves the terminal colors untouched. If the user wants to set everything in one step, they will source the file instead of importing the module.

source ./themes/nu-themes/<theme_name>.nu

Note: I was trying to have two different submodules in the same file, but there seems to be a Nushell bug that prevents the export-env from running when importing a submodule, so that didn't work. I'll write that up, but in the meantime, I think the source option to activate works just as well - Happy to hear your thoughts.

Side-note: The 3024 theme is problematic, since the parser will always see that as a number rather than a command or module name. However, I'm fairly confident that bug existed already. We could special case it by changing the name to 3024-theme or something like that (manually, in make.nu), but I haven't worried about that at this point -- Is there a way to call a command that is purely a number?

NotTheDr01ds commented 3 months ago

Next up, if @fdncred is okay with this version, is to work on the preview scripts. They should still work on your system since the module-usage is the same once again. But I still hope to make them more universal.

fdncred commented 3 months ago

The 3024 theme is problematic, since the parser will always see that as a number rather than a command or module name.

Indeed, it's always been like that. That's why there was a 3024r.nu 3024-day.nu and 3024-night.nu.

Yup, I'm fine with this. Nice touch maintaining backwards compatibility.

I commented on your issue already about the other bug you found.

Let's go!

fdncred commented 3 months ago

Darn it! I just noticed this. These need to be restored, I think. Thoughts?

 delete mode 100644 themes/nu-themes/catppuccin-latte.nu
 delete mode 100644 themes/nu-themes/catppuccin-mocha.nu
 delete mode 100644 themes/nu-themes/nushell-dark.nu
 delete mode 100644 themes/nu-themes/nushell-light.nu
 delete mode 100644 themes/nu-themes/tokyo-moon.nu

I don't want to lose these additional themes.

NotTheDr01ds commented 3 months ago

@fdncred I'll probably figure this out about the same time you answer (seems to commonly be the case ;-), but were those manually defined or sourced from somewhere else?

fdncred commented 3 months ago

No worries. Manually defined from contributors.

NotTheDr01ds commented 3 months ago

Ok, I'll add them back in, and hopefully in a way that they won't get lost again since they'll be generated by make.nu in the future.

NotTheDr01ds commented 3 months ago

@fdncred Question on the preview-* scripts ...

I'm guessing that there was some limitation when you wrote it that required generation of a separate preview-screenshot-script.nu, but I think I can rewrite the main script to just cycle through the themes without requiring a secondary script. Any objection there or something else that I might be missing?

fdncred commented 3 months ago

No objections if it works. It's been a while since I wrote that so I don't remember exactly why I had to do that.