Config: add saveConfig and resetConfig actions #7606
Conversation
… and perform appropriate actions when saving or reverting settings, respectivley. re nvaccess#7598. Some add-ons do not participate in ConfigManager/profiles at the moment. For these, there's no easy way to revert settings or Control+NVDA+C cannot be used to let add-ons save settings. To accomodate this, two new actions (extensionPoints.Action) are introduced: * config.saveConfig: a new extension point that allows add-ons to save settings to custom location(s). * config.resetConfig: allows add-ons to reload settings from anywhere and/or reset configuration to defaults (controlled by a boolean). This requires NVDA 2017.4/extensionPoints support.
|
Even though he is no longer an active member, I'd like to request comments from @jcsteh if this is something that could be useful for many. Thanks. |
|
@ctoth, @tspivey and @LeonarddeR, is this something Remote Support add-on can benefit? Also, for @nvdaes, do you find yourself using this PR in future versions of Read Feeds, Place Markers and what not? Thanks. CC @derekriemer |
|
Hi, not for the add-ons you're mentioning, which use text and pickle
files, but maybe for others as clipContentsDesigner and reportSymbols.
Cheers
2017-09-15 8:17 GMT+02:00, Joseph Lee <notifications@github.com>:
… @ctoth, @tspivey and @LeonarddeR, is this something Remote Support add-on
can benefit? Also, for @nvdaes, do you find yourself using this PR in future
versions of Read Feeds, Place Markers and what not? Thanks. CC @derekriemer
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#7606 (comment)
|
|
It seems to me that the primary use cases for this relate to being dissatisfied in some way with the core config mechanism. So, rather than considering a workaround as the first option, I think it's worth considering whether and how the core config system can be improved to address these. For example, if a way were added to remove sections and properly restrict add-on sections to the base config only (no profiles), would this be sufficient? Those are just examples.
I'm not saying the proposal at hand shouldn't be considered, just that we should consider the end goals first, rather than jumping to the means to achieve them.
|
|
On second thought, I can imagine there would be some use cases where config data isn't key/value based, in which case it could never be done with the core config framework and therefore would have to be managed by custom code. In that case, having these actions would be necessary. I think some underlying concerns about using core config need to be dealt with, but there's probably no harm in adding these either. |
|
@jcsteh commented on 15 sep. 2017 09:09 CEST:
You just stole these words out of my mouth :) Additional thought: The extension point for config profile switches is called configProfileSwitched. I'd like to propose sticking to this convention.
|
|
Hi, there are code such as someMod.handle* where the verb comes first, but I’m happy for my PR to be modified to reflect this. Thanks.
From: Leonard de Ruijter [mailto:notifications@github.com]
Sent: Friday, September 15, 2017 12:29 AM
To: nvaccess/nvda <nvda@noreply.github.com>
Cc: Joseph Lee <joseph.lee22590@gmail.com>; Author <author@noreply.github.com>
Subject: Re: [nvaccess/nvda] Config: add saveConfig and resetConfig actions (#7606)
<https://github.com/jcsteh> @jcsteh commented on 15 sep. 2017 09:09 CEST <#7606 (comment)> :
It seems to me that the primary use cases for this relate to being dissatisfied in some way with the core config mechanism. So, rather than considering a workaround as the first option, I think it's worth considering whether and how the core config system can be improved to address these.
You just stole these words out of my mouth :)
Additional thought: The extension point for config profile switches is called configProfileSwitched. I'd like to propose sticking to this convention.
* config.configSaved: notifies listeners when config is saved.
*config.configReset: notifies listeners when config is being reloaded or reset to defaults (controlled by a boolean that'll be passed around to handlers).
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <#7606 (comment)> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AHgLkFbq4MkwPilghc6LyJjb1NGjYFqlks5siidCgaJpZM4PYh7X> .
|
|
Hi, as noted in #7598, the other use case is if a handler is added at the end that’ll transport config to a cloud location for safekeeping and other possibilities. I’m trying to think of a case where key/value pairs won’t work for config storage. Thanks.
From: James Teh [mailto:notifications@github.com]
Sent: Friday, September 15, 2017 12:19 AM
To: nvaccess/nvda <nvda@noreply.github.com>
Cc: Joseph Lee <joseph.lee22590@gmail.com>; Author <author@noreply.github.com>
Subject: Re: [nvaccess/nvda] Config: add saveConfig and resetConfig actions (#7606)
On second thought, I can imagine there would be some use cases where config data isn't key/value based, in which case it could never be done with the core config framework and therefore would have to be managed by custom code. In that case, having these actions would be necessary. I think some underlying concerns about using core config need to be dealt with, but there's probably no harm in adding these either.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <#7606 (comment)> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AHgLkElWiHZsohnTgmuDw1Ed90Bk_XE5ks5siiTfgaJpZM4PYh7X> .
|
|
That's actually a very interesting use case for this. |
|
Rather than raise a new issue, I was looking for a way to save current
language symbol values, such as the word spoken and the punctuation level,
so it could be exchanged with other users the other day.
It seems there is no easy way to just do this one thing. Obviously at the
moment we can make a portable copy, load it on another machine then ask it
to make an installed copy but this of course changes every single thing.
Thus any save settings system might be expected to have radio buttons to
select exactly which bits are to be saved.
Brian
bglists@blueyonder.co.uk
Sent via blueyonder.
Please address personal email to:-
briang1@blueyonder.co.uk, putting 'Brian Gaff'
in the display name field.
|
|
This isn't a user visible change, it's just allowing addons and things to receive save notifications when config is saved. |
|
@josephsl commented on 15 Sep 2017, 09:34 CEST:
Note that I suggested configSaved, not configSave. Thus, the Past Participle of the verb. |
|
Replying to @LeonarddeR
I agree. I think this PR could help to improve the Emoticons add-on, to use profiles for activation deppending on different apps. |
|
Hi, I used “configSave” because this is essentially going to notify add-ons and other code that they should save config or deal with saved config. @derekriemer and @michaelDCurran, any thoughts? Thanks.
From: Leonard de Ruijter [mailto:notifications@github.com]
Sent: Friday, September 15, 2017 11:53 PM
To: nvaccess/nvda <nvda@noreply.github.com>
Cc: Joseph Lee <joseph.lee22590@gmail.com>; Author <author@noreply.github.com>
Subject: Re: [nvaccess/nvda] Config: add saveConfig and resetConfig actions (#7606)
<https://github.com/josephsl> @josephsl commented on 15 Sep 2017, 09:34 CEST <#7606 (comment)> :
Hi, there are code such as someMod.handle* where the verb comes first, but I’m happy for my PR to be modified to reflect this. Thanks.
Note that I suggested configSaved, not configSave. Thus, the Past Participle of the verb.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <#7606 (comment)> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AHgLkBOojGtT7yUIlFl-OzDbZaeNNydCks5si3BJgaJpZM4PYh7X> .
|
|
@josephsl commented on 16 Sep 2017, 09:27 CEST:
Strictly spoken, when looking at your code, you are notifying registered parties that the NVDA configuration has been saved. Thus, this action isn't triggered when saving NVDA's configuration failed, which is exactly what should happen :) |
|
If nothing else I think all event handlers/actionsfilters/etc should have the same name structure. This doesn't necessarily have to be handleX, as long as it is somehow possible to easily identify something as an event handler (e.g. Java has Listeners and Adapters). |
|
Hi, the comment says it should be named “configSaved”. But I intentionally used the present tense because modules might want to perform action at that very moment. That is, the action is invoked when the base config is saved, but other add-ons may come in and say, “look, we also need to save settings of our own”. Also, this brings up a question about how “configReset” should be named. For now, I’ll gladly lose in order to not drag this discussion longer than necessary, but we might want to come back to labels later. Thanks.
From: Leonard de Ruijter [mailto:notifications@github.com]
Sent: Saturday, September 16, 2017 12:59 AM
To: nvaccess/nvda <nvda@noreply.github.com>
Cc: Joseph Lee <joseph.lee22590@gmail.com>; Mention <mention@noreply.github.com>
Subject: Re: [nvaccess/nvda] Config: add saveConfig and resetConfig actions (#7606)
<https://github.com/josephsl> @josephsl commented on 16 Sep 2017, 09:27 CEST <#7606 (comment)> :
I used “configSave” because this is essentially going to notify add-ons and other code that they should save config or deal with saved config.
Strictly spoken, when looking at your code, you are notifying registered parties that the NVDA configuration has been saved. Thus, this action isn't triggered when saving NVDA's configuration failed, which is exactly what should happen :)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#7606 (comment)> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AHgLkA4OIdYnp5uMTeCeJJYS8oL5R_Cyks5si3_hgaJpZM4PYh7X> .
|
|
Let's try and do this the right way, rather than guessing Pick a set of prefixes, assign meanings to them, make every signal send 3 signals This is similar to how Apple does things, where they use should, will, and did. |
|
Hi, @LeonarddeR, Chris raises a valid point. I would imagine one or more add-ons may wish to do something after config from NVDA and add-ons were saved. As for a possible configPreSave, I’d happy to listen to use cases, but as for configPostSave, the cloud backup is a use case. Thanks.
From: Christopher Toth [mailto:notifications@github.com]
Sent: Saturday, September 16, 2017 12:23 PM
To: nvaccess/nvda <nvda@noreply.github.com>
Cc: Joseph Lee <joseph.lee22590@gmail.com>; Mention <mention@noreply.github.com>
Subject: Re: [nvaccess/nvda] Config: add saveConfig and resetConfig actions (#7606)
Let's try and do this the right way, rather than guessing
Pick a set of prefixes, assign meanings to them, make every signal send 3 signals
Something like should x, x, done_x
Problem solved.
You won't ever be able to figure out all the use cases people want to hook ahead of time, trust me. so you have to set a standard for what you're going to do, when you're doing it, and when it's done, then use that everywhere.
This is similar to how Apple does things, where they use should, will, and did.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#7606 (comment)> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AHgLkFIJS0ZtZEWwVysVR98-5KUYZvTWks5sjB_7gaJpZM4PYh7X> .
|
|
@feerrenrut: Would you be able to give your opinion on the several things discussed here regarding the naming conventions and the current state of this pr? |
|
I agree that the name of the action should be centered on what is being done internally rather than a suggestion of what subscribers should do. I like the idea of having a standard (documented) format for these actions. I'm initially drawn to |
…ess#7598. Requested by many reviewers: there are times it is helpful to let modules perform actions prior to and/or after NVDA settings are saved, most likely when add-ons need to save settings and/or a module will transport settings to a cloud location for safekeeping or cynchronization purposes. Thus split configSaved action into configPreSave (prior to) and configPostSave (after) actions. Not all modules should listen to both events at once - for add-ons, configPreSave action is enough.
source/config/__init__.py
Outdated
| configPostSave = extensionPoints.Action() | ||
| #: Notifies when configuration is reloaded from disk or factory defaults are applied. | ||
| #: Handlers are called with a boolean argument indicating whether this is a factory reset (True) or just reloading from disk (False). | ||
| configReset = extensionPoints.Action() |
There was a problem hiding this comment.
I think it would be good if this followed the pattern of pre / post. If there is no need to implement pre, please just rename this to postConfigReset.
source/config/__init__.py
Outdated
| #: Handlers can listen to "pre" or "post" action to perform tasks prior to or after NVDA's own configuration is saved. | ||
| #: Handlers are called with no arguments. | ||
| configPreSave = extensionPoints.Action() | ||
| configPostSave = extensionPoints.Action() |
There was a problem hiding this comment.
It would be much clearer that this is a general pattern (and make it easier to keep the names consistent) if these were:
preConfigSave and postConfigSave.
|
Hi, the pattern I’ve seen so far has been modulename.modname_something, having learned that from Mick. But first, I need to merge in master before working on it tonight. Thanks for the reminder.
From: Reef Turner <notifications@github.com>
Sent: Sunday, May 6, 2018 9:29 PM
To: nvaccess/nvda <nvda@noreply.github.com>
Cc: Joseph Lee <joseph.lee22590@gmail.com>; Mention <mention@noreply.github.com>
Subject: Re: [nvaccess/nvda] Config: add saveConfig and resetConfig actions (#7606)
@feerrenrut requested changes on this pull request.
_____
In source/config/__init__.py <#7606 (comment)> :
#: For example, braille switches braille displays if necessary.
#: Handlers are called with no arguments.
configProfileSwitched = extensionPoints.Action()
+#: Notifies when NVDA is saving current configuration.
+#: Handlers can listen to "pre" or "post" action to perform tasks prior to or after NVDA's own configuration is saved.
+#: Handlers are called with no arguments.
+configPreSave = extensionPoints.Action()
+configPostSave = extensionPoints.Action()
+#: Notifies when configuration is reloaded from disk or factory defaults are applied.
+#: Handlers are called with a boolean argument indicating whether this is a factory reset (True) or just reloading from disk (False).
+configReset = extensionPoints.Action()
I think it would be good if this followed the pattern of pre / post. If there is no need to implement pre, please just rename this to postConfigReset.
_____
In source/config/__init__.py <#7606 (comment)> :
#: For example, braille switches braille displays if necessary.
#: Handlers are called with no arguments.
configProfileSwitched = extensionPoints.Action()
+#: Notifies when NVDA is saving current configuration.
+#: Handlers can listen to "pre" or "post" action to perform tasks prior to or after NVDA's own configuration is saved.
+#: Handlers are called with no arguments.
+configPreSave = extensionPoints.Action()
+configPostSave = extensionPoints.Action()
It would be much clearer that this is a general pattern (and make it easier to keep the names consistent) if these were:
preConfigSave and postConfigSave.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#7606 (review)> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AHgLkIML9KYWEpCSz9ScyinmPnVsHcciks5tv82dgaJpZM4PYh7X> .
|
…s. Re nvaccess#7598. During the pull request discussion, @ctoth advised splitting save/reset actions into three parts: pre, on, and post. With a possible case of these actions being cloud settings backup/restore, it would be best to divide it like this: * preConfigSave/preConfigReset: for validation checks, preparing settings files for reset. * onConfigSave/onConfigreset: called right after NvDA own settings are saved and reset, useful as the main action by add-ons and other modules to save and/or reload settings. * postConfigSave/postConfigReset: the best use case is transporting settings to somewhere, especially to the cloud.
|
Hi, As of the latest commit, config save/reset actions are divided into three parts as suggested by @ctoth a while ago. This also brings consistency with @LeonarddeR's work on isValid/onSave/postSave methods for settings panels (NVDA 2018.2). The actions are:
Thanks. |
There was a problem hiding this comment.
I think the introduction of onConfigReset/onConfigSave only introduces confusion. Especially in the documentation, saying "during", indicates concurrency when there is none. For all intents and purposes, it is equivalent in both cases to the "post" extension point. I don't think it's a good idea to try to draw comparisons or try to be consistent with the settings dialogs. If you were trying to be consistent, then one action should be a decider, in a similar vein to isValid (eg shouldDoConfigSave / shouldDoConfigReset), and should allow an addon to decide if the config save / reset should take place at all. This would be a big decision in itself, which I don't think should be made here.
|
Hi, onConfigSave is for use by external modules to save settings, and postConfigSave is for use by modules that work on entire config package (Core and add-on settings included). Another option in the future is a follow-up action after calling config.conf.save that’ll let someone take care of the entire package and do something (say, saving the package to the cloud somewhere). As for changes requested, I’ll remove onSave* for now until we gather more cases. Thanks.
From: Reef Turner <notifications@github.com>
Sent: Tuesday, July 3, 2018 4:23 AM
To: nvaccess/nvda <nvda@noreply.github.com>
Cc: Joseph Lee <joseph.lee22590@gmail.com>; Assign <assign@noreply.github.com>
Subject: Re: [nvaccess/nvda] Config: add saveConfig and resetConfig actions (#7606)
@feerrenrut requested changes on this pull request.
I think the introduction of onConfigReset/onConfigSave only introduces confusion. Especially in the documentation, saying "during", indicates concurrency when there is none. For all intents and purposes, it is equivalent in both cases to the "post" extension point. I don't think it's a good idea to try to draw comparisons or try to be consistent with the settings dialogs. If you were trying to be consistent, then one action should be a decider, in a similar vein to isValid (eg shouldDoConfigSave / shouldDoConfigReset), and should allow an addon to decide if the config save / reset should take place at all. This would be a big decision in itself, which I don't think should be made here.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub <#7606 (review)> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AHgLkF3WVMU45k2tvZFJtoXShDWG1ShXks5uC1QjgaJpZM4PYh7X> .
|
Reviewed by Reef Turner (NV Access): just because settings dialogs have pre/on/post save handlers does not mean config should be made consistent with it. Thus remove onConfig* actions for now until future work justifies this.
…). Re nvaccess#7598. Advised by Reef Turner (NV Access) and @LeonarddeR (Babbage): use the following variable identifier format: 'pre_/post_actionname'. This means: * config.postConfigSave -> config.post_configSave. * config.preConfigSave -> config.pre_configSave. * config.postConfigReset -> config.post_configReset. * config.preConfigReset -> config.pre_configReset. * config.postConfigProfileSwitch -> config.post_configProfileSwitch.
|
Hi, Done, ready for more checkup/review. Thanks. |
There was a problem hiding this comment.
Sorry Joseph, one more thing, could you update the changes.t2t file as a part of this pull request? The release process has changed now and although it doesn't yet document this, I think it's possible that pull requests can include this change since they are going straight to master.
|
@feerrenrut: I recall that @michaelDCurran said that changes still should be applied manually to the changes.t2t, as it will cause merge conflicts if every pr is supposed to update the changes file by itself. |
|
@leonardder commented on 6 Jul 2018, 10:45 CEST:
Quote from @michaelDCurran:
|
|
Hi, I agree with @LeonarddeR. I do plan to edit the what’s new as part of docs18.3 work anyway (later in July). Thanks.
From: Leonard de Ruijter <notifications@github.com>
Sent: Friday, July 6, 2018 1:48 AM
To: nvaccess/nvda <nvda@noreply.github.com>
Cc: Joseph Lee <joseph.lee22590@gmail.com>; Assign <assign@noreply.github.com>
Subject: Re: [nvaccess/nvda] Config: add saveConfig and resetConfig actions (#7606)
<https://github.com/leonardder> @LeonarddeR commented on 6 Jul 2018, 10:45 CEST <#7606 (comment)> :
@feerrenrut <https://github.com/feerrenrut> : I recall that @michaelDCurran <https://github.com/michaelDCurran> said that changes still should be applied manually to the changes.t2t, as it will cause merge conflicts if every pr is supposed to update the changes file by itself.
Quote from @michaelDCurran <https://github.com/michaelDCurran> :
I am starting to not like the pr carrying the change to changes.t2t with a possible conflict, as by quick glancing at the pr, it simply looks like there are merge conflicts. It is not obvious that it is just because of changes.t2t.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub <#7606 (comment)> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AHgLkC5NyhJkCkAivWmdcn40TTFkRwPvks5uDyRZgaJpZM4PYh7X> .
|
|
@josephsl: Seem that consensus has changed in such a way that changes.t2t additions are now expected before merging a pr. Could you please deal with this? |
|
Hi, I already have – it’s just a matter of incorporating latest content from master, and we’re good to go. Thanks for this reminder.
From: Leonard de Ruijter <notifications@github.com>
Sent: Monday, July 30, 2018 11:19 AM
To: nvaccess/nvda <nvda@noreply.github.com>
Cc: Joseph Lee <joseph.lee22590@gmail.com>; Mention <mention@noreply.github.com>
Subject: Re: [nvaccess/nvda] Config: add saveConfig and resetConfig actions (#7606)
@josephsl <https://github.com/josephsl> : Seem that consensus has changed in such a way that changes.t2t additions are now expected before merging a pr. Could you please deal with this?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#7606 (comment)> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AHgLkNdCYfygsnhHZG9yk1qyXS5PmPUXks5uL039gaJpZM4PYh7X> .
|
…ons and renamed profile switch action. Re nvaccess#7598.
|
Hi, Thought I added what's new entries... I stand corrected. Thanks. |
Link to issue number:
closes #7598
Summary of the issue:
Add actions to notify when configuration is being saved and reloaded.
Description of how this pull request fixes the issue:
This pull request adds two new extension point actions:
Testing performed:
Added a method that plays tones or prints a message when notifications arrive from these actions.
Known issues with pull request:
Add-ons will need to be updated to register and unregister when they initialize or terminate, respectivley.
Change log entry:
None, unless required (should be listed under "changes for developers" section).