feat: add id, groupId, and groupTitle support for Windows notifications#50328
Conversation
fa28467 to
6644704
Compare
codebytere
left a comment
There was a problem hiding this comment.
generally lgtm with a few comments
| return {}; | ||
| } | ||
| } | ||
| #endif |
There was a problem hiding this comment.
We're parsing the options dict twice — once here via PeekNext + ConvertFromV8 to validate, and then again inside the Notification constructor. Imo that's a bit redundant and fragile since the two parse sites could diverge.
The gin::Dictionary(nullptr) + the IsEmpty() || IsUndefined() || ConvertFromV8(...)` conditional is also a bit hard to follow — the first two branches fall through into the body with a null-initialized dictionary, which feels like it shouldn't work.
Could we instead construct the Notification first and then validate the already-parsed members before returning the handle? Something like:
auto handle = gin_helper::CreateHandle(thrower.isolate(), new Notification(args));
#if BUILDFLAG(IS_WIN)
constexpr size_t kMaxTagLength = 64;
auto* notif = handle.get();
if (!notif->id_.empty() &&
base::UTF8ToWide(notif->id_).length() > kMaxTagLength) {
thrower.ThrowError(
"Notification id exceeds Windows limit of 64 UTF-16 characters");
return {};
}
if (!notif->group_id_.empty() &&
base::UTF8ToWide(notif->group_id_).length() > kMaxTagLength) {
thrower.ThrowError(
"Notification groupId exceeds Windows limit of 64 UTF-16 characters");
return {};
}
#endif
return handle;That way we validate against the actual values that'll be used at Show() time, and there's a single parse path for the options.
I recognize the downside here that we construct the object before potentially discarding it on the error path, but the constructor is side-effect-free, and CreateHandle + return {} is already the pattern used for the "app not ready" early return above, so it should be safe?
4013227 to
8cd36bc
Compare
|
Release Notes Persisted
|
|
I have automatically backported this PR to "42-x-y", please check out #50895 |
Description of Change
This PR extends the Windows notification implementation to support the
id,groupId, andgroupTitleproperties, bringing feature parity with macOS and adding Windows-specific header grouping support.Changes
New Features:
idproperty now works on Windows — maps directly to the toast notification’sTagproperty for deduplication and removal.groupIdproperty now works on Windows — it serves two purposes:idit uniquely identifies a notification in memoryidproperty for grouping in Action CentergroupTitleproperty (Windows only) — when bothgroupIdandgroupTitleare specified, a<header>element is generated in the toast XML to display a visual group header.Validation:
idandgroupIdon Windows (max 64 UTF-16 characters) with descriptive error messages.Internal Changes:
idis now used directly as the Windows toastTag.groupandtagvalues whenELECTRON_DEBUG_NOTIFICATIONSis set.Tests:
id,groupIdproperties).groupTitle.Example Usage
Checklist
npm testpassesRelease Notes
Notes: Added id, groupId, and groupTitle support for Windows notifications