Re: [PATCH] git-gui: Update in-memory config when changing config options

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 26, 2019, 14:22 UTC
Message-ID: <xmqqimqkknup.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190822223316.11153-1-me@yadavpratyush.com>

Pratyush Yadav <me@yadavpratyush.com> writes:

> Subject: Re: [PATCH] git-gui: Update in-memory config when changing config options

s/git-gui: Update/git-gui: update/

Show 12 quoted lines

>  lib/option.tcl | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/lib/option.tcl b/lib/option.tcl
> index e43971b..139cf44 100644
> --- a/lib/option.tcl
> +++ b/lib/option.tcl
> @@ -344,6 +344,7 @@ proc do_save_config {w} {
>  	if {[catch {save_config} err]} {
>  		error_popup [strcat [mc "Failed to completely save options:"] "\n\n$err"]
>  	}
> +	load_config 1

This may make the symptom go away, and in that sense it would be a good change in the short term.

But I have to suspect that it may indicate a misdesign in the "edit configuration" part of the program that the newly set configuration value must load back to the program from the filesystem. That feels backwards.

NaaNaïvely, one would imagine a program wia capability to save and load run-time options to disk to behave this way, no?

 * a set of in-core variables exist to control various aspects of
   the program (e.g. font size, background colour, etc.)

 * there is a "load config" helper function that can be called to
   populate these in-core variables from an external file.

 * there is a "edit config" UI that can be used to toggle these
   in-core variables (the checkboxes and radio buttons may not
   directly be connected to the underlying variables, but to their
   temporary counterparts and there may be a "OK" button in the UI
   to commit the changes to the temporaries to the real in-core
   variables).

 * there is a "save config" helper function that can be called to do
   the reverse of "load config"; one of the places that calls this
   helper is upon the success of "edit config".

I didn't look at the lib/option.tcl to check, but I would suspect that it would require a far larger change than your single liner if we wanted to restructure the option tweaking part in such a way, and it would be much more preferrable to use the single liner patch at least for now, but in the longer term you might want to consider such a clean-up.

Thanks.