{"thread":{"id":"51713","subject":"[PATCH] git-gui: Update in-memory config when changing config options","startedAt":"2019-08-22T22:33:25Z","lastAt":"2019-08-26T19:10:12Z","messageCount":4,"participants":["Pratyush Yadav","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"380987","messageId":"20190822223316.11153-1-me@yadavpratyush.com","threadId":"51713","inReplyTo":null,"subject":"[PATCH] git-gui: Update in-memory config when changing config options","fromName":"Pratyush Yadav","fromEmail":"me@yadavpratyush.com","sentAt":"2019-08-22T22:33:16Z","receivedAt":"2019-08-22T22:33:25Z","isPatch":true,"sender":{"key":"me@yadavpratyush.com","avatar":"https://avatars.githubusercontent.com/u/8817931?v=4"},"body":"When the user updates any config variable from the options menu, the new\nconfig gets saved, but the in-memory state of the config variables is\nnot updated. This results in the old settings being used until the user\neither opens the options menu again (which triggers a call to\nload_config), or re-starts git-gui.\n\nThis change fixes that problem by re-loading the config variables when\nthe Save button is pressed in the options menu.\n\nSigned-off-by: Pratyush Yadav <me@yadavpratyush.com>\n---\n\nThe commit can be found in the topic branch 'py/reload-config' at\nhttps://github.com/prati0100/git-gui/tree/py/reload-config\n\nOnce reviewed, pull the commit 92582527b91750e47c2c3e4d1e2188998e9330ce\nor just munge the patch and apply it locally, whichever you prefer.\n\n lib/option.tcl | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/lib/option.tcl b/lib/option.tcl\nindex e43971b..139cf44 100644\n--- a/lib/option.tcl\n+++ b/lib/option.tcl\n@@ -344,6 +344,7 @@ proc do_save_config {w} {\n \tif {[catch {save_config} err]} {\n \t\terror_popup [strcat [mc \"Failed to completely save options:\"] \"\\n\\n$err\"]\n \t}\n+\tload_config 1\n \treshow_diff\n \tdestroy $w\n }\n--\n2.21.0\n\n"},{"id":"381127","messageId":"20190825215902.ccvhgf5wesjmatqj@localhost.localdomain","threadId":"51713","inReplyTo":"20190822223316.11153-1-me@yadavpratyush.com","subject":"Re: [PATCH] git-gui: Update in-memory config when changing config options","fromName":"Pratyush Yadav","fromEmail":"me@yadavpratyush.com","sentAt":"2019-08-25T21:59:02Z","receivedAt":"2019-08-25T21:59:08Z","isPatch":true,"sender":{"key":"me@yadavpratyush.com","avatar":"https://avatars.githubusercontent.com/u/8817931?v=4"},"body":"Junio,\n\nThis patch hasn't got any comments, but it looks correct to me, and fit \nfor merging IMO.\n\nI updated the commit subject from 'git-gui: Update...' to 'git-gui: \nupdate...' to match with the style of other commit messages, as you \nsuggested in the other series.\n\nYou can pull the updated commit from \nhttps://github.com/prati0100/git-gui/tree/py/reload-config commit \n3d8a8d8ff795f93554dd0ab3bbcdaec6a53c5642.\n\nI don't think it is worth the email noise to send a re-roll with just \nthe commit subject changed, but if you want, I will.\n\nOn 23/08/19 04:03AM, Pratyush Yadav wrote:\n> When the user updates any config variable from the options menu, the new\n> config gets saved, but the in-memory state of the config variables is\n> not updated. This results in the old settings being used until the user\n> either opens the options menu again (which triggers a call to\n> load_config), or re-starts git-gui.\n> \n> This change fixes that problem by re-loading the config variables when\n> the Save button is pressed in the options menu.\n> \n> Signed-off-by: Pratyush Yadav <me@yadavpratyush.com>\n> ---\n> \n> The commit can be found in the topic branch 'py/reload-config' at\n> https://github.com/prati0100/git-gui/tree/py/reload-config\n> \n> Once reviewed, pull the commit 92582527b91750e47c2c3e4d1e2188998e9330ce\n> or just munge the patch and apply it locally, whichever you prefer.\n> \n>  lib/option.tcl | 1 +\n>  1 file changed, 1 insertion(+)\n> \n> diff --git a/lib/option.tcl b/lib/option.tcl\n> index e43971b..139cf44 100644\n> --- a/lib/option.tcl\n> +++ b/lib/option.tcl\n> @@ -344,6 +344,7 @@ proc do_save_config {w} {\n>  \tif {[catch {save_config} err]} {\n>  \t\terror_popup [strcat [mc \"Failed to completely save options:\"] \"\\n\\n$err\"]\n>  \t}\n> +\tload_config 1\n>  \treshow_diff\n>  \tdestroy $w\n>  }\n> --\n> 2.21.0\n> \n\n-- \nRegards,\nPratyush Yadav\n"},{"id":"381175","messageId":"xmqqimqkknup.fsf@gitster-ct.c.googlers.com","threadId":"51713","inReplyTo":"20190822223316.11153-1-me@yadavpratyush.com","subject":"Re: [PATCH] git-gui: Update in-memory config when changing config options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-26T14:22:54Z","receivedAt":"2019-08-26T14:23:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pratyush Yadav <me@yadavpratyush.com> writes:\n\n\n> Subject: Re: [PATCH] git-gui: Update in-memory config when changing config options\n\ns/git-gui: Update/git-gui: update/\n\n>  lib/option.tcl | 1 +\n>  1 file changed, 1 insertion(+)\n>\n> diff --git a/lib/option.tcl b/lib/option.tcl\n> index e43971b..139cf44 100644\n> --- a/lib/option.tcl\n> +++ b/lib/option.tcl\n> @@ -344,6 +344,7 @@ proc do_save_config {w} {\n>  \tif {[catch {save_config} err]} {\n>  \t\terror_popup [strcat [mc \"Failed to completely save options:\"] \"\\n\\n$err\"]\n>  \t}\n> +\tload_config 1\n\nThis may make the symptom go away, and in that sense it would be a\ngood change in the short term.\n\nBut I have to suspect that it may indicate a misdesign in the \"edit\nconfiguration\" part of the program that the newly set configuration\nvalue must load back to the program from the filesystem.  That feels\nbackwards.\n\nNaaNaïvely, one would imagine a program wia capability to save and\nload run-time options to disk to behave this way, no?\n\n * a set of in-core variables exist to control various aspects of\n   the program (e.g. font size, background colour, etc.)\n\n * there is a \"load config\" helper function that can be called to\n   populate these in-core variables from an external file.\n\n * there is a \"edit config\" UI that can be used to toggle these\n   in-core variables (the checkboxes and radio buttons may not\n   directly be connected to the underlying variables, but to their\n   temporary counterparts and there may be a \"OK\" button in the UI\n   to commit the changes to the temporaries to the real in-core\n   variables).\n\n * there is a \"save config\" helper function that can be called to do\n   the reverse of \"load config\"; one of the places that calls this\n   helper is upon the success of \"edit config\".\n\nI didn't look at the lib/option.tcl to check, but I would suspect\nthat it would require a far larger change than your single liner if\nwe wanted to restructure the option tweaking part in such a way, and\nit would be much more preferrable to use the single liner patch at\nleast for now, but in the longer term you might want to consider\nsuch a clean-up.\n\nThanks.\n"},{"id":"381237","messageId":"20190826191006.dmcj6kipwxnttc3s@yadavpratyush.com","threadId":"51713","inReplyTo":"xmqqimqkknup.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] git-gui: Update in-memory config when changing config options","fromName":"Pratyush Yadav","fromEmail":"me@yadavpratyush.com","sentAt":"2019-08-26T19:10:06Z","receivedAt":"2019-08-26T19:10:12Z","isPatch":true,"sender":{"key":"me@yadavpratyush.com","avatar":"https://avatars.githubusercontent.com/u/8817931?v=4"},"body":"On 26/08/19 07:22AM, Junio C Hamano wrote:\n> Pratyush Yadav <me@yadavpratyush.com> writes:\n> \n> \n> > Subject: Re: [PATCH] git-gui: Update in-memory config when changing config options\n> \n> s/git-gui: Update/git-gui: update/\n \nI fixed this in my tree, just didn't send a re-roll with it. I assumed \nyou will pull from there so you'd get the updated subject.\n\n> >  lib/option.tcl | 1 +\n> >  1 file changed, 1 insertion(+)\n> >\n> > diff --git a/lib/option.tcl b/lib/option.tcl\n> > index e43971b..139cf44 100644\n> > --- a/lib/option.tcl\n> > +++ b/lib/option.tcl\n> > @@ -344,6 +344,7 @@ proc do_save_config {w} {\n> >  \tif {[catch {save_config} err]} {\n> >  \t\terror_popup [strcat [mc \"Failed to completely save options:\"] \"\\n\\n$err\"]\n> >  \t}\n> > +\tload_config 1\n> \n> This may make the symptom go away, and in that sense it would be a\n> good change in the short term.\n \nTrue.\n\n> But I have to suspect that it may indicate a misdesign in the \"edit\n> configuration\" part of the program that the newly set configuration\n> value must load back to the program from the filesystem.  That feels\n> backwards.\n> \n> NaaNaïvely, one would imagine a program wia capability to save and\n> load run-time options to disk to behave this way, no?\n> \n>  * a set of in-core variables exist to control various aspects of\n>    the program (e.g. font size, background colour, etc.)\n> \n>  * there is a \"load config\" helper function that can be called to\n>    populate these in-core variables from an external file.\n> \n>  * there is a \"edit config\" UI that can be used to toggle these\n>    in-core variables (the checkboxes and radio buttons may not\n>    directly be connected to the underlying variables, but to their\n>    temporary counterparts and there may be a \"OK\" button in the UI\n>    to commit the changes to the temporaries to the real in-core\n>    variables).\n> \n>  * there is a \"save config\" helper function that can be called to do\n>    the reverse of \"load config\"; one of the places that calls this\n>    helper is upon the success of \"edit config\".\n\nI took a deeper look, and saving config should _in theory_ update the \nin-memory state, and this indeed does happen for repo-specific settings \n(which I unfortunately didn't test too well. Sorry). Changing global \nsettings is what is flawed.\n\nI leave it up to you to decide if you want to pull the current patch. I \ndon't mind if you don't. I'll see if I can find some time to debug this \nand send a proper fix.\n\nThanks for your input.\n \n> I didn't look at the lib/option.tcl to check, but I would suspect\n> that it would require a far larger change than your single liner if\n> we wanted to restructure the option tweaking part in such a way, and\n> it would be much more preferrable to use the single liner patch at\n> least for now, but in the longer term you might want to consider\n> such a clean-up.\n\n-- \nRegards,\nPratyush Yadav\n"}]}