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 null when subscribing notification #33641

Merged
merged 2 commits into from Apr 13, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 9 additions & 3 deletions docs/api/system-preferences.md
Expand Up @@ -84,7 +84,7 @@ that contains the user information dictionary sent along with the notification.

### `systemPreferences.subscribeNotification(event, callback)` _macOS_

* `event` string
* `event` string | null
codebytere marked this conversation as resolved.
Show resolved Hide resolved
* `callback` Function
* `event` string
* `userInfo` Record<string, unknown>
Expand All @@ -109,9 +109,11 @@ example values of `event` are:
* `AppleColorPreferencesChangedNotification`
* `AppleShowScrollBarsSettingChanged`

If `event` is null, the `NSDistributedNotificationCenter` doesn’t use it as criteria for delivery to the observer. See [docs](https://developer.apple.com/documentation/foundation/nsnotificationcenter/1411723-addobserverforname?language=objc) for more information.

### `systemPreferences.subscribeLocalNotification(event, callback)` _macOS_

* `event` string
* `event` string | null
* `callback` Function
* `event` string
* `userInfo` Record<string, unknown>
Expand All @@ -122,9 +124,11 @@ Returns `number` - The ID of this subscription
Same as `subscribeNotification`, but uses `NSNotificationCenter` for local defaults.
This is necessary for events such as `NSUserDefaultsDidChangeNotification`.

If `event` is null, the `NSNotificationCenter` doesn’t use it as criteria for delivery to the observer. See [docs](https://developer.apple.com/documentation/foundation/nsnotificationcenter/1411723-addobserverforname?language=objc) for more information.

### `systemPreferences.subscribeWorkspaceNotification(event, callback)` _macOS_

* `event` string
* `event` string | null
* `callback` Function
* `event` string
* `userInfo` Record<string, unknown>
Expand All @@ -135,6 +139,8 @@ Returns `number` - The ID of this subscription
Same as `subscribeNotification`, but uses `NSWorkspace.sharedWorkspace.notificationCenter`.
This is necessary for events such as `NSWorkspaceDidActivateApplicationNotification`.

If `event` is null, the `NSWorkspaceNotificationCenter` doesn’t use it as criteria for delivery to the observer. See [docs](https://developer.apple.com/documentation/foundation/nsnotificationcenter/1411723-addobserverforname?language=objc) for more information.

### `systemPreferences.unsubscribeNotification(id)` _macOS_

* `id` Integer
Expand Down
8 changes: 4 additions & 4 deletions shell/browser/api/electron_api_system_preferences.h
Expand Up @@ -76,17 +76,17 @@ class SystemPreferences
void PostNotification(const std::string& name,
base::DictionaryValue user_info,
gin::Arguments* args);
int SubscribeNotification(const std::string& name,
int SubscribeNotification(v8::Local<v8::Value> maybe_name,
nornagon marked this conversation as resolved.
Show resolved Hide resolved
const NotificationCallback& callback);
void UnsubscribeNotification(int id);
void PostLocalNotification(const std::string& name,
base::DictionaryValue user_info);
int SubscribeLocalNotification(const std::string& name,
int SubscribeLocalNotification(v8::Local<v8::Value> maybe_name,
const NotificationCallback& callback);
void UnsubscribeLocalNotification(int request_id);
void PostWorkspaceNotification(const std::string& name,
base::DictionaryValue user_info);
int SubscribeWorkspaceNotification(const std::string& name,
int SubscribeWorkspaceNotification(v8::Local<v8::Value> maybe_name,
const NotificationCallback& callback);
void UnsubscribeWorkspaceNotification(int request_id);
v8::Local<v8::Value> GetUserDefault(v8::Isolate* isolate,
Expand Down Expand Up @@ -130,7 +130,7 @@ class SystemPreferences
~SystemPreferences() override;

#if BUILDFLAG(IS_MAC)
int DoSubscribeNotification(const std::string& name,
int DoSubscribeNotification(v8::Local<v8::Value> maybe_name,
const NotificationCallback& callback,
NotificationCenterKind kind);
void DoUnsubscribeNotification(int request_id, NotificationCenterKind kind);
Expand Down
29 changes: 21 additions & 8 deletions shell/browser/api/electron_api_system_preferences_mac.mm
Expand Up @@ -154,10 +154,11 @@ AVMediaType ParseMediaType(const std::string& media_type) {
}

int SystemPreferences::SubscribeNotification(
const std::string& name,
v8::Local<v8::Value> maybe_name,
const NotificationCallback& callback) {
return DoSubscribeNotification(
name, callback, NotificationCenterKind::kNSDistributedNotificationCenter);
maybe_name, callback,
NotificationCenterKind::kNSDistributedNotificationCenter);
}

void SystemPreferences::UnsubscribeNotification(int request_id) {
Expand All @@ -174,9 +175,9 @@ AVMediaType ParseMediaType(const std::string& media_type) {
}

int SystemPreferences::SubscribeLocalNotification(
const std::string& name,
v8::Local<v8::Value> maybe_name,
const NotificationCallback& callback) {
return DoSubscribeNotification(name, callback,
return DoSubscribeNotification(maybe_name, callback,
NotificationCenterKind::kNSNotificationCenter);
}

Expand All @@ -196,10 +197,11 @@ AVMediaType ParseMediaType(const std::string& media_type) {
}

int SystemPreferences::SubscribeWorkspaceNotification(
const std::string& name,
v8::Local<v8::Value> maybe_name,
const NotificationCallback& callback) {
return DoSubscribeNotification(
name, callback, NotificationCenterKind::kNSWorkspaceNotificationCenter);
maybe_name, callback,
NotificationCenterKind::kNSWorkspaceNotificationCenter);
}

void SystemPreferences::UnsubscribeWorkspaceNotification(int request_id) {
Expand All @@ -208,14 +210,25 @@ AVMediaType ParseMediaType(const std::string& media_type) {
}

int SystemPreferences::DoSubscribeNotification(
const std::string& name,
v8::Local<v8::Value> maybe_name,
const NotificationCallback& callback,
NotificationCenterKind kind) {
int request_id = g_next_id++;
__block NotificationCallback copied_callback = callback;

v8::Isolate* isolate = JavascriptEnvironment::GetIsolate();
std::string name_str;
if (!(maybe_name->IsNull() ||
gin::ConvertFromV8(isolate, maybe_name, &name_str))) {
isolate->ThrowException(v8::Exception::Error(
gin::StringToV8(isolate, "Must pass null or a string")));
return -1;
}

auto* name = maybe_name->IsNull() ? nil : base::SysUTF8ToNSString(name_str);

g_id_map[request_id] = [GetNotificationCenter(kind)
addObserverForName:base::SysUTF8ToNSString(name)
addObserverForName:name
object:nil
queue:nil
usingBlock:^(NSNotification* notification) {
Expand Down
33 changes: 33 additions & 0 deletions spec-main/api-system-preferences-spec.ts
Expand Up @@ -116,6 +116,39 @@ describe('systemPreferences module', () => {
});
});

ifdescribe(process.platform === 'darwin')('systemPreferences.subscribeNotification(event, callback)', () => {
it('throws an error if invalid arguments are passed', () => {
const badArgs = [123, {}, ['hi', 'bye'], new Date()];
for (const bad of badArgs) {
expect(() => {
systemPreferences.subscribeNotification(bad as any, () => {});
}).to.throw('Must pass null or a string');
}
});
});

ifdescribe(process.platform === 'darwin')('systemPreferences.subscribeLocalNotification(event, callback)', () => {
it('throws an error if invalid arguments are passed', () => {
const badArgs = [123, {}, ['hi', 'bye'], new Date()];
for (const bad of badArgs) {
expect(() => {
systemPreferences.subscribeNotification(bad as any, () => {});
}).to.throw('Must pass null or a string');
}
});
});

ifdescribe(process.platform === 'darwin')('systemPreferences.subscribeWorkspaceNotification(event, callback)', () => {
it('throws an error if invalid arguments are passed', () => {
const badArgs = [123, {}, ['hi', 'bye'], new Date()];
for (const bad of badArgs) {
expect(() => {
systemPreferences.subscribeWorkspaceNotification(bad as any, () => {});
}).to.throw('Must pass null or a string');
}
});
});

ifdescribe(process.platform === 'darwin')('systemPreferences.getSystemColor(color)', () => {
it('throws on invalid system colors', () => {
const color = 'bad-color';
Expand Down