-
Notifications
You must be signed in to change notification settings - Fork 6
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
Enable modification of NVDA settings in automation #14
base: main
Are you sure you want to change the base?
Conversation
Prior to this patch, the project's C++ code could only be built if Microsoft Visual Studio was run with the privileges of a system administrator. Encapsulate installation steps in the MakeVoice.exe binary, and extend that binary to require administrative privileges (allowing the code to be built without them). This change also supports forthcoming work to eliminate duplicated installation logic between the project's Node.js code and C++ code.
Maintain MakeVoice.exe's focus on managing the automation voice.
Store the Node.js code which defines the at-driver command-line interface in a dedicated directory to more clearly delineate it from the other "library" (i.e. interpreted) code that this project now includes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
@@ -0,0 +1 @@ | |||
7658 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the file just the port?
return Promise.race([whenClosed, invert(connect(4382, 'at=nvda&port=', SUB_PROTOCOL))]); | ||
}); | ||
|
||
test('server rejects empty assistive technology port', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'server rejects non-number assistive technology port'
* @param {object} body - the data to send | ||
*/ | ||
module.exports = function postJSON(port, body) { | ||
return new Promise((resolve, reject) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the nesting can be reduced some to be easier to read.
module.exports = async function postJSON(port, body) {
const response = await new Promise((resolve, reject) => {
const postData = JSON.stringify(body);
const request = http.request(
{
hostname: "localhost",
method: "POST",
port,
headers: {
"Content-Type": "application/json",
"Content-Length": Buffer.byteLength(postData),
},
},
resolve
);
request.on("error", reject);
request.write(postData);
request.end();
});
const responseBody = await new Promise((resolve) => {
let responseBody = "";
response.setEncoding("utf-8");
response.on("data", (chunk) => (responseBody += chunk));
response.on("end", () => {
resolve(responseBody);
});
});
if (!response.complete) {
throw new Error("HTTP response interrupted");
}
if (response.statusCode >= 300) {
throw new Error(responseBody);
}
};
This work builds on gh-13 (which itself builds on gh-11), and as such, potential reviewers may want to hold off on offering feedback. The change set in gh-13 may evolve prior to being merged, and that may necessitate further modification to this patch. I'm submitting it as a "Draft Pull Request" to give some more visibility to the work, even before it's ready for formal review.