Skip to content
This repository has been archived by the owner on Sep 2, 2021. It is now read-only.

Don't add shortcuts that have a key available only when AltGr key is pressed. #446

Merged
merged 1 commit into from
May 22, 2014
Merged

Don't add shortcuts that have a key available only when AltGr key is pressed. #446

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 20 additions & 10 deletions appshell/appshell_extensions_win.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1528,18 +1528,26 @@ bool UpdateAcceleratorTable(int32 tag, ExtensionString& keyStr)
// Get the virtual key code for non-alpha-numeric characters.
int keyCode = ::VkKeyScan(ascii);
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to be careful about data types here so when we port to a 64-bit arch we don't have a lot to cleanup. That said, keyCode should be a SHORT as defined by Windows.

WORD vKey = (short)(keyCode & 0x000000FF);
Copy link
Contributor

Choose a reason for hiding this comment

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

cast to a WORD not short
why all of the extra zeros? 0xFF is sufficient to mask off the high order BYTE.

bool isAltGr = ((keyCode & 0x0000FF00) >> 8) >= 6;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same goes here. A WORD is 2 BYTES so you only need 0xFF00 here but I think you just want the low-order NIBBLE of the high-order BYTE. According to the docs you only want bits 0-4, bits 5-8 are reserved for the keyboard driver. so you need to mask off the high order NIBBLE. (keyCode & 0x0F00)

Couldn't you just do a simple bit test? isAltGr = ((keyCode & 0x60) == 0x60) ?


// Get unshifted key from keyCode so that we can determine whether the
// key is a shifted one or not.
UINT unshiftedChar = ::MapVirtualKey(vKey, 2);
Copy link
Contributor

Choose a reason for hiding this comment

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

Use the MAPVK_VK_TO_CHAR constant from the WinSDK

bool isDeadKey = ((unshiftedChar & 0x80000000) == 0x80000000);

// If key code is -1 or unshiftedChar is 0 or the key is a shifted key sharing with
// one of the number keys on the keyboard, then we don't have a functionable shortcut.
// If one of the following is found, then the shortcut is not available for the
// current keyboard layout.
//
// * keyCode is -1 -- meaning the key is not available on the current keyboard layout
// * is a dead key -- a key used to attach a specific diacritic to a base letter.
// * is altGr character -- the character is only available when Ctrl and Alt keys are also pressed together.
// * unshiftedChar is 0 -- meaning the key is not available on the current keyboard layout
// * The key is a shifted character sharing with one of the number keys on the keyboard. An example
// of this is the '/' key on German keyboard layout. It is a shifted key on number key '7'.
//
// So don't update the accelerator table. Just return false here so that the
// caller can remove the shortcut string from the menu title. An example of this
// is the '/' key on German keyboard layout. It is a shifted key on number key '7'.
if (keyCode == -1 || isDeadKey || unshiftedChar == 0 ||
// caller can remove the shortcut string from the menu title.
if (keyCode == -1 || isDeadKey || isAltGr || unshiftedChar == 0 ||
(unshiftedChar >= '0' && unshiftedChar <= '9')) {
LocalFree(lpaccelNew);
return false;
Expand Down Expand Up @@ -1674,6 +1682,13 @@ int32 AddMenuItem(CefRefPtr<CefBrowser> browser, ExtensionString parentCommand,
if (displayKeyStr.length() > 0) {
title = title + L"\t" + displayKeyStr;
}

// Add shortcut to the accelerator table first. If the shortcut cannot
// be added, then don't show it in the menu title.
if (!isSeparator && !UpdateAcceleratorTable(tag, keyStr)) {
title = title.substr(0, title.find('\t'));
}

int32 positionIdx;
int32 errCode = getNewMenuPosition(browser, parentCommand, position, relativeId, positionIdx);
bool inserted = false;
Expand Down Expand Up @@ -1724,11 +1739,6 @@ int32 AddMenuItem(CefRefPtr<CefBrowser> browser, ExtensionString parentCommand,

NativeMenuModel::getInstance(getMenuParent(browser)).setOsItem(tag, (void*)submenu);

if (!isSeparator && !UpdateAcceleratorTable(tag, keyStr)) {
title = title.substr(0, title.find('\t'));
SetMenuTitle(browser, command, title);
}

return errCode;
}

Expand Down