Skip to content

fix: Notification with reply obscuring first action on macOS#37381

Merged
codebytere merged 1 commit intomainfrom
fix-notification-index
Mar 1, 2023
Merged

fix: Notification with reply obscuring first action on macOS#37381
codebytere merged 1 commit intomainfrom
fix-notification-index

Conversation

@codebytere
Copy link
Member

Description of Change

Closes #37376.

Fixes an issue where passing both hasReply and actions to a main process Notification on macOS resulted in the first action being obscured and unavailable. This was happening because we called setHasActionButton for the first available button, followed by setHasReplyButton. Per documentation, if hasReplyButton and hasActionButton are both YES, the reply button is shown. This meant that the first action would be lost.

We fix this by setting the reply first, and if there are actions, setting them all as additional actions.

Before Screenshot 2023-02-22 at 2 28 15 PM
After Screenshot 2023-02-22 at 2 27 59 PM

Tested with https://gist.github.com/bb92e0d0cf83439e176b3d970e533104

Checklist

Release Notes

Notes: Fixes an issue where passing both hasReply and actions to a main process Notification on macOS resulted in the first action being obscured and unavailable.

@codebytere codebytere added semver/patch backwards-compatible bug fixes target/22-x-y PR should also be added to the "22-x-y" branch. target/23-x-y PR should also be added to the "23-x-y" branch. target/24-x-y PR should also be added to the "24-x-y" branch. labels Feb 22, 2023
@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label Feb 22, 2023
@codebytere codebytere changed the title fix: Notification with reply obscuring first action on macOS fix: Notification with reply obscuring first action on macOS Feb 22, 2023
[notification_ setSoundName:base::SysUTF16ToNSString(options.sound)];
}

[notification_ setHasActionButton:false];
Copy link
Member Author

Choose a reason for hiding this comment

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

the default value is false - we don't need to call this explicitly.

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened recently label Feb 23, 2023
@codebytere codebytere force-pushed the fix-notification-index branch from 844e85a to 386feb4 Compare February 23, 2023 21:44
@codebytere codebytere requested a review from ckerr February 27, 2023 14:19
@codebytere codebytere merged commit 8fb0f43 into main Mar 1, 2023
@codebytere codebytere deleted the fix-notification-index branch March 1, 2023 08:46
@release-clerk
Copy link

release-clerk bot commented Mar 1, 2023

Release Notes Persisted

Fixes an issue where passing both hasReply and actions to a main process Notification on macOS resulted in the first action being obscured and unavailable.

@trop
Copy link
Contributor

trop bot commented Mar 1, 2023

I have automatically backported this PR to "22-x-y", please check out #37447

@trop
Copy link
Contributor

trop bot commented Mar 1, 2023

I have automatically backported this PR to "23-x-y", please check out #37448

@trop
Copy link
Contributor

trop bot commented Mar 1, 2023

I have automatically backported this PR to "24-x-y", please check out #37449

@trop trop bot added in-flight/23-x-y and removed target/23-x-y PR should also be added to the "23-x-y" branch. target/24-x-y PR should also be added to the "24-x-y" branch. labels Mar 1, 2023
@trop trop bot added merged/22-x-y PR was merged to the "22-x-y" branch. merged/23-x-y PR was merged to the "23-x-y" branch. merged/24-x-y PR was merged to the "24-x-y" branch and removed in-flight/22-x-y labels Mar 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merged/22-x-y PR was merged to the "22-x-y" branch. merged/23-x-y PR was merged to the "23-x-y" branch. merged/24-x-y PR was merged to the "24-x-y" branch semver/patch backwards-compatible bug fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Notification using hasReply removes the first action

2 participants