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

feat: allow registering multiple shortcuts #15542

Merged
merged 7 commits into from Nov 7, 2018
@@ -5,6 +5,7 @@
#include "atom/browser/api/atom_api_global_shortcut.h"

#include <string>
#include <vector>

#include "atom/common/native_mate_converters/accelerator_converter.h"
#include "atom/common/native_mate_converters/callback.h"
@@ -38,8 +39,20 @@ void GlobalShortcut::OnKeyPressed(const ui::Accelerator& accelerator) {
accelerator_callback_map_[accelerator].Run();
}

bool GlobalShortcut::Register(const ui::Accelerator& accelerator,
bool GlobalShortcut::Register(const std::vector<ui::Accelerator>& accelerators,
const base::Closure& callback) {
for (auto& accelerator : accelerators) {
GlobalShortcutListener* listener = GlobalShortcutListener::GetInstance();
if (!listener->RegisterAccelerator(accelerator, this))
This conversation was marked as resolved by codebytere

This comment has been minimized.

Copy link
@MarshallOfSound

MarshallOfSound Nov 2, 2018

Member

If one fails, should we unregister the successful ones?

Otherwise we will be returning false as if it failed when in fact some were successfully registered

This comment has been minimized.

Copy link
@codebytere

codebytere Nov 2, 2018

Author Member

oh yes this is a good point 🧐

return false;

accelerator_callback_map_[accelerator] = callback;
}
return true;
}

bool GlobalShortcut::RegisterAccelerator(const ui::Accelerator& accelerator,
const base::Closure& callback) {
if (!GlobalShortcutListener::GetInstance()->RegisterAccelerator(accelerator,
this)) {
return false;
@@ -78,6 +91,7 @@ void GlobalShortcut::BuildPrototype(v8::Isolate* isolate,
prototype->SetClassName(mate::StringToV8(isolate, "GlobalShortcut"));
mate::ObjectTemplateBuilder(isolate, prototype->PrototypeTemplate())
.SetMethod("register", &GlobalShortcut::Register)
.SetMethod("registerAccelerator", &GlobalShortcut::RegisterAccelerator)
.SetMethod("isRegistered", &GlobalShortcut::IsRegistered)
.SetMethod("unregister", &GlobalShortcut::Unregister)
.SetMethod("unregisterAll", &GlobalShortcut::UnregisterAll);
@@ -7,6 +7,7 @@

#include <map>
#include <string>
#include <vector>

#include "atom/browser/api/trackable_object.h"
#include "base/callback.h"
@@ -33,8 +34,10 @@ class GlobalShortcut : public extensions::GlobalShortcutListener::Observer,
private:
typedef std::map<ui::Accelerator, base::Closure> AcceleratorCallbackMap;

bool Register(const ui::Accelerator& accelerator,
bool Register(const std::vector<ui::Accelerator>& accelerators,
const base::Closure& callback);
bool RegisterAccelerator(const ui::Accelerator& accelerator,
const base::Closure& callback);
bool IsRegistered(const ui::Accelerator& accelerator);
void Unregister(const ui::Accelerator& accelerator);
void UnregisterAll();
@@ -44,6 +44,17 @@ The `globalShortcut` module has the following methods:

### `globalShortcut.register(accelerator, callback)`

* `accelerators` String[] - an array of [Accelerator](accelerator.md)s.
* `callback` Function

Registers a global shortcut of all `accelerator` items in `accelerators`. The `callback` is called when any of the registered shortcuts are pressed by the user.

When a given accelerator is already taken by other applications, this call will
silently fail. This behavior is intended by operating systems, since they don't
want applications to fight for global shortcuts.

### `globalShortcut.registerAccelerator(accelerator, callback)`
This conversation was marked as resolved by codebytere

This comment has been minimized.

Copy link
@MarshallOfSound

MarshallOfSound Nov 2, 2018

Member

Can we make this backwards compatible in a single API that takes string[] or string. We can avoid a breaking change here


* `accelerator` [Accelerator](accelerator.md)
* `callback` Function

@@ -18,19 +18,36 @@ describe('globalShortcut module', () => {
globalShortcut.unregisterAll()
})

it('can register and unregister accelerators', () => {
it('can register and unregister single accelerators', () => {
const accelerator = 'CommandOrControl+A+B+C'

expect(globalShortcut.isRegistered(accelerator)).to.be.false()
globalShortcut.register(accelerator, () => {})
globalShortcut.registerAccelerator(accelerator, () => {})
expect(globalShortcut.isRegistered(accelerator)).to.be.true()
globalShortcut.unregister(accelerator)
expect(globalShortcut.isRegistered(accelerator)).to.be.false()

expect(globalShortcut.isRegistered(accelerator)).to.be.false()
globalShortcut.register(accelerator, () => {})
globalShortcut.registerAccelerator(accelerator, () => {})
expect(globalShortcut.isRegistered(accelerator)).to.be.true()
globalShortcut.unregisterAll()
expect(globalShortcut.isRegistered(accelerator)).to.be.false()
})

it('can register andunregistermultiple accelerators', () => {
const accelerators = ['CommandOrControl+X', 'CommandOrControl+Y']

expect(globalShortcut.isRegistered(accelerators[0])).to.be.false()
expect(globalShortcut.isRegistered(accelerators[1])).to.be.false()

globalShortcut.register(accelerators, () => {})

expect(globalShortcut.isRegistered(accelerators[0])).to.be.true()
expect(globalShortcut.isRegistered(accelerators[1])).to.be.true()

globalShortcut.unregisterAll()

expect(globalShortcut.isRegistered(accelerators[0])).to.be.false()
expect(globalShortcut.isRegistered(accelerators[1])).to.be.false()
})
})
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.