feat: add signal option to dialog.showMessageBox by zcbenz · Pull Request #26102 · electron/electron · GitHub
Skip to content

feat: add signal option to dialog.showMessageBox#26102

Merged
zcbenz merged 13 commits into
mainfrom
cancel-dialog
Jul 14, 2021
Merged

feat: add signal option to dialog.showMessageBox#26102
zcbenz merged 13 commits into
mainfrom
cancel-dialog

Conversation

@zcbenz

@zcbenz zcbenz commented Oct 22, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description of Change

This PR adds a signal option to close message box, once this API is approved, I'll implement same API to close file dialogs. Refs #5577.

const controller = new AbortController();
dialog.showMessageBox(win, {
  signal: controller.signal,
  message: 'message',
})

controller.abort()
Duplicate code

There are some duplicate code when checking dialog IDs, which I would like to refactor after adding the API for closing file dialogs. Optimizing them in this PR is too early.

Checklist

Release Notes

Notes: Add signal option to dialog.showMessageBox.

@electron-cation electron-cation Bot added the new-pr 🌱 PR opened recently label Oct 22, 2020
@zcbenz zcbenz requested a review from a team October 22, 2020 07:33
@electron-cation electron-cation Bot removed the new-pr 🌱 PR opened recently label Oct 23, 2020
miniak
miniak reviewed Oct 28, 2020
View reviewed changes
Comment thread shell/browser/ui/message_box_gtk.cc Outdated
@nornagon

nornagon commented Oct 28, 2020

Copy link
Copy Markdown
Contributor

@zcbenz

zcbenz commented Oct 29, 2020

Copy link
Copy Markdown
Contributor Author

I don't love this ID system. Perhaps we could use something like AbortSignal? https://w3ctag.github.io/design-principles/#aborting

I did not know it exists, it is a better design than a custom ID system. So I think the new API would look like this?

const controller = new AbortController();
dialog.showMessageBox(win, {
  signal: controller.signal,
  message: 'message',
})

controller.abort();

There are 2 downsides though:

  1. AbortController is a currently DOM only thing. Node is adding its own AbortController since v15, but it is currently marked experimental and not supported by any promise API, using it seems a bit risky.
  2. It can only be used to cancel the task, and does not provide a way to specify options. We might want to extend the API in future.

So I think maybe we can invent our own AbortController style API:

const controller = new dialog.MessageBoxAbortController()

dialog.showMessageBox(win, {
  signal: controller.signal,
  message: 'message',
})

controller.abort();
// future extensions.
controller.close({button: 1})

@nornagon

nornagon commented Oct 29, 2020

Copy link
Copy Markdown
Contributor

I'm not convinced we need the option to "pretend" that a particular button was pressed. User code can handle that plumbing by itself if needed, e.g.

const controller = new AbortController()
let abortedSignal = null

dialog.showMessageBox(win, {
  signal: controller.signal,
  message: 'message'
}).then(status => {
  if (status == null)
    status = abortedSignal
})

abortedSignal = 1
controller.abort();

as for Node adding its own, it seems like they are planning to match the DOM implementation (similar to their implementation of URL). The DOM version is standardized, so it seems pretty safe to build our own shim that matches that and switch to Node's once it's available.

@zcbenz

zcbenz commented Oct 30, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I'm going to wait for the next major upgrade of Node in Electron, adding typescript declarations for our own AbortSignal is going to have problems like node-fetch/node-fetch#741.

@zcbenz zcbenz added no-backport semver/minor backwards-compatible functionality labels Jul 5, 2021
@zcbenz zcbenz changed the title feat: add dialog.closeMessageBox API [WIP] feat: add signal option to dialog.showMessageBox to close the dialog Jul 5, 2021
@zcbenz zcbenz force-pushed the cancel-dialog branch 2 times, most recently from a088707 to c1fd58a Compare July 5, 2021 06:22
@zcbenz zcbenz changed the title [WIP] feat: add signal option to dialog.showMessageBox to close the dialog feat: add signal option to dialog.showMessageBox Jul 5, 2021
@zcbenz

zcbenz commented Jul 5, 2021

Copy link
Copy Markdown
Contributor Author

I have updated this PR to use AbortSignal as option.

@zcbenz zcbenz requested review from a team and MarshallOfSound July 5, 2021 08:28
@zcbenz zcbenz removed the wip ⚒ label Jul 5, 2021
nornagon
nornagon reviewed Jul 8, 2021
View reviewed changes
Comment thread shell/browser/ui/message_box_win.cc Outdated
Comment thread shell/browser/ui/message_box_win.cc Outdated
Comment thread spec-main/api-dialog-spec.ts
const signal = controller.signal;
const w = new BrowserWindow();
const p = dialog.showMessageBox(w, { signal, message: 'i am message' });
await delay(500);

@nornagon nornagon Jul 8, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why 500ms here? Would 0ms work equally well? If not, is there some event we can listen for?

delay(500) smells very much like flake to me.

@zcbenz zcbenz Jul 9, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

On Windows it takes some time before we can know the dialog's HWND (the dialog runs in a new thread and the dialog HWND is created asynchronously), the delay here is to make the test run the code path that closes the window after HWND is available.

Sometimes even 500ms is not enough for the HWND to be available, but this is fine as no matter which code path it runs the showMessageBox should be cancelled successfully.

@nornagon nornagon Jul 9, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hm, is there any other way we could detect this event? e.g. a blur event?

@zcbenz zcbenz Jul 12, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Showing a a child dialog does not remove the focus of parent window, so browser window's blur event will not emit. The only way I can think of is to load a web page and setup a window.onblur listener in it to send the event, which needs to careful to be not flaky.

Waiting for fixed time is usually bad because it makes test flaky, but this test is not flaky no matter what the timeout is, it only needs to confirm dialog can be closed after fully opened, and I think delay(500) does the work well.

@nornagon nornagon Jul 12, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, it won't necessarily make the test fail, but it does mean the test will test different things, undetectably, depending on how fast the underlying OS is. I'd prefer that our tests test the same thing every time if possible.

I won't block the PR on this concern though.

Comment thread spec-main/api-dialog-spec.ts
@zcbenz zcbenz merged commit 05ba635 into main Jul 14, 2021
@zcbenz zcbenz deleted the cancel-dialog branch July 14, 2021 22:59
@release-clerk

release-clerk Bot commented Jul 14, 2021

Copy link
Copy Markdown

Release Notes Persisted

Add signal option to dialog.showMessageBox.

@commit-lint commit-lint Bot mentioned this pull request Jul 14, 2021
Merged
@trop

trop Bot commented Jul 14, 2021

Copy link
Copy Markdown
Contributor

I was unable to backport this PR to "14-x-y" cleanly;
you will need to perform this backport manually.

@zcbenz

zcbenz commented Jul 15, 2021

Copy link
Copy Markdown
Contributor Author

@gtritchie gtritchie mentioned this pull request Jul 27, 2021
Closed
4 tasks
@gtritchie gtritchie mentioned this pull request Aug 10, 2021
Merged
4 tasks
BlackHole1 pushed a commit to BlackHole1/electron that referenced this pull request Aug 30, 2021
@zcbenz @BlackHole1
feat: add signal option to dialog.showMessageBox (electron#26102)
6e099fa 
* mac: add dialog.closeMessageBox API

* win: Implement dialog.closeMessageBox

* mac: Return cancelId with closeMessageBox

* gtk: Implement dialog.closeMessageBox

* win: Fix 32bit build

* win: Reduce the scope of lock

* fix: Build error after rebase

* feat: Use AbortSignal to close message box

* chore: silently handle duplicate ID

* win: Add more notes about the threads

* chore: apply reviews

* fix: base::NoDestructor should be warpped in function

* chore: fix style on windows
ckerr pushed a commit that referenced this pull request Mar 29, 2022
@zcbenz @ckerr
feat: add signal option to dialog.showMessageBox (#26102)
64024cd 
* mac: add dialog.closeMessageBox API

* win: Implement dialog.closeMessageBox

* mac: Return cancelId with closeMessageBox

* gtk: Implement dialog.closeMessageBox

* win: Fix 32bit build

* win: Reduce the scope of lock

* fix: Build error after rebase

* feat: Use AbortSignal to close message box

* chore: silently handle duplicate ID

* win: Add more notes about the threads

* chore: apply reviews

* fix: base::NoDestructor should be warpped in function

* chore: fix style on windows
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@miniak miniak miniak left review comments
@jkleinsc jkleinsc jkleinsc approved these changes
@MarshallOfSound MarshallOfSound MarshallOfSound approved these changes
+1 more reviewer
@nornagon nornagon nornagon approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api-review/approved ✅ semver/minor backwards-compatible functionality

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@zcbenz @nornagon @jkleinsc @miniak @MarshallOfSound