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

Only use default role accelerator in app menu #6385

Merged
merged 5 commits into from Jul 8, 2016

Conversation

Projects
None yet
3 participants
@kevinsawicki
Contributor

kevinsawicki commented Jul 7, 2016

This pull request is a reworking of #6229 but done differently in terms of how the option to use a default role accelerator is passed through the menu system.

The bulk of the diff here is mostly around narrowing the references from ui::MenuModel to atom::AtomMenuModel.

A new option, useDefaultAccelerator, is set to true when requesting the accelerator for a menu item that is only set for application menus on all platforms, so context, tray, and dock menus will not display the default role accelerator.

Closes #6206

@kevinsawicki kevinsawicki changed the title from [WIP] Only use default role accelerator in app menu to Only use default role accelerator in app menu Jul 7, 2016

// ui::SimpleMenuModel:
void MenuClosed() override;
using SimpleMenuModel::GetSubmenuModelAt;
AtomMenuModel* GetSubmenuModelAt(int index);

This comment has been minimized.

@kevinsawicki

kevinsawicki Jul 7, 2016

Contributor

@zcbenz I overloaded this and narrowed it to return an AtomMenuModel instead of a ui::MenuModel so calling methods didn't need to call static_cast themselves.

Let me know if you are okay with this or would prefer the calling method cast the return value down.

kevinsawicki added some commits Jul 7, 2016

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Jul 8, 2016

👍

@zcbenz zcbenz merged commit 6e81c55 into master Jul 8, 2016

8 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
electron-linux-arm Build #3615530 succeeded in 45s
Details
electron-linux-ia32 Build #3615531 succeeded in 38s
Details
electron-linux-x64 Build #3615532 succeeded in 127s
Details
electron-mas-x64 Build #1822 succeeded in 6 min 11 sec
Details
electron-osx-x64 Build #1832 succeeded in 6 min 28 sec
Details
electron-win-ia32 Build #833 succeeded in 6 min 11 sec
Details
electron-win-x64 Build #822 succeeded in 6 min 15 sec
Details

@zcbenz zcbenz deleted the only-use-role-accelerator-in-app-menu branch Jul 8, 2016

@davej

This comment has been minimized.

Contributor

davej commented Jul 8, 2016

Will the accelerator still be shown if it is explicitly defined in the menuItem?

@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented Jul 8, 2016

Will the accelerator still be shown if it is explicitly defined in the menuItem?

Yes, any explicitly set accelerators will always show, this only affects the default accelerators for role-based items added in #6190

@davej

This comment has been minimized.

Contributor

davej commented Jul 8, 2016

Yes, any explicitly set accelerators will always show, this only affects the default accelerators for role-based items added in #6190

👍

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