X Tutup
Skip to content

feat: promisify dialog.showMessageBox() #17298

Merged
codebytere merged 2 commits intomasterfrom
promisify-showmessagebox
Mar 12, 2019
Merged

feat: promisify dialog.showMessageBox() #17298
codebytere merged 2 commits intomasterfrom
promisify-showmessagebox

Conversation

@codebytere
Copy link
Member

@codebytere codebytere commented Mar 8, 2019 •
edited
Loading

Description of Change

Promisify dialog.showMessageBox() by splitting it into a sync and async method.

Checklist

Release Notes

Notes: Split dialog.showMessageBox() into a synchronous version and a version that returns a Promise.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label Mar 8, 2019
@codebytere codebytere force-pushed the promisify-showmessagebox branch from fbbd012 to 14b4f1c Compare March 8, 2019 19:37
MarshallOfSound
MarshallOfSound reviewed Mar 8, 2019
View reviewed changes
@codebytere codebytere mentioned this pull request Mar 8, 2019
Closed
4 tasks
@codebytere codebytere force-pushed the promisify-showmessagebox branch 2 times, most recently from 8784230 to 72eaf8b Compare March 8, 2019 23:01
@codebytere codebytere marked this pull request as ready for review March 8, 2019 23:02
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened recently label Mar 9, 2019
@codebytere codebytere force-pushed the promisify-showmessagebox branch 2 times, most recently from a1c60a1 to 125bf27 Compare March 11, 2019 20:12
@codebytere codebytere force-pushed the promisify-showmessagebox branch from 125bf27 to fe834ac Compare March 11, 2019 21:08
zcbenz
zcbenz suggested changes Mar 11, 2019
View reviewed changes
atom/browser/api/atom_api_dialog.cc Outdated
cancel_id, options, title, message, detail, icon);
}

void ResolvePromiseObject(const atom::util::CopyablePromise& promise,
Copy link
Contributor

@zcbenz zcbenz Mar 11, 2019

Choose a reason for hiding this comment

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

CopyablePromise should only be used when there is no other choice, such as passing it to some Chromium APIs.

For ShowMessageBox, we should change its callback type to base::OnceCallback, and then use base::BindOnce and move-only Promise for callbacks.

@codebytere codebytere force-pushed the promisify-showmessagebox branch from 94dac9d to ec9a591 Compare March 12, 2019 05:30
@codebytere codebytere requested a review from zcbenz March 12, 2019 05:52
zcbenz
zcbenz reviewed Mar 12, 2019
View reviewed changes
@codebytere codebytere force-pushed the promisify-showmessagebox branch from ec9a591 to 44e7fa9 Compare March 12, 2019 15:08
@codebytere codebytere changed the title feat: promisify dialog.showMessageBox() feat: promisify dialog.showMessageBox() Mar 12, 2019
@codebytere codebytere merged commit 8991c00 into master Mar 12, 2019
@release-clerk
Copy link

release-clerk bot commented Mar 12, 2019

Release Notes Persisted

Split dialog.showMessageBox() into a synchronous version and a version that returns a Promise.

@codebytere codebytere deleted the promisify-showmessagebox branch March 12, 2019 18:07
@miniak miniak mentioned this pull request Mar 13, 2019
Merged
3 tasks
deepak1556 added a commit to microsoft/vscode that referenced this pull request Jun 14, 2019
@deepak1556
fix: update ipcMain event and promisifiy dialog apis
49ac885 
electron/electron#17038
electron/electron#17298
deepak1556 added a commit to microsoft/vscode that referenced this pull request Jun 14, 2019
@deepak1556
fix: promisify dialog.showMessageBox and dialog.showSaveDialog
fe1c783 
electron/electron#17054
electron/electron#17298
@audionerd
Copy link

audionerd commented Jul 30, 2019 •
edited
Loading

This should probably be listed as a "breaking change" in the release notes. Or maybe described with more context, e.g.:

dialog.showMessageBox has been renamed to dialog.showMessageBoxSync. dialog.showMessageBox now returns a Promise.

Any existing call to dialog.showMessageBox will now return a Promise instead of a value. So users will need to change to dialog.showMessageBoxSync to return to the prior behavior, or modify their code to accept a Promise instead.

@mhenr18
Copy link

mhenr18 commented Oct 22, 2019

I fully support @audionerd 's comment about updating the docs - I only caught this because the typings for electron updated to reflect the breaking API change and our typescript codebase failed to compile.

@kuychaco kuychaco mentioned this pull request Jan 29, 2020
Merged
@karaggeorge karaggeorge mentioned this pull request Apr 28, 2020
Closed
3 tasks
@dmsnell dmsnell mentioned this pull request May 20, 2020
Closed
This was referenced Oct 25, 2020
Merged
Closed
Merged
@germain-gg germain-gg mentioned this pull request Mar 25, 2021
Closed
3 tasks
jtojnar added a commit to jtojnar/markdown-image-insertion that referenced this pull request Apr 14, 2021
@jtojnar
Fix for async dialog.showOpenDialog
af2909d 
Electron 6 used by Atom 1.54 splits the methods:
- electron/electron#17298
- electron/electron#16973

Fixes: #5
@bstein bstein mentioned this pull request Aug 3, 2022
Closed
36 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MarshallOfSound MarshallOfSound MarshallOfSound left review comments

+1 more reviewer

@zcbenz zcbenz zcbenz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@codebytere @audionerd @mhenr18 @zcbenz @MarshallOfSound
X Tutup