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

Add async menu.popup option #8702

Merged
merged 16 commits into from Feb 22, 2017

Conversation

Projects
None yet
4 participants
@kevinsawicki
Contributor

kevinsawicki commented Feb 16, 2017

Adds an async option to menu.popup that returns immediately on each platform and does not block the render loop on macOS.

The menu.popup signature is also updated (but backwards-compatible) to take an options object instead of x, y, and positioningItem arguments.

win.webContents.on('context-menu', (event, params) => {
  menu.popup(win, {
    x: params.x,
    y: params.y,
    async: true
  })

  // Close it for some reason after 2 seconds
  setTimeout(function () {
    menu.closePopup(win)
  }, 2000)
})
Sync (current behavior) Async (new behavior)
sync-menu async-menu
  • Implement async option on macOS
  • Implement async option on Linux/Windows
  • Implement menu.closePopup on macOS
  • Implement menu.closePopup on Linux/Windows
  • Update menu.md API docs
  • Add new method signature to planned-breaking-changes.md
  • Add tests

This does not change the default behavior, this new option is opt-in but that default could be revisited in the 2.0 API.

This pull request also adds a new menu.closePopup([browserWindow]) API that can be used to explicitly close the context menu.

Closes #1854

@bsclifton

This comment has been minimized.

Show comment
Hide comment
@bsclifton

bsclifton Feb 19, 2017

Member

@kevinsawicki any reason this is defaulted to async=false? Since Windows works great, I would think defaulting to true (and omitting the parameter altogether) would be the way to go

edit: I need to learn to read (derp on my part)- it's defaulted to false to keep the behavior the same, in case this would be a breaking change 😄

Member

bsclifton commented Feb 19, 2017

@kevinsawicki any reason this is defaulted to async=false? Since Windows works great, I would think defaulting to true (and omitting the parameter altogether) would be the way to go

edit: I need to learn to read (derp on my part)- it's defaulted to false to keep the behavior the same, in case this would be a breaking change 😄

bsclifton added a commit to brave/muon that referenced this pull request Feb 19, 2017

@bsclifton bsclifton referenced this pull request Feb 19, 2017

Merged

Mac async popup #161

@kevinsawicki kevinsawicki merged commit 62f4a77 into master Feb 22, 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 #5605244 succeeded in 65s
Details
electron-linux-ia32 Build #5605245 succeeded in 60s
Details
electron-linux-x64 Build #5605246 succeeded in 137s
Details
electron-mas-x64 Build #3473 succeeded in 8 min 13 sec
Details
electron-osx-x64 Build #3484 succeeded in 8 min 39 sec
Details
electron-win-ia32 Build #2490 succeeded in 8 min 0 sec
Details
electron-win-x64 Build #2468 succeeded in 8 min 4 sec
Details

@kevinsawicki kevinsawicki deleted the async-menu-popup branch Feb 22, 2017

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Feb 22, 2017

Contributor

@bsclifton just a heads up, I pushed a few more tweaks to this if you end up pulling this into Brave in brave/muon#161

Contributor

kevinsawicki commented Feb 22, 2017

@bsclifton just a heads up, I pushed a few more tweaks to this if you end up pulling this into Brave in brave/muon#161

@bsclifton

This comment has been minimized.

Show comment
Hide comment
@bsclifton

bsclifton Feb 23, 2017

Member

@kevinsawicki great, thank you! 😄

Member

bsclifton commented Feb 23, 2017

@kevinsawicki great, thank you! 😄

bsclifton added a commit to brave/muon that referenced this pull request Feb 23, 2017

bsclifton added a commit to brave/muon that referenced this pull request Feb 23, 2017

bsclifton added a commit to brave/muon that referenced this pull request Feb 28, 2017

bsclifton added a commit to brave/muon that referenced this pull request Feb 28, 2017

@kevinsawicki kevinsawicki referenced this pull request Mar 2, 2017

Merged

Use async menu popup #998

CharlieHess added a commit to electron-userland/electron-spellchecker that referenced this pull request Mar 16, 2017

bsclifton added a commit to brave/muon that referenced this pull request May 9, 2017

bsclifton added a commit to brave/muon that referenced this pull request May 9, 2017

@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero May 11, 2017

Contributor

@kevinsawicki about this becoming the default or not in 2.0, isn't this quite a breaking change for people that have code like this (pseudo):

menu.popup(async: false);
onMenuClosedCallback.fire(true); // <-- if async is true, this would fire directly, even though the menu was not dismissed

We in VS Code for example need to know in certain cases when the menu was closed (which can happen not only when a menu item is picked but also when the user clicks somewhere else or hits escape) and as far as I know there is no event that we can use to find out. This is also the reason why at the moment we cannot really adopt the async: true option.

I think what is missing compared to the async: false solution is a way to find out that the menu was closed (e.g. by passing in a callback to the popup function that is called).

Wouldn't it make sense to align the API here with how dialogs work and just make the popup call async if a callback is passed in and otherwise make it sync?

Contributor

bpasero commented May 11, 2017

@kevinsawicki about this becoming the default or not in 2.0, isn't this quite a breaking change for people that have code like this (pseudo):

menu.popup(async: false);
onMenuClosedCallback.fire(true); // <-- if async is true, this would fire directly, even though the menu was not dismissed

We in VS Code for example need to know in certain cases when the menu was closed (which can happen not only when a menu item is picked but also when the user clicks somewhere else or hits escape) and as far as I know there is no event that we can use to find out. This is also the reason why at the moment we cannot really adopt the async: true option.

I think what is missing compared to the async: false solution is a way to find out that the menu was closed (e.g. by passing in a callback to the popup function that is called).

Wouldn't it make sense to align the API here with how dialogs work and just make the popup call async if a callback is passed in and otherwise make it sync?

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki May 11, 2017

Contributor

Wouldn't it make sense to align the API here with how dialogs work and just make the popup call async if a callback is passed in and otherwise make it sync?

Yeah, could add this for sure 👍

Contributor

kevinsawicki commented May 11, 2017

Wouldn't it make sense to align the API here with how dialogs work and just make the popup call async if a callback is passed in and otherwise make it sync?

Yeah, could add this for sure 👍

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki May 11, 2017

Contributor

@bpasero want to open a new issue to track the callback option to menu.popup?

Contributor

kevinsawicki commented May 11, 2017

@bpasero want to open a new issue to track the callback option to menu.popup?

@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero
Contributor

bpasero commented May 12, 2017

bsclifton added a commit to brave/muon that referenced this pull request May 24, 2017

bsclifton added a commit to brave/muon that referenced this pull request May 24, 2017

bsclifton added a commit to brave/muon that referenced this pull request May 24, 2017

Manual application of electron patch electron/electron#8702
Additional updates (per electron/electron#8702)

Fixes brave/browser-laptop#5470

- Default `async` to true for popup menus
- fix lint errors
- menus do not destroy themselves

hferreiro added a commit to brave/muon that referenced this pull request Jun 29, 2017

Manual application of electron patch electron/electron#8702
Additional updates (per electron/electron#8702)

Fixes brave/browser-laptop#5470

- Default `async` to true for popup menus
- fix lint errors
- menus do not destroy themselves
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment