Apply theme updates to implicitly-set universal color variables

On first run we copy the default theme to universal variables for colors
(since these defaults are not included in the binary).

Similarly, we have "fish_config theme save", which copies the given theme
to the same universal variables.

Sometimes we make changes to a default theme.  Existing users won't see
those updates by default because previous defaults have been turned into
universal variables, which are indistinguishable from user-set variables.

This also means that installing fish on two systems might result in different
colors if the initially installed versions differ.  This is surprising.

It feels difficult to get rid of universal variables here without breaking
features such as instant propagation on "set fish_color_normal blue"
or tab-completion on "set fish_color" showing the current value even when
that's a default.

Assuming that we don't want to stop setting these universal variables,
try a different solution: keep track of whether a variable wants to follow
future updates/removals.
This is opt-in via "--track" (e.g. "fish_config theme save --track THEME")
but crucially, enabled for the default theme when that one is saved on first launch.
(Of course this means that this enhancement only works on new installations.)

This allows customizing individual color variables (which likely means
removing --track marker) while still getting updates for other color variables.

Note that whenever we add a new color, we still need to write a migration
(see "__fish_initialized") that initializes this color in the universal
scope with the "--track" marker.

TODO:
- add a "follow future updates to this theme" checkbox to webconfig
- changelog

doc_src/cmds/fish_config.rst

@@ -10,7 +10,7 @@ Synopsis
 
     fish_config [browse]
     fish_config prompt (choose | list | save | show)
-    fish_config theme (choose | demo | dump | list | save | show)
+    fish_config theme (choose | demo | dump | list | save | show | update)
 
 Description
 -----------
@@ -40,6 +40,9 @@ Available subcommands for the ``theme`` command:
 - ``list`` lists the names of the available sample themes.
 - ``save`` saves the given theme to :ref:`universal variables <variables-universal>`.
 - ``show`` shows what the given sample theme (or all) would look like.
+- ``update`` updates any ``fish_color_*`` and ``fish_pager_color_*`` variables whose value contains
+             "--track=THEME". They are set to the latest version of that theme, and the tracking
+             option is preserved. Note that ``fish_config theme update`` is run at fish startup.
 
 The **-h** or **--help** option displays help about using this command.
 

doc_src/cmds/set_color.rst

@@ -59,6 +59,12 @@ The following options are available:
 **-u** or **--underline**, or **-uSTYLE** or **--underline=STYLE**
     Set the underline mode; supported styles are **single** (default) and **curly**.
 
+**--track=THEME**
+    Ignored. Included by default in universally-scoped color variables to tell fish to update
+    them as the associated theme changes.
+    This flag can be set for all variables when loading a theme with the `--track`` option, that is
+    :doc:`fish_config theme save THEME --track <fish_config>`.
+
 **-h** or **--help**
     Displays help about using this command.
 

share/completions/fish_config.fish

@@ -1,6 +1,6 @@
 complete fish_config -f
 set -l prompt_commands choose save show list
-set -l theme_commands choose demo dump save show list
+set -l theme_commands choose demo dump save show list update
 complete fish_config -n __fish_use_subcommand -a prompt -d 'View and pick from the sample prompts'
 complete fish_config -n "__fish_seen_subcommand_from prompt; and not __fish_seen_subcommand_from $prompt_commands" \
     -a choose -d 'View and pick from the sample prompts'
@@ -16,6 +16,7 @@ complete fish_config -n __fish_use_subcommand -a browse -d 'Open the web-based U
 
 complete fish_config -n __fish_use_subcommand -a theme -d 'View and pick from the sample themes'
 complete fish_config -n '__fish_seen_subcommand_from theme; and __fish_seen_subcommand_from choose save show' -a '(fish_config theme list)'
+complete fish_config -n '__fish_seen_subcommand_from theme; and __fish_seen_subcommand_from save' -l track -d 'Add --track to color variables to apply future theme updates'
 complete fish_config -n "__fish_seen_subcommand_from theme; and not __fish_seen_subcommand_from $theme_commands" \
     -a choose -d 'View and pick from the sample themes'
 complete fish_config -n "__fish_seen_subcommand_from theme; and not __fish_seen_subcommand_from $theme_commands" \
@@ -28,3 +29,5 @@ complete fish_config -n "__fish_seen_subcommand_from theme; and not __fish_seen_
     -a demo -d 'Show example in the current theme'
 complete fish_config -n "__fish_seen_subcommand_from theme; and not __fish_seen_subcommand_from $theme_commands" \
     -a dump -d 'Print the current theme in .theme format'
+complete fish_config -n "__fish_seen_subcommand_from theme; and not __fish_seen_subcommand_from $theme_commands" \
+    -a update -d "Update universal colors that have the tracking flag set"

share/completions/set_color.fish

@@ -8,3 +8,4 @@ complete -c set_color -s r -l reverse -d 'Reverse color text'
 complete -c set_color -s u -l underline -d 'Underline style' -a 'single curly'
 complete -c set_color -s h -l help -d 'Display help and exit'
 complete -c set_color -s c -l print-colors -d 'Print a list of all accepted color names'
+complete -c set_color -l track -xa '(fish_config theme list)' -d 'Ignored. Used in color variables to follow theme changes'

share/functions/__fish_config_interactive.fish

@@ -30,12 +30,13 @@ if status is-interactive
     # Commands to run in interactive sessions can go here
 end" >$__fish_config_dir/config.fish
 
-        echo yes | fish_config theme save "fish default"
+        echo yes | fish_config theme save "fish default" --track
         set -Ue fish_color_keyword fish_color_option
     end
     if test $__fish_initialized -lt 3800 && test "$fish_color_search_match[1]" = bryellow
         set --universal fish_color_search_match[1] white
     end
+    fish_config theme update
 
     #
     # Generate man page completions if not present.

share/functions/fish_config.fish

@@ -1,5 +1,11 @@
-function fish_config --description "Launch fish's web based configuration"
-    argparse h/help -- $argv
+# Variables a theme is allowed to set
+set -l theme_var_filter '^fish_(?:pager_)?color.*$'
+
+function fish_config --description "Launch fish's web based configuration" \
+    --inherit-variable theme_var_filter
+
+    set -l _flag_track
+    argparse h/help track -- $argv
     or return
 
     if set -q _flag_help
@@ -10,6 +16,11 @@ function fish_config --description "Launch fish's web based configuration"
     set -l cmd $argv[1]
     set -e argv[1]
 
+    if set -q _flag_track[1] && not { test "$cmd" = theme && test "$argv[1]" = save }
+        echo >&2 fish_config: --track: unknown option
+        return 1
+    end
+
     set -q cmd[1]
     or set cmd browse
 
@@ -65,9 +76,6 @@ 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
@@ -292,34 +300,7 @@ function fish_config --description "Launch fish's web based configuration"
                     # 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 $dirs/$argv[1].theme
-                        set -l file
-
-                        for f in $files
-                            if test -e "$f"
-                                set file $f
-                                break
-                            end
-                        end
-
-                        if not set -q file[1]
-                            if status list-files tools/web_config/themes/$argv[1].theme &>/dev/null
-                                set file tools/web_config/themes/$argv[1].theme
-                            else
-                                echo "No such theme: $argv[1]" >&2
-                                echo "Searched directories: $dirs" >&2
-                                return 1
-                            end
-                        end
-
-                        set -l content
-                        if string match -qr '^tools/' -- $file
-                            set content (status get-file $file)
-                        else
-                            read -z content < $file
-                        end
-
-                        printf %s\n $content | while read -lat toks
+                        __fish_config_theme_get $argv[1] | while read -lat toks
                             # 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.
@@ -331,7 +312,7 @@ function fish_config --description "Launch fish's web based configuration"
                             if test x"$scope" = x-U; and set -qg $toks[1]
                                 set -eg $toks[1]
                             end
-                            set $scope $toks
+                            set $scope $toks $_flag_track=$argv[1]
                             set -a have_colors $toks[1]
                         end
 
@@ -343,7 +324,7 @@ function fish_config --description "Launch fish's web based configuration"
                             # Erase conflicting global variables so we don't get a warning and
                             # so changes are observed immediately.
                             set -eg $c
-                            set $scope $c
+                            set $scope $c $_flag_track=$argv[1]
                         end
                     else
                         # We're persisting whatever current colors are loaded (maybe in the global scope)
@@ -365,6 +346,8 @@ function fish_config --description "Launch fish's web based configuration"
                     # 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 update
+                    __fish_config_theme_update $argv
                 case dump
                     # Write the current theme in .theme format, to stdout.
                     set -L | string match -r $theme_var_filter
@@ -374,3 +357,88 @@ function fish_config --description "Launch fish's web based configuration"
             end
     end
 end
+
+function __fish_config_theme_get
+    set -l dirs $__fish_config_dir/themes $__fish_data_dir/tools/web_config/themes
+    set -l files $dirs/$argv[1].theme
+    set -l file
+
+    for f in $files
+        if test -e "$f"
+            set file $f
+            break
+        end
+    end
+
+    if not set -q file[1]
+        if status list-files tools/web_config/themes/$argv[1].theme &>/dev/null
+            set file tools/web_config/themes/$argv[1].theme
+        else
+            echo "No such theme: $argv[1]" >&2
+            echo "Searched directories: $dirs" >&2
+            return 1
+        end
+    end
+
+    if string match -qr '^tools/' -- $file
+        status get-file $file
+    else
+        string join \n <$file
+    end
+end
+
+function __fish_config_show_tracked_color_vars
+    set -l color_var $argv[1]
+    set -l _flag_track
+    argparse --ignore-unknown track= -- _set_color $argv[2..]
+    or return $status
+    if not set -q _flag_track[1]
+        return
+    end
+    if set -q _flag_track[2]
+        echo >&2 "fish_config: $color_var: --track option can only be specified once"
+        exit 1
+    end
+    if test (printf %s $_flag_track | count) -ne 0
+        echo >&2 "fish_config: $color_var: error: tracking theme name must not contain newlines"
+        exit 1
+    end
+    printf %s\n $color_var $_flag_track
+end
+
+function __fish_config_theme_update --inherit-variable theme_var_filter
+    if set -q argv[1]
+        echo "fish_config: too many arguments" >&2
+        return 1
+    end
+    set -l themes
+
+    set -l tracking_variables (
+            set --universal --long |
+            string match -r '^fish_(?:pager_)?color.*$' |
+            string replace -r '.*' '__fish_config_show_tracked_color_vars $0' |
+            source
+    )
+    or return $status
+    string join \n $tracking_variables |
+        while read --line _colorvar theme
+            if not contains -- $theme $themes
+                set -a themes $theme
+            end
+        end
+
+    for theme in $themes
+        set -l colorvars
+        string join \n $tracking_variables |
+            while read --line color_var t
+                if test $t = $theme
+                    set -a colorvars $color_var
+                end
+            end
+        set -l theme_escaped (string escape -- $theme)
+        __fish_config_theme_get $theme |
+            string match -r -- "^(?:$(string join '|' $colorvars))\b .*" |
+            string replace -r '.*' "set -U \$0 --track=$theme_escaped" |
+            source
+    end
+end

src/builtins/set_color.rs

@@ -86,6 +86,13 @@ pub fn set_color(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) -
                 // In future we change both to actually print an error.
                 return Err(STATUS_INVALID_ARGS);
             }
+            Err(MultipleTracking) => {
+                streams.err.append(wgettext_fmt!(
+                    "%ls: --track option can only be specified once\n",
+                    argv[0]
+                ));
+                return Err(STATUS_INVALID_ARGS);
+            }
             Err(UnknownColor(arg)) => {
                 streams
                     .err
@@ -124,7 +131,7 @@ pub fn set_color(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) -
         specified_face.fg.unwrap_or(Color::None),
         specified_face.bg.unwrap_or(Color::None),
         specified_face.underline_color.unwrap_or(Color::None),
-        specified_face.style,
+        specified_face.style.unwrap_or_default(),
     ));
 
     if specified_face.fg.is_some() && outp.contents().is_empty() {

src/highlight/highlight.rs

@@ -28,7 +28,7 @@
 };
 use crate::path::{path_as_implicit_cd, path_get_cdpath, path_get_path, paths_are_same_file};
 use crate::terminal::Outputter;
-use crate::text_face::{parse_text_face, TextFace, UnderlineStyle};
+use crate::text_face::{parse_text_face, SpecifiedTextFace, TextFace, UnderlineStyle};
 use crate::threads::assert_is_background_thread;
 use crate::tokenizer::{variable_assignment_equals_pos, PipeOrRedir};
 use crate::wchar::{wstr, WString, L};
@@ -140,12 +140,16 @@ pub(crate) fn resolve_spec_uncached(
         vars: &dyn Environment,
     ) -> TextFace {
         let resolve_role = |role| {
-            vars.get_unless_empty(get_highlight_var_name(role))
-                .or_else(|| vars.get_unless_empty(get_highlight_var_name(get_fallback(role))))
-                .or_else(|| vars.get_unless_empty(get_highlight_var_name(HighlightRole::normal)))
-                .as_ref()
-                .map(parse_text_face_for_highlight)
-                .unwrap_or_else(TextFace::default)
+            for role in [role, get_fallback(role), HighlightRole::normal] {
+                if let Some(face) = vars
+                    .get_unless_empty(get_highlight_var_name(role))
+                    .as_ref()
+                    .and_then(parse_text_face_for_highlight)
+                {
+                    return face;
+                }
+            }
+            TextFace::default()
         };
         let mut face = resolve_role(highlight.foreground);
 
@@ -162,7 +166,8 @@ pub(crate) fn resolve_spec_uncached(
         if highlight.valid_path {
             if let Some(valid_path_var) = vars.get(L!("fish_color_valid_path")) {
                 // Historical behavior is to not apply background.
-                let valid_path_face = parse_text_face_for_highlight(&valid_path_var);
+                let valid_path_face =
+                    parse_text_face_for_highlight(&valid_path_var).unwrap_or_default();
                 // Apply the foreground, except if it's normal. The intention here is likely
                 // to only override foreground if the valid path color has an explicit foreground.
                 if !valid_path_face.fg.is_normal() {
@@ -181,19 +186,21 @@ pub(crate) fn resolve_spec_uncached(
 }
 
 /// Return the internal color code representing the specified color.
-pub(crate) fn parse_text_face_for_highlight(var: &EnvVar) -> TextFace {
+pub(crate) fn parse_text_face_for_highlight(var: &EnvVar) -> Option<TextFace> {
     let face = parse_text_face(var.as_list());
-    let default = TextFace::default();
-    let fg = face.fg.unwrap_or(default.fg);
-    let bg = face.bg.unwrap_or(default.bg);
-    let underline_color = face.underline_color.unwrap_or(default.underline_color);
-    let style = face.style;
-    TextFace {
-        fg,
-        bg,
-        underline_color,
-        style,
-    }
+    (face != SpecifiedTextFace::default()).then(|| {
+        let default = TextFace::default();
+        let fg = face.fg.unwrap_or(default.fg);
+        let bg = face.bg.unwrap_or(default.bg);
+        let underline_color = face.underline_color.unwrap_or(default.underline_color);
+        let style = face.style.unwrap_or_default();
+        TextFace {
+            fg,
+            bg,
+            underline_color,
+            style,
+        }
+    })
 }
 
 fn command_is_valid(

src/reader.rs

@@ -2688,7 +2688,9 @@ fn handle_readline_command(&mut self, c: ReadlineCmd) {
 
                     let mut outp = Outputter::stdoutput().borrow_mut();
                     if let Some(fish_color_cancel) = self.vars().get(L!("fish_color_cancel")) {
-                        outp.set_text_face(parse_text_face_for_highlight(&fish_color_cancel));
+                        outp.set_text_face(
+                            parse_text_face_for_highlight(&fish_color_cancel).unwrap_or_default(),
+                        );
                     }
                     outp.write_wstr(L!("^C"));
                     outp.reset_text_face();

src/text_face.rs

@@ -111,6 +111,12 @@ pub(crate) struct TextFace {
     pub(crate) style: TextStyling,
 }
 
+impl Default for TextFace {
+    fn default() -> Self {
+        Self::default()
+    }
+}
+
 impl TextFace {
     pub const fn default() -> Self {
         Self {
@@ -131,12 +137,12 @@ pub fn new(fg: Color, bg: Color, underline_color: Color, style: TextStyling) ->
     }
 }
 
-#[derive(Default)]
+#[derive(Default, Eq, PartialEq)]
 pub(crate) struct SpecifiedTextFace {
     pub(crate) fg: Option<Color>,
     pub(crate) bg: Option<Color>,
     pub(crate) underline_color: Option<Color>,
-    pub(crate) style: TextStyling,
+    pub(crate) style: Option<TextStyling>,
 }
 
 pub(crate) fn parse_text_face(arguments: &[WString]) -> SpecifiedTextFace {
@@ -167,6 +173,7 @@ pub(crate) enum ParsedArgs<'argarray, 'args> {
 
 pub(crate) enum ParseError<'args> {
     MissingOptArg,
+    MultipleTracking,
     UnknownColor(&'args wstr),
     UnknownUnderlineStyle(&'args wstr),
     UnknownOption(usize),
@@ -189,6 +196,7 @@ pub(crate) fn parse_text_face_and_options<'argarray, 'args>(
         wopt(L!("reverse"), ArgType::NoArgument, 'r'),
         wopt(L!("help"), ArgType::NoArgument, 'h'),
         wopt(L!("print-colors"), ArgType::NoArgument, 'c'),
+        wopt(L!("track"), ArgType::RequiredArgument, '\x01'),
     ];
     let long_options = &long_options[..long_options.len() - builtin_extra_args];
 
@@ -210,6 +218,7 @@ pub(crate) fn parse_text_face_and_options<'argarray, 'args>(
     let mut underline_colors = vec![];
     let mut style = TextStyling::default();
     let mut print_color_mode = false;
+    let mut tracking = false;
 
     let mut w = WGetopter::new(short_options, long_options, argv);
     while let Some(c) = w.next_opt() {
@@ -219,6 +228,12 @@ pub(crate) fn parse_text_face_and_options<'argarray, 'args>(
                     bg_colors.push(bg);
                 }
             }
+            '\x01' => {
+                if is_builtin && tracking {
+                    return Err(MultipleTracking);
+                }
+                tracking = true;
+            }
             '\x02' => {
                 if let Some(underline_color) = parse_color(w.woptarg.unwrap())? {
                     underline_colors.push(underline_color);
@@ -297,6 +312,6 @@ pub(crate) fn parse_text_face_and_options<'argarray, 'args>(
         fg,
         bg,
         underline_color,
-        style,
+        style: (style != TextStyling::default()).then_some(style),
     }))
 }

tests/checks/fish_config.fish

@@ -0,0 +1,40 @@
+#RUN: %fish %s
+
+mkdir $__fish_config_dir/themes
+echo >$__fish_config_dir/themes/foo.theme 'fish_color_normal cyan'
+
+set -g fish_pager_color_secondary_background custom-value
+fish_config theme choose foo
+
+set -S fish_color_normal
+# CHECK: $fish_color_normal: set in global scope, unexported, with 1 elements
+# CHECK: $fish_color_normal[1]: |cyan|
+set -S fish_pager_color_secondary_background
+# CHECK: $fish_pager_color_secondary_background: set in global scope, unexported, with 0 elements
+
+function change-theme
+    echo >$__fish_config_dir/themes/fake-default.theme 'fish_color_command' $argv
+end
+
+change-theme 'green'
+echo yes | fish_config theme save --track fake-default
+echo $fish_color_command
+# CHECK: green --track=fake-default
+
+change-theme 'green --bold'
+fish_config theme update
+
+echo $fish_color_command
+# CHECK: green --bold --track=fake-default
+
+# Test that we silently update when there is a shadowing global.
+change-theme 'green --italics'
+set -g fish_color_command normal
+fish_config theme update
+set -S fish_color_command
+# CHECK: $fish_color_command: set in global scope, unexported, with 1 elements
+# CHECK: $fish_color_command[1]: |normal|
+# CHECK: $fish_color_command: set in universal scope, unexported, with 3 elements
+# CHECK: $fish_color_command[1]: |green|
+# CHECK: $fish_color_command[2]: |--italics|
+# CHECK: $fish_color_command[3]: |--track=fake-default|