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

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

[Pratyush Yadav]

When the user updates any config variable from the options menu, the new
config gets saved, but the in-memory state of the config variables is
not updated. This results in the old settings being used until the user
either opens the options menu again (which triggers a call to
load_config), or re-starts git-gui.

This change fixes that problem by re-loading the config variables when
the Save button is pressed in the options menu.

Signed-off-by: Pratyush Yadav <me@yadavpratyush.com>
---

The commit can be found in the topic branch 'py/reload-config' at
https://github.com/prati0100/git-gui/tree/py/reload-config

Once reviewed, pull the commit 92582527b91750e47c2c3e4d1e2188998e9330ce
or just munge the patch and apply it locally, whichever you prefer.

 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
 	reshow_diff
 	destroy $w
 }
--
2.21.0

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

[Pratyush Yadav]

Junio,

This patch hasn't got any comments, but it looks correct to me, and fit 
for merging IMO.

I updated the commit subject from 'git-gui: Update...' to 'git-gui: 
update...' to match with the style of other commit messages, as you 
suggested in the other series.

You can pull the updated commit from 
https://github.com/prati0100/git-gui/tree/py/reload-config commit 
3d8a8d8ff795f93554dd0ab3bbcdaec6a53c5642.

I don't think it is worth the email noise to send a re-roll with just 
the commit subject changed, but if you want, I will.

On 23/08/19 04:03AM, Pratyush Yadav wrote:
> When the user updates any config variable from the options menu, the new
> config gets saved, but the in-memory state of the config variables is
> not updated. This results in the old settings being used until the user
> either opens the options menu again (which triggers a call to
> load_config), or re-starts git-gui.
> 
> This change fixes that problem by re-loading the config variables when
> the Save button is pressed in the options menu.
> 
> Signed-off-by: Pratyush Yadav <me@yadavpratyush.com>
> ---
> 
> The commit can be found in the topic branch 'py/reload-config' at
> https://github.com/prati0100/git-gui/tree/py/reload-config
> 
> Once reviewed, pull the commit 92582527b91750e47c2c3e4d1e2188998e9330ce
> or just munge the patch and apply it locally, whichever you prefer.
> 
>  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
>  	reshow_diff
>  	destroy $w
>  }
> --
> 2.21.0
> 

-- 
Regards,
Pratyush Yadav

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

[Junio C Hamano]

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/

>  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.

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

[Pratyush Yadav]

On 26/08/19 07:22AM, Junio C Hamano wrote:
> 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/
 
I fixed this in my tree, just didn't send a re-roll with it. I assumed 
you will pull from there so you'd get the updated subject.

> >  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.
 
True.

> 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 took a deeper look, and saving config should _in theory_ update the 
in-memory state, and this indeed does happen for repo-specific settings 
(which I unfortunately didn't test too well. Sorry). Changing global 
settings is what is flawed.

I leave it up to you to decide if you want to pull the current patch. I 
don't mind if you don't. I'll see if I can find some time to debug this 
and send a proper fix.

Thanks for your input.
 
> 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.

-- 
Regards,
Pratyush Yadav