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

Support explicit cancelId on macOS #8733

Merged
merged 1 commit into from Feb 24, 2017

Conversation

Projects
None yet
4 participants
@kevinsawicki
Contributor

kevinsawicki commented Feb 22, 2017

Use setKeyEquivalent to support the specified cancelId on macOS when there is more than one button.

Closes #8573

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Feb 24, 2017

Contributor

🎩 hat tip to @jwheare who suggested this approach over in #8573

Contributor

kevinsawicki commented Feb 24, 2017

🎩 hat tip to @jwheare who suggested this approach over in #8573

@kevinsawicki kevinsawicki merged commit b936221 into master Feb 24, 2017

8 of 9 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
electron-linux-arm Build #5636793 succeeded in 67s
Details
electron-linux-ia32 Build #5636794 succeeded in 60s
Details
electron-linux-x64 Build #5636795 succeeded in 137s
Details
electron-mas-x64 Build #3494 succeeded in 8 min 29 sec
Details
electron-osx-x64 Build #3506 succeeded in 8 min 54 sec
Details
electron-win-ia32 Build #2513 succeeded in 7 min 59 sec
Details
electron-win-x64 Build #2487 succeeded in 8 min 1 sec
Details

@kevinsawicki kevinsawicki deleted the macos-cancel-id branch Feb 24, 2017

@jwheare

This comment has been minimized.

Show comment
Hide comment
@jwheare

jwheare Feb 24, 2017

Contributor

Brilliant, thank you! I wonder if the docs can be further clarified. Perhaps this:

  * `cancelId` Integer (optional) - The index of the button to be used to cancel the dialog, via
    the `Esc` key. By default this is assigned to the first button with "cancel" or "no" as the
    label. If no such labeled buttons exist and this option is not set, `0` will be used as the
    return value or callback response. This option is ignored on Windows.

Or have I misunderstood the desired/expected behaviour?

Contributor

jwheare commented Feb 24, 2017

Brilliant, thank you! I wonder if the docs can be further clarified. Perhaps this:

  * `cancelId` Integer (optional) - The index of the button to be used to cancel the dialog, via
    the `Esc` key. By default this is assigned to the first button with "cancel" or "no" as the
    label. If no such labeled buttons exist and this option is not set, `0` will be used as the
    return value or callback response. This option is ignored on Windows.

Or have I misunderstood the desired/expected behaviour?

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Feb 24, 2017

Contributor

Brilliant, thank you! I wonder if the docs can be further clarified.

Yeah, sounds good 👍, can you open up a pull request with your improvements?

Contributor

kevinsawicki commented Feb 24, 2017

Brilliant, thank you! I wonder if the docs can be further clarified.

Yeah, sounds good 👍, can you open up a pull request with your improvements?

@jwheare

This comment has been minimized.

Show comment
Hide comment
@jwheare

jwheare Feb 24, 2017

Contributor

Will do.

Contributor

jwheare commented Feb 24, 2017

Will do.

@SamvelRaja

This comment has been minimized.

Show comment
Hide comment
@SamvelRaja

SamvelRaja Apr 20, 2018

If I am not wrong,

@kevinsawicki According to the docs PR by @jwheare
The code change is contradicting.

According to the change,
if cancelId is not provided and the button doesn't have cancel or no then esc button will never been assigned to any button leading to esc will not work at all. Which contradicts the doc explanation of it will defaults to 0

If your desired behavior is like in the docs, we need to fix it in code to default cancel_id to 0 always when we start. If you are ok with the changes I will raise the PR for it.

SamvelRaja commented Apr 20, 2018

If I am not wrong,

@kevinsawicki According to the docs PR by @jwheare
The code change is contradicting.

According to the change,
if cancelId is not provided and the button doesn't have cancel or no then esc button will never been assigned to any button leading to esc will not work at all. Which contradicts the doc explanation of it will defaults to 0

If your desired behavior is like in the docs, we need to fix it in code to default cancel_id to 0 always when we start. If you are ok with the changes I will raise the PR for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment