GPII-1885: Implemented isConfigurable and makeConfigurable functions#462
GPII-1885: Implemented isConfigurable and makeConfigurable functions#462kaspermarkus wants to merge 3 commits intoGPII:masterfrom
Conversation
|
CI job passed. |
| * If no 'isConfigured' directive is found in the solutionRecord, true is returned | ||
| */ | ||
| gpii.lifecycleManager.isConfigurable = function (that, solutionRecord, sessionState) { | ||
| if (solutionRecord.isConfigurable !== undefined) { |
There was a problem hiding this comment.
Sorry, ignore this comment, I see that it is
…d up documentation. As discussed with Antranig in the channel today, I will do a separate pull request fixing up the horrible behavior of the settings handlers, where they crash if the settings file doesn't exist
|
CI job passed. |
| ``` | ||
| "makeConfigurable": [ | ||
| { | ||
| "type": "gpii.lifecycleActions.createFile", |
There was a problem hiding this comment.
Both of these examples now refer to nonexistent functions. I think it would be better to reinstate them, with some tests, so that we create a facility which is actually usable. Don't we have an actual use case for this feature - if not, why are we implementing it? Unfortunately GPII-1885 is a TLJ so the reason why it is on our roadmap is unclear.
Could you please
i) Flesh out GPII-1885 by adding or linking to information explaining why we are implementing it
ii) Write up a JIRA for the facility of being able to invoke settingsHandlers from the makeConfigurable/isConfigurable blocks together with a sketch of what this might look like
There was a problem hiding this comment.
thanks for the feedback @amb26. I've updated GPII-1885 with more details now, and created (and linked) GPII-2057 for the settingshandler work. Would you suggest that I just fix 2057 while I'm at it - together with and implementation of gpii.lifecycleActions.pathExists, these might make for some good examples and useful functionality
There was a problem hiding this comment.
Yes, it sounds good - we may as well verify that this functionality is actually good for something, since it is no longer good for the original use case. Is it possible that the ORCA settings handler might be the only genuine use for this function in our current solutions? If we can demonstrate that we can simplify the logic there and have it simply consist of the JSON settings handler, then we have a reasonable win. If we can't actually find a single genuine use of this system, I wonder whether we shouldn't mothball it. I know it's an awful thing to suggest given you have put so much work into this pull, but I think it is desirable to keep the principle that any code we have in the codebase meets at least one real use case.
There was a problem hiding this comment.
no - I completely agree, that if there is no proper use case it should be mothballed, else it'll just pollute the code base, risk going stale, etc..
I talked with @javihernandez yesterday, and it does indeed seem that ORCA (and, I believe NVDA) needs extra actions the first time they're used. Though the use case there is that they need to be started and then stopped (which will trigger their initial setup routine), before the settings are set. Anyway, I'll let him confirm/unconfirm this
There was a problem hiding this comment.
Yes, I can confirm that behavior with both Orca and NVDA. Isn't it a good use case?
Can't we just move my implementation of "makeConfigurable" (https://github.com/GPII/linux/blob/a0acb4730edd0382dfd2010a9a713cff29268523/gpii/node_modules/orca/orcaSettingsHandler.js#L118) into a utility function in universal that can be used by any application that requires a start/stop cycle before being configured? By having this and by adding a few improvements to the JSON settings handler (mostly, being able to write the settings in the right profile), the infamous orca settings handler would disappear for the rest of the days 💃
|
@javihernandez - sounds like a great idea - +1 for removing the ORCA settings handler. Could you update this pull with that suggestion (also with respect to master) - thanks |
|
ok to test |
|
CI job passed. |
|
We should run by this again - it sounds like from the previous discussion we were very close to having this either mothballed or resolved (possibly both) |
|
mothballed until needed by a high priority solution |
5 participants
No description provided.