| Message ID | 20190822223316.11153-1-me@yadavpratyush.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | git-gui: Update in-memory config when changing config options | expand |
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 >
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.
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.
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 }
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(+) -- 2.21.0