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

fix nu-themes package #899

Closed
wants to merge 3 commits into from
Closed

fix nu-themes package #899

wants to merge 3 commits into from

Conversation

amtoine
Copy link
Member

@amtoine amtoine commented Jul 19, 2024

description

this PR puts back a few themes that have been removed in the previous PR and fixes the commands in the README.

a few questions

@NotTheDr01ds, i have a few questions too :)

  • is it required to have a separate activate module? why not directly put the export-env in the module?
  • i don't understand the update terminal command, what does it do?

@amtoine amtoine marked this pull request as draft July 19, 2024 08:44
@amtoine amtoine marked this pull request as ready for review July 19, 2024 08:51
@fdncred
Copy link
Collaborator

fdncred commented Jul 19, 2024

update terminal uses the osc ansi escapes 10, 11, 12 to change the foreground color, background color, and cursor color in terminals that support it.

@amtoine
Copy link
Member Author

amtoine commented Jul 19, 2024

update terminal uses the osc ansi escapes 10, 11, 12 to change the foreground color, background color, and cursor color in terminals that support it.

yeah, but when i run the following

source nu-themes/dracula.nu

it immediately changes the theme to Dracula for me

then, the following does not appear to change anything, that's why im not sure i fully understand this command 😉

use nu-themes/dracula.nu
dracula update terminal

@fdncred
Copy link
Collaborator

fdncred commented Jul 19, 2024

Correct. This is a bug that @NotTheDr01ds has filed. nushell/nushell#13403

@NotTheDr01ds
Copy link
Contributor

NotTheDr01ds commented Jul 19, 2024

@amtoine Perhaps I need to make the README more clear, but:

Before the change:

  • Terminal OSC codes were never updated automatically:

    use <theme_file>
    $env.config.color_config = (<theme>)
    

    That only updated the color_config, but not the terminal's colors (OSC codes). There was no code, nor instructions in the README, on how to update the terminal colors. I only found that code in @fdncred's preview scripts.

After the change:

  • source'ing it will update everything (including OSC)
  • use'ing it will allow the user to set or query elements individually. This provides full backwards compatibility and allows users to have it to still work like it used to, which is what I personally want - I don't want to update my background color when using a new "dark" theme - Most of them change it from deep black to a gray-level.

As for this PR - I'm in the process of updating the old themes (manual) so that they work like the updated ones do. While I'm fine with the "old" ones going in temporarily, I'm working on making sure that they don't get lost again by having their generation be part of the make.nu script.

@NotTheDr01ds
Copy link
Contributor

Correct. This is a bug that ...

Not quite - The behavior that @amtoine describes is correct. If you source the theme, it's fully activated, then a <theme> update terminal isn't going to have any effect, because that code was already run when it was sourced.

@amtoine
Copy link
Member Author

amtoine commented Jul 19, 2024

oooooooh, i understand now @NotTheDr01ds !!
i was confused about the background changing color on source 😱

your explanation is much much clearer, thanks a lot 🙏
could you mention this in the README?

  • maybe move the first source out of the "installation" section
  • mention that set color_config won't change the background whereas source ... and update terminal will (the background is the easiest to see it appears)

@NotTheDr01ds
Copy link
Contributor

NotTheDr01ds commented Jul 19, 2024

@amtoine A couple of questions:

  • Can we close this one in favor of Recover non-lemnos themes #902?
  • Should the Nupm instructions be moved to the root README at the nu_scripts level, since that's what actually gets installed, right?

@amtoine
Copy link
Member Author

amtoine commented Jul 20, 2024

yup, we can do that, thanks @NotTheDr01ds 🙏

@amtoine amtoine closed this Jul 20, 2024
@amtoine amtoine deleted the fix-themes branch July 20, 2024 09:01
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.

3 participants