From 22332b892d0657902a0e27a9703d28d5588e1689 Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Wed, 12 Oct 2022 21:10:15 -0500 Subject: [PATCH 1/2] Fix `fish_config theme save` The documentation states that running `fish_config theme save` after `fish_config theme choose [theme_name]` will result in "saving" the currently chosen theme, but this does not match the actual behavior of `fish_config theme save` which expects a trailing argument specifying the name of the theme to select/persist. Given that the documented way has been included in a release and that it makes more sense than calling `fish_config theme save xxx` when you are *loading from* xxx and not *saving to* xxx, this patch revises `fish_config.fish` to support the documented behavior. When `fish_config theme save xxx` is used, xxx is loaded w/ its specified colors saved to the according variables in the universal scope. But if `fish_config theme save` is used without a theme's name specified, then the currently specified (known) fish color variables are persisted from whatever scope they're currently in (usually in the global scope from previewing a theme) to universal variables of the same name. This does *not* catch color variables unknown to fish! If a theme and a prompt agree on some variable to hold some color but it's not a color variable known to fish, it won't be persisted! Closes #9088. --- share/functions/fish_config.fish | 109 ++++++++++++++++++------------- 1 file changed, 64 insertions(+), 45 deletions(-) diff --git a/share/functions/fish_config.fish b/share/functions/fish_config.fish index 1533ab967..47fd279ec 100644 --- a/share/functions/fish_config.fish +++ b/share/functions/fish_config.fish @@ -205,17 +205,17 @@ function fish_config --description "Launch fish's web based configuration" echo "Too many arguments" >&2 return 1 end - if not set -q argv[1] + # The name of the theme to save *from* is optional for `fish_config theme save` + if not set -q argv[1] && contains -- $cmd choose echo "Too few arguments" >&2 return 1 end - set -l files $dir/$argv[1].theme - set -l file - set -l scope -g + set -l have_colors + if contains -- $cmd save - read -P"Overwrite theme? [y/N]" -l yesno + read -P"Overwrite your current theme? [y/N] " -l yesno if not string match -riq 'y(es)?' -- $yesno echo Not overwriting >&2 return 1 @@ -223,19 +223,6 @@ function fish_config --description "Launch fish's web based configuration" set scope -U end - for f in $files - if test -e "$f" - set file $f - break - end - end - - if not set -q file[1] - echo "No such theme: $argv[1]" >&2 - echo "Dirs: $dir" >&2 - return 1 - end - set -l known_colors fish_color_{normal,command,keyword,quote,redirection,\ end,error,param,option,comment,selection,operator,escape,autosuggestion,\ cwd,user,host,host_remote,cancel,search_match} \ @@ -243,39 +230,71 @@ function fish_config --description "Launch fish's web based configuration" selected_background,selected_prefix,selected_completion,selected_description,\ secondary_background,secondary_prefix,secondary_completion,secondary_description} + # If we are choosing a theme or saving from a named theme, load the theme now. + # Otherwise, we'll persist the currently loaded/themed variables (in case of `theme save`). + if set -q argv[1] + set -l files $dir/$argv[1].theme + set -l file - set -l have_colors - while read -lat toks - # We only allow color variables. - # Not the specific list, but something named *like* a color variable. - # - # This also takes care of empty lines and comment lines. - string match -rq '^fish_(?:pager_)?color.*$' -- $toks[1] - or continue - - # If we're supposed to set universally, remove any shadowing globals, - # so the change takes effect immediately (and there's no warning). - if test x"$scope" = x-U; and set -qg $toks[1] - set -eg $toks[1] + for f in $files + if test -e "$f" + set file $f + break + end end - set $scope $toks - set -a have_colors $toks[1] - end <$file - # Set all colors that aren't mentioned to empty - for c in $known_colors - contains -- $c $have_colors - and continue + if not set -q file[1] + echo "No such theme: $argv[1]" >&2 + echo "Searched directories: $dir" >&2 + return 1 + end - # Erase conflicting global variables so we don't get a warning and - # so changes are observed immediately. - set -eg $c - set $scope $c + while read -lat toks + # We only allow color variables. + # Not the specific list, but something named *like* a color variable. + # + # This also takes care of empty lines and comment lines. + string match -rq '^fish_(?:pager_)?color.*$' -- $toks[1] + or continue + + # If we're supposed to set universally, remove any shadowing globals + # so the change takes effect immediately (and there's no warning). + if test x"$scope" = x-U; and set -qg $toks[1] + set -eg $toks[1] + end + set $scope $toks + set -a have_colors $toks[1] + end <$file + + # Set all colors that aren't mentioned to empty + for c in $known_colors + contains -- $c $have_colors + and continue + + # Erase conflicting global variables so we don't get a warning and + # so changes are observed immediately. + set -eg $c + set $scope $c + end + else + # We're persisting whatever current colors are loaded (maybe in the local scope) + # to the universal scope, without overriding them from a theme file. + # Like above, make sure to erase from other scopes first. This branch is only + # reachable in the case of `theme save` so $scope is always `-U`. + + for color in $known_colors + if set -q $color + # Cache the value from whatever scope currently defines it + set -l value $$color + set -eg $color + set -U $color "$value" + end + end end - # Return true if we changed at least one color - set -q have_colors[1] - return + # If we've made it this far, we've either found a theme file or persisted the current + # state (if any). In all cases we haven't failed, so return 0. + return 0 case dump # Write the current theme in .theme format, to stdout. set -L | string match -r '^fish_(?:pager_)?color.*$' From 201a0d7319e8fdf3a56c9c98cadb4c965f7b5ed8 Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Sat, 15 Oct 2022 14:24:31 -0500 Subject: [PATCH 2/2] Persist all color-like variables in `fish_config theme save` Don't just save known color values but any values that could have been loaded from a .theme file. Also, refactor the theme variable name whitelist/filter in a shared "global" variable so we never forget to update it at any of the individual use sites. --- share/functions/fish_config.fish | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/share/functions/fish_config.fish b/share/functions/fish_config.fish index 47fd279ec..9ce97ebeb 100644 --- a/share/functions/fish_config.fish +++ b/share/functions/fish_config.fish @@ -37,6 +37,9 @@ function fish_config --description "Launch fish's web based configuration" return 1 end + # Variables a theme is allowed to set + set -l theme_var_filter '^fish_(?:pager_)?color.*$'; + switch $cmd case prompt # prompt - for prompt switching @@ -250,11 +253,10 @@ function fish_config --description "Launch fish's web based configuration" end while read -lat toks - # We only allow color variables. + # The whitelist allows only color variables. # Not the specific list, but something named *like* a color variable. - # # This also takes care of empty lines and comment lines. - string match -rq '^fish_(?:pager_)?color.*$' -- $toks[1] + string match -rq -- $theme_var_filter $toks[1] or continue # If we're supposed to set universally, remove any shadowing globals @@ -277,12 +279,13 @@ function fish_config --description "Launch fish's web based configuration" set $scope $c end else - # We're persisting whatever current colors are loaded (maybe in the local scope) + # We're persisting whatever current colors are loaded (maybe in the global scope) # to the universal scope, without overriding them from a theme file. - # Like above, make sure to erase from other scopes first. This branch is only - # reachable in the case of `theme save` so $scope is always `-U`. + # Like above, make sure to erase from other scopes first and ensure known color + # variables are defined, even if empty. + # This branch is only reachable in the case of `theme save` so $scope is always `-U`. - for color in $known_colors + for color in (printf "%s\n" $known_colors (set --names | string match -r $theme_var_filter) | sort -u) if set -q $color # Cache the value from whatever scope currently defines it set -l value $$color @@ -297,7 +300,7 @@ function fish_config --description "Launch fish's web based configuration" return 0 case dump # Write the current theme in .theme format, to stdout. - set -L | string match -r '^fish_(?:pager_)?color.*$' + set -L | string match -r $theme_var_filter case '*' echo "No such command: $cmd" >&2 return 1