Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add-on config: allow add-ons to respond to config save/reload/reset actions via a new extension points action #7598

Closed
josephsl opened this issue Sep 13, 2017 · 10 comments
Milestone

Comments

@josephsl
Copy link
Collaborator

@josephsl josephsl commented Sep 13, 2017

Hi,

For various reasons, some add-ons do not participate in Config Manager/Profiles scheme. In case of one of my add-ons, the ability to delete sections from aggregated sections is needed, while @tspivey has expressed some reservations about current approach to config profile handling.

To cater to these scenarios - especially if add-on settings are stored elsewhere, and in order to provide a possible justification for cloud-based settings, I'd like to propose adding two or three extension point actions:

  • config.saveConfig: this action will ask registered add-ons to save settings.
  • config.reloadConfig: this action will ask add-ons to reload last saved settings from disk.
  • config.resetConfig: this action will ask add-ons to reset their settings to defaults.
    Use cases:
  1. A user using Remote Support add-on has configured a lab machine to act as an always-on accessibility training workstation. After configuring server details, this user presses Control+NVDA+C to save settings, but found that the settings were not saved to disk, as they'll be saved once the configuration dialog closes or when NVDA exits.
  2. A student using an add-on that takes advantage of a future cloud settings backup service wants to load add-on settings stored on the cloud. After playing around with settings, she misconfigured an option and now would like to restore her settings from a cloud backup.

Algorithm:

  1. Similar to profile switch extension point action (see notes from @jcsteh), config.conf.save method will call config.saveConfig.notify to notify add-ons to save settings.
  2. Similarly, config.conf.reset will either call config.reloadConfig.notify or config.resetConfig.notify depending on if this is just a pure reload or a complete reset.

Impact and requirements: this proposal requires NVDA 2017.4. The impact would be greatest for add-ons that takes advantage of this.

Mitigation strategies for add-ons:

  1. Add-ons that wishes to support older NVDA releases must check for presence of extensionPoints module (import/except test).
  2. Any add-on that wishes to fully support this proposal must target NVDA 2017.4 or later to fully take advantage of this.
  3. For cloud-based settings: add-ons that stores config files on disk must be willing to deal with a possibility where settings can come from anywhere, including from a URL/JSON data, command line switches and what not.

Thanks.

josephsl added a commit to josephsl/nvda that referenced this issue Sep 15, 2017
… 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.
@feerrenrut
Copy link
Member

@feerrenrut feerrenrut commented Sep 20, 2017

@josephsl I am aware that this issue has progressed into implementation, and has active discussion on the PR. However, I don't think the original use case is very clear. Could you please expand on the problem that is being solved here? A concrete example would probably help.

Loading

@josephsl
Copy link
Collaborator Author

@josephsl josephsl commented Sep 20, 2017

Loading

@feerrenrut
Copy link
Member

@feerrenrut feerrenrut commented Oct 2, 2017

Could you take this further, why would an add-on store its config in a separate file? Can you name one that suffers from this problem?

The suggestion of a cloud storage addon is a good one. However, I would be cautious of extending a new area of code in order to provide support for an add on that does not exist. The extension points code is quite new, and hopefully it does become stable. Growing the available extension points quickly, at this stage, could prove to be problematic. If we ran into some problem with the core use cases, we may need to change the design.

Loading

@josephsl
Copy link
Collaborator Author

@josephsl josephsl commented Oct 2, 2017

Loading

@derekriemer
Copy link
Collaborator

@derekriemer derekriemer commented Oct 3, 2017

Also, tip of the day uses ini with configobj, but the data is not config based, but many of the things I store are specifically state. I do that in a separate file.

Loading

@feerrenrut
Copy link
Member

@feerrenrut feerrenrut commented Oct 3, 2017

So the other question is why do these add-ons (perhaps just explain one) store config in a separate file?
What is the current work around for deleting sections?

Loading

@derekriemer
Copy link
Collaborator

@derekriemer derekriemer commented Oct 3, 2017

The reason I'm doing it, is because the config system was not mature enough in the old days to do this, and I also don't want things like the current tip value being stored in other than base config. See my issue #7467

Loading

@josephsl
Copy link
Collaborator Author

@josephsl josephsl commented Oct 3, 2017

Loading

@derekriemer
Copy link
Collaborator

@derekriemer derekriemer commented Oct 3, 2017

@josephsl commented on Oct 2, 2017, 7:57 PM MDT:

Hi, not that I know of at the moment,

Know of what?
Cheers

Loading

@josephsl
Copy link
Collaborator Author

@josephsl josephsl commented Oct 3, 2017

Loading

josephsl added a commit to josephsl/nvda that referenced this issue Apr 12, 2018
…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.
josephsl added a commit to josephsl/nvda that referenced this issue May 8, 2018
Renamed the following actions:
* configProfileSwitched -> postConfigProfileSwitch
* configPreSave -> preConfigSave
* configPostSave -> postConfigSave
* configPostReset -> postConfigReset
Modules that uses config switch action will be modified next.
josephsl added a commit to josephsl/nvda that referenced this issue May 8, 2018
…vaccess#7598.

To keep in sync with renamed actions, various modules with handleConfigProfileSwitch has been renamed to handlePostConfigProfileSwitch.
josephsl added a commit to josephsl/nvda that referenced this issue May 8, 2018
josephsl added a commit to josephsl/nvda that referenced this issue Jul 2, 2018
…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.
josephsl added a commit to josephsl/nvda that referenced this issue Jul 3, 2018
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.
josephsl added a commit to josephsl/nvda that referenced this issue Jul 6, 2018
…). 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.
josephsl added a commit to josephsl/nvda that referenced this issue Jul 30, 2018
@nvaccessAuto nvaccessAuto added this to the 2018.3 milestone Aug 1, 2018
feerrenrut added a commit that referenced this issue Aug 1, 2018
Closes #7598

Config: add the following actions so add-ons can respond and perform appropriate actions when saving or reverting settings, respectively:
* `config.post_configSave` 
* `config.pre_configSave`
* `config.post_configReset`
* `config.pre_configReset`
* `config.post_configProfileSwitch`

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. These new actions now alleviate this.
 Example use cases:
* pre/post config save: allows add-ons to save settings to custom location(s).
* pre/post config reset: allows add-ons to reload settings from anywhere and/or reset configuration to defaults (controlled by a boolean).

Pre / post: 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 synchronization purposes. Not all modules should listen to both events at once - mostly, `pre_configSave` action is enough.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants