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

Mac acc perm #2

Open
wants to merge 49 commits into
base: master
Choose a base branch
from
Open

Mac acc perm #2

wants to merge 49 commits into from

Conversation

EugeneP-Zello
Copy link
Collaborator

this branch includes bugfixes
and contains link to the forked libuiohook project,
and client code which in general follows src code from libuiohook mac sample.

@@ -1,3 +1,79 @@
var binding = require('node-gyp-build')(__dirname);
module.exports = binding;
class ShortcutHelper {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

index.js Outdated
throw new TypeError('win32 impl does not track the keys');
}
this.impl.collectPressedKeyCodes(true);
return true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this always returns true (or throws), we can probably just remove this return value

@@ -1,6 +1,6 @@
{
"name": "desktop-hotkeys",
"version": "1.0.3",
"version": "1.4.3",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason why the version jumped from 1.0 to 1.4 and not 1.1?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There were several (published) releases in between

sample/sample.js Outdated Show resolved Hide resolved
const F1 = (process.platform === 'win32') ? 112 : 59;
const F6 = F1 + 5;
const F7 = F6 + 1;
try {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation looks off, code inside the try block should be indented

sample/sample.js Outdated
const F7 = 65;

hk1 = dh.registerShortcut([ CTRL, ALT, F1 ], fnPressed, fnReleased)
const CTRL = (process.platform === 'win32') ? 17 : 29;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can extract this condition to const isWindows = process.platform === 'win32'

exports.Set("macCheckAccessibilityGranted", Napi::Function::New(env, HotKeys::macCheckAccessibilityGranted));
exports.Set("macShowAccessibilitySettings", Napi::Function::New(env, HotKeys::macShowAccessibilitySettings));
exports.Set("macSubscribeAccessibilityUpdates", Napi::Function::New(env, HotKeys::macSubscribeAccessibilityUpdates));
exports.Set("macUnsubscribeAccessibilityUpdates", Napi::Function::New(env, HotKeys::macUnsubscribeAccessibilityUpdates));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation is off

Copy link
Contributor

@vocoded vocoded left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haven't finished yet but we should take care of these couple of things first

}

collectPressedKeyCodes() {
console.log('\r\n(DHK) looking for pressed keys');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this should be behind an interface of some kind

}

setupAccessibilityCallback(enable, cb) {
if (process.platform === 'darwin') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check for this.macImpl instead?

Napi::Number HotKeys::macSubscribeAccessibilityUpdates(const Napi::CallbackInfo& info)
{
Napi::Env env = info.Env();
if (info.Length() < 1 || !info[0].IsFunction()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

formatting is off in this file as well, let's please fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants