From ea5b86f5c2b2a70ec0c7a365bf1cfc4d3caeb274 Mon Sep 17 00:00:00 2001 From: Eism Date: Mon, 26 Oct 2026 14:19:02 +0200 Subject: [PATCH 1/4] added unit-tests added the ability to test different layouts (english, russian, german) --- .../macos/macosshortcutsinstancemodel.h | 3 + .../macos/macosshortcutsinstancemodel.mm | 84 +++++- framework/shortcuts_v2/tests/CMakeLists.txt | 17 ++ .../macosshortcutsinstancemodel_tests.cpp | 260 ++++++++++++++++++ .../mocks/commandshortcutsregistermock.h | 58 ++++ .../tests/mocks/shortcutscontrollermock.h | 38 +++ 6 files changed, 445 insertions(+), 15 deletions(-) create mode 100644 framework/shortcuts_v2/tests/macosshortcutsinstancemodel_tests.cpp create mode 100644 framework/shortcuts_v2/tests/mocks/commandshortcutsregistermock.h create mode 100644 framework/shortcuts_v2/tests/mocks/shortcutscontrollermock.h diff --git a/framework/shortcuts_v2/qml/Muse/Shortcuts/platform/macos/macosshortcutsinstancemodel.h b/framework/shortcuts_v2/qml/Muse/Shortcuts/platform/macos/macosshortcutsinstancemodel.h index 1e63b9fc3a..d23bde7a10 100644 --- a/framework/shortcuts_v2/qml/Muse/Shortcuts/platform/macos/macosshortcutsinstancemodel.h +++ b/framework/shortcuts_v2/qml/Muse/Shortcuts/platform/macos/macosshortcutsinstancemodel.h @@ -37,6 +37,9 @@ class MacOSShortcutsInstanceModel : public ShortcutsInstanceModel public: explicit MacOSShortcutsInstanceModel(QObject* parent = nullptr); +protected: + virtual QString keyboardLayoutId() const; + private: void doLoadShortcuts() override; void doActivate(const QString& seq) override; diff --git a/framework/shortcuts_v2/qml/Muse/Shortcuts/platform/macos/macosshortcutsinstancemodel.mm b/framework/shortcuts_v2/qml/Muse/Shortcuts/platform/macos/macosshortcutsinstancemodel.mm index 8fa28ed536..31bfe693aa 100644 --- a/framework/shortcuts_v2/qml/Muse/Shortcuts/platform/macos/macosshortcutsinstancemodel.mm +++ b/framework/shortcuts_v2/qml/Muse/Shortcuts/platform/macos/macosshortcutsinstancemodel.mm @@ -86,27 +86,68 @@ { kVK_ANSI_KeypadEquals, "=" } }; -static UCKeyboardLayout* keyboardLayout() +//! NOTE Returns the current keyboard layout if inputSourceId is empty. +//! The returned input source owns the layout data and must be released by the caller +static TISInputSourceRef copyKeyboardLayoutInputSource(const QString& inputSourceId) { static TISInputSourceRef (* TISCopyCurrentKeyboardLayoutInputSource)(void); - static void*(* TISGetInputSourceProperty)(TISInputSourceRef inputSource, CFStringRef propertyKey); + static CFArrayRef (* TISCreateInputSourceList)(CFDictionaryRef properties, Boolean includeAllInstalled); CFBundleRef bundle = CFBundleGetBundleWithIdentifier(CFSTR("com.apple.Carbon")); if (bundle) { - *(void**)& TISGetInputSourceProperty = CFBundleGetFunctionPointerForName(bundle, CFSTR("TISGetInputSourceProperty")); *(void**)& TISCopyCurrentKeyboardLayoutInputSource = CFBundleGetFunctionPointerForName(bundle, CFSTR("TISCopyCurrentKeyboardLayoutInputSource")); + *(void**)& TISCreateInputSourceList = CFBundleGetFunctionPointerForName(bundle, CFSTR("TISCreateInputSourceList")); } - if (!TISCopyCurrentKeyboardLayoutInputSource || !TISGetInputSourceProperty) { + if (!TISCopyCurrentKeyboardLayoutInputSource || !TISCreateInputSourceList) { LOGE() << "Error getting functions from Carbon framework"; - return 0; + return nullptr; + } + + if (inputSourceId.isEmpty()) { + return TISCopyCurrentKeyboardLayoutInputSource(); + } + + CFStringRef id = inputSourceId.toCFString(); + const void* keys[] = { CFSTR("TISPropertyInputSourceID") }; + const void* values[] = { id }; + CFDictionaryRef properties = CFDictionaryCreate(kCFAllocatorDefault, keys, values, 1, + &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks); + + CFArrayRef sources = TISCreateInputSourceList(properties, true); + + CFRelease(properties); + CFRelease(id); + + TISInputSourceRef source = nullptr; + if (sources) { + if (CFArrayGetCount(sources) > 0) { + source = (TISInputSourceRef)CFRetain(CFArrayGetValueAtIndex(sources, 0)); + } + CFRelease(sources); + } + + return source; +} + +static UCKeyboardLayout* keyboardLayout(TISInputSourceRef inputSource) +{ + static void*(* TISGetInputSourceProperty)(TISInputSourceRef inputSource, CFStringRef propertyKey); + + CFBundleRef bundle = CFBundleGetBundleWithIdentifier(CFSTR("com.apple.Carbon")); + + if (bundle) { + *(void**)& TISGetInputSourceProperty = CFBundleGetFunctionPointerForName(bundle, CFSTR("TISGetInputSourceProperty")); } - TISInputSourceRef currentKeyboard = TISCopyCurrentKeyboardLayoutInputSource(); - CFDataRef uchr = (CFDataRef)TISGetInputSourceProperty(currentKeyboard, CFSTR("TISPropertyUnicodeKeyLayoutData")); + if (!TISGetInputSourceProperty) { + LOGE() << "Error getting functions from Carbon framework"; + return nullptr; + } + CFDataRef uchr = (CFDataRef)TISGetInputSourceProperty(inputSource, CFSTR("TISPropertyUnicodeKeyLayoutData")); return (UCKeyboardLayout*)CFDataGetBytePtr(uchr); } @@ -285,18 +326,16 @@ static QString keyCodeToString(UCKeyboardLayout* keyboard, UInt32 keyNativeCode) return ""; } -static QKeySequence translateToCurrentKeyboardLayout(const QKeySequence& sequence) +static QKeySequence translateToKeyboardLayout(UCKeyboardLayout* keyboard, const QKeySequence& sequence) { - const QKeyCombination keyCombination = sequence[0]; - - const Qt::Key qKey = keyCombination.key(); - - UCKeyboardLayout* keyboard = keyboardLayout(); if (!keyboard) { - LOGE() << "The keyboard layout is not valid"; return {}; } + const QKeyCombination keyCombination = sequence[0]; + + const Qt::Key qKey = keyCombination.key(); + bool found = false; UInt32 keyNativeCode = nativeKeycode(keyboard, qKey, found); if (!found) { @@ -371,6 +410,12 @@ static QKeySequence translateToCurrentKeyboardLayout(const QKeySequence& sequenc } }; + TISInputSourceRef inputSource = copyKeyboardLayoutInputSource(keyboardLayoutId()); + UCKeyboardLayout* keyboard = inputSource ? keyboardLayout(inputSource) : nullptr; + if (!keyboard) { + LOGE() << "The keyboard layout is not valid"; + } + ShortcutList shortcuts = commandShortcutsRegister()->shortcuts(); for (const Shortcut& sc : shortcuts) { @@ -382,7 +427,7 @@ static QKeySequence translateToCurrentKeyboardLayout(const QKeySequence& sequenc // Attempt to translate from combination of keys to character, e.g., `Shift+.` becomes `>`, in the case of a QWERTY layout QKeySequence translatedSequence - = translateToCurrentKeyboardLayout(untranslatedSequence); + = translateToKeyboardLayout(keyboard, untranslatedSequence); if (translatedSequence.isEmpty() || !(untranslatedSequence[0].key() & 0xff) || untranslatedSequence[0].key() == Qt::Key_A) { QString untranslatedSequenceStrNormalised = untranslatedSequence.toString(QKeySequence::PortableText); @@ -405,11 +450,20 @@ static QKeySequence translateToCurrentKeyboardLayout(const QKeySequence& sequenc } } + if (inputSource) { + CFRelease(inputSource); + } + assert(m_shortcuts.size() == m_macSequenceMap.size()); emit shortcutsChanged(); } +QString MacOSShortcutsInstanceModel::keyboardLayoutId() const +{ + return QString(); +} + void MacOSShortcutsInstanceModel::doActivate(const QString& seq) { // `seq` will always be in the map, because it comes from one of the QML `Shortcut` objects diff --git a/framework/shortcuts_v2/tests/CMakeLists.txt b/framework/shortcuts_v2/tests/CMakeLists.txt index c3c985afd7..6b8f5a7c14 100644 --- a/framework/shortcuts_v2/tests/CMakeLists.txt +++ b/framework/shortcuts_v2/tests/CMakeLists.txt @@ -38,4 +38,21 @@ set(MODULE_TEST_DEF SHORTCUTS_TESTDATA_DIR="${CMAKE_CURRENT_LIST_DIR}/testdata" ) +if (OS_IS_MAC AND MUSE_MODULE_SHORTCUTS_QML) + set(MODULE_TEST_SRC ${MODULE_TEST_SRC} + ${CMAKE_CURRENT_LIST_DIR}/mocks/commandshortcutsregistermock.h + ${CMAKE_CURRENT_LIST_DIR}/mocks/shortcutscontrollermock.h + ${CMAKE_CURRENT_LIST_DIR}/macosshortcutsinstancemodel_tests.cpp + ) + + set(MODULE_TEST_LINK ${MODULE_TEST_LINK} + muse::shortcuts_qml + Qt6::Qml + ) + + set(MODULE_TEST_INCLUDE ${MODULE_TEST_INCLUDE} + ${MUSE_FRAMEWORK_SRC_PATH}/shortcuts_v2/qml/Muse/Shortcuts/platform/macos + ) +endif() + include(SetupGTest) diff --git a/framework/shortcuts_v2/tests/macosshortcutsinstancemodel_tests.cpp b/framework/shortcuts_v2/tests/macosshortcutsinstancemodel_tests.cpp new file mode 100644 index 0000000000..d940c05c07 --- /dev/null +++ b/framework/shortcuts_v2/tests/macosshortcutsinstancemodel_tests.cpp @@ -0,0 +1,260 @@ +/* + * SPDX-License-Identifier: GPL-3.0-only + * MuseScore-CLA-applies + * + * MuseScore Studio + * Music Composition & Notation + * + * Copyright (C) 2026 MuseScore Limited and others + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 3 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +#include +#include + +#include +#include + +#include "modularity/ioc.h" + +#include "macosshortcutsinstancemodel.h" + +#include "mocks/commandshortcutsregistermock.h" +#include "mocks/shortcutscontrollermock.h" + +using ::testing::NiceMock; +using ::testing::Return; +using ::testing::ReturnRef; + +using namespace muse; +using namespace muse::shortcuts; + +namespace muse::shortcuts { +static const QString US_LAYOUT = "com.apple.keylayout.US"; +static const QString GERMAN_LAYOUT = "com.apple.keylayout.German"; +static const QString RUSSIAN_LAYOUT = "com.apple.keylayout.Russian"; + +class TestMacOSShortcutsInstanceModel : public MacOSShortcutsInstanceModel +{ +public: + TestMacOSShortcutsInstanceModel(const QString& keyboardLayoutId, QObject* parent) + : MacOSShortcutsInstanceModel(parent), m_keyboardLayoutId(keyboardLayoutId) {} + +protected: + QString keyboardLayoutId() const override + { + return m_keyboardLayoutId; + } + +private: + QString m_keyboardLayoutId; +}; + +class Shortcuts_MacOSShortcutsInstanceModelTests : public ::testing::Test +{ +public: + void SetUp() override + { + m_iocCtx = std::make_shared(1); + + m_shortcutsRegister = std::make_shared >(); + m_controller = std::make_shared >(); + + modularity::globalIoc()->registerExport("utests", m_shortcutsRegister); + modularity::ioc(m_iocCtx)->registerExport("utests", m_controller); + + ON_CALL(*m_shortcutsRegister, shortcuts()) + .WillByDefault(ReturnRef(m_shortcuts)); + + //! NOTE The model gets the IoC context from the QML context it belongs to + m_engine = std::make_unique(); + QmlIoCContext* qmlIoc = new QmlIoCContext(m_engine.get()); + qmlIoc->ctx = m_iocCtx; + m_engine->rootContext()->setContextProperty("ioc_context", QVariant::fromValue(qmlIoc)); + } + + void TearDown() override + { + m_model.reset(); + m_engine.reset(); + + modularity::globalIoc()->unregister("utests"); + modularity::removeIoC(m_iocCtx); + } + + void addShortcut(const std::string& command, const std::vector& sequences) + { + Shortcut shortcut; + shortcut.command = rcommand::Command(command); + shortcut.sequences = sequences; + m_shortcuts.push_back(shortcut); + } + + void loadShortcuts(const QString& keyboardLayoutId) + { + m_model = std::make_unique(keyboardLayoutId, nullptr); + QQmlEngine::setContextForObject(m_model.get(), m_engine->rootContext()); + m_model->init(); + } + + QStringList registeredSequences() const + { + return m_model->shortcuts().keys(); + } + + //! NOTE Simulates pressing the shortcut: `sequence` is what Qt sees and passes to the QML `Shortcut` object, + //! returns the sequence that is passed to the shortcuts controller + std::string activate(const QString& sequence) + { + std::string activated; + EXPECT_CALL(*m_controller, activate(::testing::_)) + .WillOnce([&activated](const std::string& seq) { + activated = seq; + }); + + m_model->activate(sequence); + + ::testing::Mock::VerifyAndClearExpectations(m_controller.get()); + + return activated; + } + + modularity::ContextPtr m_iocCtx; + std::shared_ptr > m_shortcutsRegister; + std::shared_ptr > m_controller; + + ShortcutList m_shortcuts; + + std::unique_ptr m_engine; + std::unique_ptr m_model; +}; +} + +/** + * @brief Shortcuts with a character that requires Shift must not be translated to the character of the same key without Shift + * @details `:` is `Shift+;` on the US layout. It used to be translated to `;`, so the shortcut with `:` took over `;` + */ +TEST_F(Shortcuts_MacOSShortcutsInstanceModelTests, US_ColonDoesNotOverrideSemicolon) +{ + //! [GIVEN] `:` comes before `;`, so its wrong translation to `;` would be recorded first + addShortcut("test://a", { ":" }); + addShortcut("test://b", { ";" }); + + //! [WHEN] The shortcuts are loaded for the US layout + loadShortcuts(US_LAYOUT); + + //! [THEN] Both sequences are registered + EXPECT_TRUE(registeredSequences().contains(":")); + EXPECT_TRUE(registeredSequences().contains(";")); + + //! [THEN] Each one activates its own shortcut + EXPECT_EQ(activate(";"), ";"); + EXPECT_EQ(activate(":"), ":"); +} + +TEST_F(Shortcuts_MacOSShortcutsInstanceModelTests, US_SemicolonDoesNotOverrideColon) +{ + //! [GIVEN] The same shortcuts in the reverse order + addShortcut("test://b", { ";" }); + addShortcut("test://a", { ":" }); + + //! [WHEN] The shortcuts are loaded for the US layout + loadShortcuts(US_LAYOUT); + + //! [THEN] Each sequence activates its own shortcut + EXPECT_EQ(activate(";"), ";"); + EXPECT_EQ(activate(":"), ":"); +} + +/** + * @brief The other default shortcuts with a character that requires Shift on the US layout + */ +TEST_F(Shortcuts_MacOSShortcutsInstanceModelTests, US_ShiftedCharactersDoNotOverrideUnshifted) +{ + //! [GIVEN] Pairs of shortcuts with a character and the character of the same key without Shift + const std::vector > pairs = { + { "_", "-" }, + { "(", "9" }, + { "+", "=" }, + { "Alt++", "Alt+=" }, + { "{", "[" }, + { "}", "]" }, + }; + + for (size_t i = 0; i < pairs.size(); ++i) { + addShortcut("test://shifted-" + std::to_string(i), { pairs[i].first }); + addShortcut("test://unshifted-" + std::to_string(i), { pairs[i].second }); + } + + //! [WHEN] The shortcuts are loaded for the US layout + loadShortcuts(US_LAYOUT); + + //! [THEN] Each sequence activates its own shortcut + for (const auto& [shifted, unshifted] : pairs) { + EXPECT_EQ(activate(QString::fromStdString(unshifted)), unshifted); + EXPECT_EQ(activate(QString::fromStdString(shifted)), shifted); + } +} + +TEST_F(Shortcuts_MacOSShortcutsInstanceModelTests, German_ColonDoesNotOverrideOtherShortcuts) +{ + //! [GIVEN] `:` is `Shift+.` and `;` is `Shift+,` on the German layout + addShortcut("test://a", { ":" }); + addShortcut("test://b", { ";" }); + addShortcut("test://c", { "." }); + addShortcut("test://d", { "," }); + + //! [WHEN] The shortcuts are loaded for the German layout + loadShortcuts(GERMAN_LAYOUT); + + //! [THEN] Each sequence activates its own shortcut + EXPECT_EQ(activate(":"), ":"); + EXPECT_EQ(activate(";"), ";"); + EXPECT_EQ(activate("."), "."); + EXPECT_EQ(activate(","), ","); +} + +/** + * @brief Shortcuts with Latin letters are translated to the letters of a non-Latin layout typed by the same keys + */ +TEST_F(Shortcuts_MacOSShortcutsInstanceModelTests, Russian_LatinLettersAreTranslated) +{ + //! [GIVEN] Shortcuts with Latin letters + addShortcut("test://a", { "Ctrl+A" }); + addShortcut("test://b", { "Ctrl+Z" }); + + //! [WHEN] The shortcuts are loaded for the Russian layout + loadShortcuts(RUSSIAN_LAYOUT); + + //! [THEN] The sequences are registered for the Cyrillic letters on the same keys + EXPECT_TRUE(registeredSequences().contains("Ctrl+Ф")); + EXPECT_TRUE(registeredSequences().contains("Ctrl+Я")); + + //! [THEN] Pressing the keys activates the original shortcuts + EXPECT_EQ(activate("Ctrl+Ф"), "Ctrl+A"); + EXPECT_EQ(activate("Ctrl+Я"), "Ctrl+Z"); +} + +TEST_F(Shortcuts_MacOSShortcutsInstanceModelTests, Russian_ColonDoesNotOverrideSemicolon) +{ + //! [GIVEN] `;` is on the `Ж` key, while `:` is typed with Shift on the Russian layout + addShortcut("test://a", { ":" }); + addShortcut("test://b", { ";" }); + + //! [WHEN] The shortcuts are loaded for the Russian layout + loadShortcuts(RUSSIAN_LAYOUT); + + //! [THEN] `;` is translated to the `Ж` key, and `:` is kept as is + EXPECT_EQ(activate("Ж"), ";"); + EXPECT_EQ(activate(":"), ":"); +} diff --git a/framework/shortcuts_v2/tests/mocks/commandshortcutsregistermock.h b/framework/shortcuts_v2/tests/mocks/commandshortcutsregistermock.h new file mode 100644 index 0000000000..ea4e1e6c62 --- /dev/null +++ b/framework/shortcuts_v2/tests/mocks/commandshortcutsregistermock.h @@ -0,0 +1,58 @@ +/* + * SPDX-License-Identifier: GPL-3.0-only + * MuseScore-CLA-applies + * + * MuseScore Studio + * Music Composition & Notation + * + * Copyright (C) 2026 MuseScore Limited and others + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 3 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +#pragma once + +#include + +#include "icommandshortcutsregister.h" + +namespace muse::shortcuts { +class CommandShortcutsRegisterMock : public ICommandShortcutsRegister +{ +public: + MOCK_METHOD(const ShortcutList&, shortcuts, (), (const, override)); + MOCK_METHOD(Ret, updateShortcuts, (const ShortcutList&), (override)); + MOCK_METHOD(void, resetShortcuts, (), (override)); + MOCK_METHOD(async::Notification, shortcutsChanged, (), (const, override)); + + MOCK_METHOD(Ret, setAdditionalShortcuts, (const std::string&, const ShortcutList&), (override)); + + MOCK_METHOD(ShortcutList, shortcutsForSequence, (const std::string&), (const, override)); + MOCK_METHOD(const Shortcut&, shortcut, (const rcommand::Command&), (const, override)); + MOCK_METHOD(const Shortcut&, defaultShortcut, (const rcommand::Command&), (const, override)); + + MOCK_METHOD(Ret, importFromFile, (const io::path_t&), (override)); + MOCK_METHOD(Ret, exportToFile, (const io::path_t&), (const, override)); + + MOCK_METHOD(std::vector, availablePresets, (), (const, override)); + + MOCK_METHOD(std::string, currentPresetName, (), (const, override)); + MOCK_METHOD(void, setCurrentPresetName, (const std::string&), (override)); + MOCK_METHOD(async::Channel, currentPresetNameChanged, (), (const, override)); + + MOCK_METHOD(bool, isPresetEdited, (const std::string&), (const, override)); + MOCK_METHOD(bool, canDeletePreset, (const std::string&), (const, override)); + MOCK_METHOD(void, deletePreset, (const std::string&), (override)); + + MOCK_METHOD(void, reload, (bool), (override)); +}; +} diff --git a/framework/shortcuts_v2/tests/mocks/shortcutscontrollermock.h b/framework/shortcuts_v2/tests/mocks/shortcutscontrollermock.h new file mode 100644 index 0000000000..b58bf3029a --- /dev/null +++ b/framework/shortcuts_v2/tests/mocks/shortcutscontrollermock.h @@ -0,0 +1,38 @@ +/* + * SPDX-License-Identifier: GPL-3.0-only + * MuseScore-CLA-applies + * + * MuseScore Studio + * Music Composition & Notation + * + * Copyright (C) 2026 MuseScore Limited and others + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 3 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +#pragma once + +#include + +#include "ishortcutscontroller.h" + +namespace muse::shortcuts { +class ShortcutsControllerMock : public IShortcutsController +{ +public: + MOCK_METHOD(bool, active, (), (override)); + MOCK_METHOD(void, setActive, (bool), (override)); + MOCK_METHOD(async::Notification, activeChanged, (), (const, override)); + + MOCK_METHOD(void, activate, (const std::string&), (override)); +}; +} From 7864091b8994ba2033c4227d1bde7a04820eae7e Mon Sep 17 00:00:00 2001 From: Elnur Ismailzada Date: Mon, 26 Oct 2026 14:45:11 +0200 Subject: [PATCH 2/4] fixed shortcuts with shifted characters on macOS Problem: nativeKeycode() searched for a character in all tables of the keyboard layout, including the Shift and Option ones, but returned only the key code. keyCodeToString() then translated the key without modifiers, so e.g. `:` (Shift+; on US) became `;`, `(` became `9`, `_` became `-`. The translated sequence clashed with the shortcut for that character, and depending on the order of shortcuts one of them took over the other. Changes: - nativeKeycode() searches only the tables for the unmodified state and for Command (the latter holds the Latin characters of non-Latin layouts). Characters that require Shift or Option are not translated and are registered as is. - Letters are matched regardless of case, since Qt::Key_A is `A`. - Only the keyboard type that UCKeyTranslate uses (LMGetKbdType) is searched. - Added keyboardType() helper, LMGetKbdType is loaded dynamically as Carbon is not linked. --- .../macos/macosshortcutsinstancemodel.mm | 121 +++++++++++++----- 1 file changed, 87 insertions(+), 34 deletions(-) diff --git a/framework/shortcuts_v2/qml/Muse/Shortcuts/platform/macos/macosshortcutsinstancemodel.mm b/framework/shortcuts_v2/qml/Muse/Shortcuts/platform/macos/macosshortcutsinstancemodel.mm index 31bfe693aa..3126aba5a4 100644 --- a/framework/shortcuts_v2/qml/Muse/Shortcuts/platform/macos/macosshortcutsinstancemodel.mm +++ b/framework/shortcuts_v2/qml/Muse/Shortcuts/platform/macos/macosshortcutsinstancemodel.mm @@ -148,9 +148,31 @@ static TISInputSourceRef copyKeyboardLayoutInputSource(const QString& inputSourc } CFDataRef uchr = (CFDataRef)TISGetInputSourceProperty(inputSource, CFSTR("TISPropertyUnicodeKeyLayoutData")); + if (!uchr) { + return nullptr; + } + return (UCKeyboardLayout*)CFDataGetBytePtr(uchr); } +static UInt32 keyboardType() +{ + static UInt8 (* LMGetKbdType)(void); + + CFBundleRef bundle = CFBundleGetBundleWithIdentifier(CFSTR("com.apple.Carbon")); + + if (bundle) { + *(void**)& LMGetKbdType = CFBundleGetFunctionPointerForName(bundle, CFSTR("LMGetKbdType")); + } + + if (!LMGetKbdType) { + LOGE() << "Error getting functions from Carbon framework"; + return 0; + } + + return LMGetKbdType(); +} + static UInt32 nativeKeycode(UCKeyboardLayout* keyboard, Qt::Key keyCode, bool& found) { found = true; @@ -241,45 +263,84 @@ static UInt32 nativeKeycode(UCKeyboardLayout* keyboard, Qt::Key keyCode, bool& f } if (keyCode < 0 || keyCode > 0xFFFF) { - LOGW() << "Unhandled key code: " << keyCode; + LOGD() << "Unhandled key code: " << keyCode; found = false; return 0; } - UTF16Char keyCodeChar = keyCode; - UCKeyboardTypeHeader* table = keyboard->keyboardTypeList; + const UTF16Char keyCodeChar = keyCode; + // Letters are matched regardless of case: Qt::Key_A is `A`, while the unmodified key produces `a` + const UTF16Char keyCodeCharLower = QChar(keyCodeChar).toLower().unicode(); + + auto matches = [keyCodeChar, keyCodeCharLower](UTF16Char ch) { + return ch == keyCodeChar || ch == keyCodeCharLower; + }; uint8_t* data = (uint8_t*)keyboard; + + // Use the same keyboard type that UCKeyTranslate (see keyCodeToString) will use + const UInt32 kbdType = keyboardType(); + const UCKeyboardTypeHeader* header = &keyboard->keyboardTypeList[0]; for (UInt32 i = 0; i < keyboard->keyboardTypeCount; i++) { - UCKeyStateRecordsIndex* stateRec = 0; - if (table[i].keyStateRecordsIndexOffset != 0) { - stateRec = reinterpret_cast(data + table[i].keyStateRecordsIndexOffset); - if (stateRec->keyStateRecordsIndexFormat != kUCKeyStateRecordsIndexFormat) { - stateRec = 0; + if (keyboard->keyboardTypeList[i].keyboardTypeFirst <= kbdType && kbdType <= keyboard->keyboardTypeList[i].keyboardTypeLast) { + header = &keyboard->keyboardTypeList[i]; + break; + } + } + + UCKeyStateRecordsIndex* stateRec = 0; + if (header->keyStateRecordsIndexOffset != 0) { + stateRec = reinterpret_cast(data + header->keyStateRecordsIndexOffset); + if (stateRec->keyStateRecordsIndexFormat != kUCKeyStateRecordsIndexFormat) { + stateRec = 0; + } + } + + UCKeyToCharTableIndex* charTable = reinterpret_cast(data + header->keyToCharTableIndexOffset); + if (charTable->keyToCharTableIndexFormat != kUCKeyToCharTableIndexFormat) { + found = false; + return 0; + } + + // Only search the tables for the unmodified state and for Command (the latter holds e.g. the Latin characters + // of non-Latin layouts). keyCodeToString translates the key without modifiers, so a character that requires + // e.g. Shift (`:` is `Shift+;` on QWERTY) would be translated to a different character (`;`) and clash with + // the shortcuts for that character. The tables for other modifiers can't be used either: e.g. the Control table + // may have a QWERTY-like layout, and Command+CapsLock may select the Shift table + std::vector allowedTables(charTable->keyToCharTableCount, false); + const UCKeyModifiersToTableNum* modifiersToTable + = reinterpret_cast(data + header->keyModifiersToTableNumOffset); + if (modifiersToTable->keyModifiersToTableNumFormat == kUCKeyModifiersToTableNumFormat) { + for (UInt32 modifiers : { UInt32(0), UInt32((cmdKey >> 8) & 0xFF) }) { + const UInt32 tableNum = modifiers < modifiersToTable->modifiersCount + ? modifiersToTable->tableNum[modifiers] : modifiersToTable->defaultTableNum; + if (tableNum < allowedTables.size()) { + allowedTables[tableNum] = true; } } + } else if (!allowedTables.empty()) { + allowedTables[0] = true; + } - UCKeyToCharTableIndex* charTable = reinterpret_cast(data + table[i].keyToCharTableIndexOffset); - if (charTable->keyToCharTableIndexFormat != kUCKeyToCharTableIndexFormat) { + for (UInt32 j = 0; j < charTable->keyToCharTableCount; j++) { + if (!allowedTables[j]) { continue; } - for (UInt32 j = 0; j < charTable->keyToCharTableCount; j++) { - UCKeyOutput* keyToChar = reinterpret_cast(data + charTable->keyToCharTableOffsets[j]); - for (UInt32 k = 0; k < charTable->keyToCharTableSize; k++) { - if (keyToChar[k] & kUCKeyOutputTestForIndexMask) { - long idx = keyToChar[k] & kUCKeyOutputGetIndexMask; - if (stateRec && idx < stateRec->keyStateRecordCount) { - UCKeyStateRecord* rec = reinterpret_cast(data + stateRec->keyStateRecordOffsets[idx]); - if (rec->stateZeroCharData == keyCodeChar) { - return k; - } - } - } else if (!(keyToChar[k] & kUCKeyOutputSequenceIndexMask) && keyToChar[k] < 0xFFFE) { - if (keyToChar[k] == keyCodeChar) { + UCKeyOutput* keyToChar = reinterpret_cast(data + charTable->keyToCharTableOffsets[j]); + for (UInt32 k = 0; k < charTable->keyToCharTableSize; k++) { + if (keyToChar[k] & kUCKeyOutputTestForIndexMask) { + long idx = keyToChar[k] & kUCKeyOutputGetIndexMask; + if (stateRec && idx < stateRec->keyStateRecordCount) { + UCKeyStateRecord* rec = reinterpret_cast(data + stateRec->keyStateRecordOffsets[idx]); + if (matches(rec->stateZeroCharData)) { return k; } } + } else if (!(keyToChar[k] & kUCKeyOutputSequenceIndexMask) && keyToChar[k] < 0xFFFE) { + if (matches(keyToChar[k])) { + return k; + } } } } @@ -294,14 +355,6 @@ static QString keyCodeToString(UCKeyboardLayout* keyboard, UInt32 keyNativeCode) return specialKeysMap.at(keyNativeCode); } - static UInt8 (* LMGetKbdType)(void); - - CFBundleRef bundle = CFBundleGetBundleWithIdentifier(CFSTR("com.apple.Carbon")); - - if (bundle) { - *(void**)& LMGetKbdType = CFBundleGetFunctionPointerForName(bundle, CFSTR("LMGetKbdType")); - } - UInt32 deadKeyState = 0; UniCharCount actualLength = 0; const int maxLength = 4; @@ -312,7 +365,7 @@ static QString keyCodeToString(UCKeyboardLayout* keyboard, UInt32 keyNativeCode) UInt16(keyNativeCode), UInt16(kUCKeyActionDisplay), UInt32((0 >> 8) & 0xFF), - UInt32(LMGetKbdType()), + keyboardType(), OptionBits(kUCKeyTranslateNoDeadKeysBit), &deadKeyState, maxLength, @@ -339,7 +392,7 @@ static QKeySequence translateToKeyboardLayout(UCKeyboardLayout* keyboard, const bool found = false; UInt32 keyNativeCode = nativeKeycode(keyboard, qKey, found); if (!found) { - LOGW() << "Key " << qKey << " not found in the keyboard layout"; + LOGD() << "Key " << qKey << " not found in the keyboard layout"; return {}; } @@ -438,7 +491,7 @@ static QKeySequence translateToKeyboardLayout(UCKeyboardLayout* keyboard, const } if (translatedSequence.isEmpty()) { - LOGW() << "Failed to translate sequence " << untranslatedSequenceStr; + LOGD() << "Failed to translate sequence " << untranslatedSequenceStr; } else { QString translatedSequenceStrNormalised = translatedSequence.toString(QKeySequence::PortableText); From 48029ef79797bab5ecaeb78cae3cae6ac59df48a Mon Sep 17 00:00:00 2001 From: Elnur Ismailzada Date: Thu, 29 Oct 2026 14:27:59 +0200 Subject: [PATCH 3/4] fixed Option shortcuts on layouts where Option types ASCII characters Problem: On e.g. the Russian layout Option+1 types `!`. Characters that require modifiers were registered as is, so Qt matched Option+1 against both `Alt+1` and `!`, treated the key press as ambiguous and activated neither shortcut. Changes: - A character that requires Shift/Option is translated to these modifiers and the key (`!` -> `Shift+1`) instead of being registered as is. - Such translations are recorded last, so they don't take over the shortcuts bound directly to that combination of keys. --- .../macos/macosshortcutsinstancemodel.mm | 95 ++++++++++----- .../macosshortcutsinstancemodel_tests.cpp | 115 +++++++++++++----- 2 files changed, 153 insertions(+), 57 deletions(-) diff --git a/framework/shortcuts_v2/qml/Muse/Shortcuts/platform/macos/macosshortcutsinstancemodel.mm b/framework/shortcuts_v2/qml/Muse/Shortcuts/platform/macos/macosshortcutsinstancemodel.mm index 3126aba5a4..78e597ab57 100644 --- a/framework/shortcuts_v2/qml/Muse/Shortcuts/platform/macos/macosshortcutsinstancemodel.mm +++ b/framework/shortcuts_v2/qml/Muse/Shortcuts/platform/macos/macosshortcutsinstancemodel.mm @@ -173,9 +173,11 @@ static UInt32 keyboardType() return LMGetKbdType(); } -static UInt32 nativeKeycode(UCKeyboardLayout* keyboard, Qt::Key keyCode, bool& found) +//! NOTE requiredModifiers receives the modifiers needed to type the character with the returned key +static UInt32 nativeKeycode(UCKeyboardLayout* keyboard, Qt::Key keyCode, Qt::KeyboardModifiers& requiredModifiers, bool& found) { found = true; + requiredModifiers = Qt::NoModifier; switch (keyCode) { case Qt::Key_Return: @@ -302,45 +304,59 @@ static UInt32 nativeKeycode(UCKeyboardLayout* keyboard, Qt::Key keyCode, bool& f return 0; } - // Only search the tables for the unmodified state and for Command (the latter holds e.g. the Latin characters - // of non-Latin layouts). keyCodeToString translates the key without modifiers, so a character that requires - // e.g. Shift (`:` is `Shift+;` on QWERTY) would be translated to a different character (`;`) and clash with - // the shortcuts for that character. The tables for other modifiers can't be used either: e.g. the Control table - // may have a QWERTY-like layout, and Command+CapsLock may select the Shift table - std::vector allowedTables(charTable->keyToCharTableCount, false); + // The tables to search, in order of priority, and the modifiers needed to type a character from each of them. + // keyCodeToString translates the key without modifiers, so a character that requires e.g. Shift (`:` is `Shift+;` + // on QWERTY) must be translated together with that modifier, otherwise it clashes with the shortcuts for the + // character of the same key without the modifier (`;`). The Command table needs no modifiers, as it holds e.g. + // the Latin characters of non-Latin layouts, that Qt also takes into account. The tables for other modifiers + // can't be used: e.g. the Control table may have a QWERTY-like layout + const std::vector > tablesModifiers = { + { 0, Qt::NoModifier }, + { cmdKey, Qt::NoModifier }, + { shiftKey, Qt::ShiftModifier }, + { optionKey, Qt::AltModifier }, + { optionKey | shiftKey, Qt::AltModifier | Qt::ShiftModifier }, + }; + const UCKeyModifiersToTableNum* modifiersToTable = reinterpret_cast(data + header->keyModifiersToTableNumOffset); - if (modifiersToTable->keyModifiersToTableNumFormat == kUCKeyModifiersToTableNumFormat) { - for (UInt32 modifiers : { UInt32(0), UInt32((cmdKey >> 8) & 0xFF) }) { - const UInt32 tableNum = modifiers < modifiersToTable->modifiersCount - ? modifiersToTable->tableNum[modifiers] : modifiersToTable->defaultTableNum; - if (tableNum < allowedTables.size()) { - allowedTables[tableNum] = true; - } + const bool hasModifiersToTable = modifiersToTable->keyModifiersToTableNumFormat == kUCKeyModifiersToTableNumFormat; + + std::vector searchedTables(charTable->keyToCharTableCount, false); + + for (const auto& [carbonModifiers, qtModifiers] : tablesModifiers) { + const UInt32 modifiers = (carbonModifiers >> 8) & 0xFF; + + UInt32 tableNum = 0; + if (hasModifiersToTable) { + tableNum = modifiers < modifiersToTable->modifiersCount + ? modifiersToTable->tableNum[modifiers] : modifiersToTable->defaultTableNum; + } else if (modifiers != 0) { + break; } - } else if (!allowedTables.empty()) { - allowedTables[0] = true; - } - for (UInt32 j = 0; j < charTable->keyToCharTableCount; j++) { - if (!allowedTables[j]) { + if (tableNum >= searchedTables.size() || searchedTables[tableNum]) { continue; } - UCKeyOutput* keyToChar = reinterpret_cast(data + charTable->keyToCharTableOffsets[j]); + searchedTables[tableNum] = true; + + UCKeyOutput* keyToChar = reinterpret_cast(data + charTable->keyToCharTableOffsets[tableNum]); for (UInt32 k = 0; k < charTable->keyToCharTableSize; k++) { + bool keyMatches = false; if (keyToChar[k] & kUCKeyOutputTestForIndexMask) { long idx = keyToChar[k] & kUCKeyOutputGetIndexMask; if (stateRec && idx < stateRec->keyStateRecordCount) { UCKeyStateRecord* rec = reinterpret_cast(data + stateRec->keyStateRecordOffsets[idx]); - if (matches(rec->stateZeroCharData)) { - return k; - } + keyMatches = matches(rec->stateZeroCharData); } } else if (!(keyToChar[k] & kUCKeyOutputSequenceIndexMask) && keyToChar[k] < 0xFFFE) { - if (matches(keyToChar[k])) { - return k; - } + keyMatches = matches(keyToChar[k]); + } + + if (keyMatches) { + requiredModifiers = qtModifiers; + return k; } } } @@ -390,7 +406,8 @@ static QKeySequence translateToKeyboardLayout(UCKeyboardLayout* keyboard, const const Qt::Key qKey = keyCombination.key(); bool found = false; - UInt32 keyNativeCode = nativeKeycode(keyboard, qKey, found); + Qt::KeyboardModifiers requiredModifiers; + UInt32 keyNativeCode = nativeKeycode(keyboard, qKey, requiredModifiers, found); if (!found) { LOGD() << "Key " << qKey << " not found in the keyboard layout"; return {}; @@ -399,7 +416,7 @@ static QKeySequence translateToKeyboardLayout(UCKeyboardLayout* keyboard, const QString keyStr = keyCodeToString(keyboard, keyNativeCode); Qt::Key translatedQKey = QKeySequence::fromString(keyStr, QKeySequence::PortableText)[0].key(); - return QKeyCombination(keyCombination.keyboardModifiers(), translatedQKey); + return QKeyCombination(keyCombination.keyboardModifiers() | requiredModifiers, translatedQKey); } MacOSShortcutsInstanceModel::MacOSShortcutsInstanceModel(QObject* parent) @@ -469,6 +486,18 @@ static QKeySequence translateToKeyboardLayout(UCKeyboardLayout* keyboard, const LOGE() << "The keyboard layout is not valid"; } + // A character that requires modifiers (e.g. `!` is `Shift+1` on QWERTY) is translated to these modifiers and the + // key. Such translations are recorded last, so that they don't take over the shortcuts bound directly to that + // combination of keys (e.g. `@` is `Alt+2` on the Russian layout). We must not record the untranslated character + // in this case: Qt matches a key press against both the key with modifiers (`Alt+2`) and the character typed + // (`@`), and if there are shortcuts for both, it treats the key press as ambiguous and activates neither of them + struct DeferredMapping { + QString translatedSequence; + QString rawSequence; + bool autoRepeat = false; + }; + std::vector deferredMappings; + ShortcutList shortcuts = commandShortcutsRegister()->shortcuts(); for (const Shortcut& sc : shortcuts) { @@ -495,6 +524,11 @@ static QKeySequence translateToKeyboardLayout(UCKeyboardLayout* keyboard, const } else { QString translatedSequenceStrNormalised = translatedSequence.toString(QKeySequence::PortableText); + if (translatedSequence[0].keyboardModifiers() != untranslatedSequence[0].keyboardModifiers()) { + deferredMappings.push_back({ translatedSequenceStrNormalised, untranslatedSequenceStr, sc.autoRepeat }); + continue; + } + // If it was successful, record the translated sequence too, and map it to the untranslated sequence // Again, map to non-normalised recordMapping(translatedSequenceStrNormalised, untranslatedSequenceStr, false); @@ -503,6 +537,11 @@ static QKeySequence translateToKeyboardLayout(UCKeyboardLayout* keyboard, const } } + for (const DeferredMapping& mapping : deferredMappings) { + recordMapping(mapping.translatedSequence, mapping.rawSequence, false); + recordAutoRepeat(mapping.translatedSequence, mapping.autoRepeat); + } + if (inputSource) { CFRelease(inputSource); } diff --git a/framework/shortcuts_v2/tests/macosshortcutsinstancemodel_tests.cpp b/framework/shortcuts_v2/tests/macosshortcutsinstancemodel_tests.cpp index d940c05c07..b7cc14d5e1 100644 --- a/framework/shortcuts_v2/tests/macosshortcutsinstancemodel_tests.cpp +++ b/framework/shortcuts_v2/tests/macosshortcutsinstancemodel_tests.cpp @@ -22,6 +22,7 @@ #include #include +#include #include #include @@ -107,9 +108,14 @@ class Shortcuts_MacOSShortcutsInstanceModelTests : public ::testing::Test m_model->init(); } - QStringList registeredSequences() const + static QString normalized(const QString& sequence) { - return m_model->shortcuts().keys(); + return QKeySequence::fromString(sequence, QKeySequence::PortableText).toString(QKeySequence::PortableText); + } + + bool isRegistered(const QString& sequence) const + { + return m_model->shortcuts().contains(normalized(sequence)); } //! NOTE Simulates pressing the shortcut: `sequence` is what Qt sees and passes to the QML `Shortcut` object, @@ -122,7 +128,7 @@ class Shortcuts_MacOSShortcutsInstanceModelTests : public ::testing::Test activated = seq; }); - m_model->activate(sequence); + m_model->activate(normalized(sequence)); ::testing::Mock::VerifyAndClearExpectations(m_controller.get()); @@ -141,7 +147,7 @@ class Shortcuts_MacOSShortcutsInstanceModelTests : public ::testing::Test } /** - * @brief Shortcuts with a character that requires Shift must not be translated to the character of the same key without Shift + * @brief Shortcuts with a character that requires Shift are translated to Shift and the key * @details `:` is `Shift+;` on the US layout. It used to be translated to `;`, so the shortcut with `:` took over `;` */ TEST_F(Shortcuts_MacOSShortcutsInstanceModelTests, US_ColonDoesNotOverrideSemicolon) @@ -153,13 +159,14 @@ TEST_F(Shortcuts_MacOSShortcutsInstanceModelTests, US_ColonDoesNotOverrideSemico //! [WHEN] The shortcuts are loaded for the US layout loadShortcuts(US_LAYOUT); - //! [THEN] Both sequences are registered - EXPECT_TRUE(registeredSequences().contains(":")); - EXPECT_TRUE(registeredSequences().contains(";")); + //! [THEN] `:` is registered as the keys that type it + EXPECT_TRUE(isRegistered("Shift+;")); + EXPECT_TRUE(isRegistered(";")); + EXPECT_FALSE(isRegistered(":")); //! [THEN] Each one activates its own shortcut EXPECT_EQ(activate(";"), ";"); - EXPECT_EQ(activate(":"), ":"); + EXPECT_EQ(activate("Shift+;"), ":"); } TEST_F(Shortcuts_MacOSShortcutsInstanceModelTests, US_SemicolonDoesNotOverrideColon) @@ -173,7 +180,7 @@ TEST_F(Shortcuts_MacOSShortcutsInstanceModelTests, US_SemicolonDoesNotOverrideCo //! [THEN] Each sequence activates its own shortcut EXPECT_EQ(activate(";"), ";"); - EXPECT_EQ(activate(":"), ":"); + EXPECT_EQ(activate("Shift+;"), ":"); } /** @@ -181,28 +188,35 @@ TEST_F(Shortcuts_MacOSShortcutsInstanceModelTests, US_SemicolonDoesNotOverrideCo */ TEST_F(Shortcuts_MacOSShortcutsInstanceModelTests, US_ShiftedCharactersDoNotOverrideUnshifted) { + struct Case { + std::string shifted; + QString shiftedKeys; + std::string unshifted; + }; + //! [GIVEN] Pairs of shortcuts with a character and the character of the same key without Shift - const std::vector > pairs = { - { "_", "-" }, - { "(", "9" }, - { "+", "=" }, - { "Alt++", "Alt+=" }, - { "{", "[" }, - { "}", "]" }, + const std::vector cases = { + { "_", "Shift+-", "-" }, + { "(", "Shift+9", "9" }, + // `+` is also on the keypad, so it is kept as is + { "+", "+", "=" }, + { "Alt++", "Alt++", "Alt+=" }, + { "{", "Shift+[", "[" }, + { "}", "Shift+]", "]" }, }; - for (size_t i = 0; i < pairs.size(); ++i) { - addShortcut("test://shifted-" + std::to_string(i), { pairs[i].first }); - addShortcut("test://unshifted-" + std::to_string(i), { pairs[i].second }); + for (size_t i = 0; i < cases.size(); ++i) { + addShortcut("test://shifted-" + std::to_string(i), { cases[i].shifted }); + addShortcut("test://unshifted-" + std::to_string(i), { cases[i].unshifted }); } //! [WHEN] The shortcuts are loaded for the US layout loadShortcuts(US_LAYOUT); //! [THEN] Each sequence activates its own shortcut - for (const auto& [shifted, unshifted] : pairs) { - EXPECT_EQ(activate(QString::fromStdString(unshifted)), unshifted); - EXPECT_EQ(activate(QString::fromStdString(shifted)), shifted); + for (const Case& c : cases) { + EXPECT_EQ(activate(QString::fromStdString(c.unshifted)), c.unshifted); + EXPECT_EQ(activate(c.shiftedKeys), c.shifted); } } @@ -218,8 +232,8 @@ TEST_F(Shortcuts_MacOSShortcutsInstanceModelTests, German_ColonDoesNotOverrideOt loadShortcuts(GERMAN_LAYOUT); //! [THEN] Each sequence activates its own shortcut - EXPECT_EQ(activate(":"), ":"); - EXPECT_EQ(activate(";"), ";"); + EXPECT_EQ(activate("Shift+."), ":"); + EXPECT_EQ(activate("Shift+,"), ";"); EXPECT_EQ(activate("."), "."); EXPECT_EQ(activate(","), ","); } @@ -237,8 +251,8 @@ TEST_F(Shortcuts_MacOSShortcutsInstanceModelTests, Russian_LatinLettersAreTransl loadShortcuts(RUSSIAN_LAYOUT); //! [THEN] The sequences are registered for the Cyrillic letters on the same keys - EXPECT_TRUE(registeredSequences().contains("Ctrl+Ф")); - EXPECT_TRUE(registeredSequences().contains("Ctrl+Я")); + EXPECT_TRUE(isRegistered("Ctrl+Ф")); + EXPECT_TRUE(isRegistered("Ctrl+Я")); //! [THEN] Pressing the keys activates the original shortcuts EXPECT_EQ(activate("Ctrl+Ф"), "Ctrl+A"); @@ -247,14 +261,57 @@ TEST_F(Shortcuts_MacOSShortcutsInstanceModelTests, Russian_LatinLettersAreTransl TEST_F(Shortcuts_MacOSShortcutsInstanceModelTests, Russian_ColonDoesNotOverrideSemicolon) { - //! [GIVEN] `;` is on the `Ж` key, while `:` is typed with Shift on the Russian layout + //! [GIVEN] `;` is on the `Ж` key, while `:` is `Shift+5` on the Russian layout addShortcut("test://a", { ":" }); addShortcut("test://b", { ";" }); //! [WHEN] The shortcuts are loaded for the Russian layout loadShortcuts(RUSSIAN_LAYOUT); - //! [THEN] `;` is translated to the `Ж` key, and `:` is kept as is + //! [THEN] Both are translated to the keys that type them EXPECT_EQ(activate("Ж"), ";"); - EXPECT_EQ(activate(":"), ":"); + EXPECT_EQ(activate("Shift+5"), ":"); +} + +/** + * @brief Option shortcuts don't become ambiguous with the shortcuts for the characters typed with Option + * @details `Option+1` types `!` and `Option+2` types `@` on the Russian layout. Qt matches such a key press against + * both `Alt+1` and `!`, and if there are shortcuts for both, it activates neither of them + */ +TEST_F(Shortcuts_MacOSShortcutsInstanceModelTests, Russian_OptionShortcutsAreNotAmbiguous) +{ + //! [GIVEN] Shortcuts with Option and the characters typed by the same keys + addShortcut("test://a", { "!" }); + addShortcut("test://b", { "@" }); + addShortcut("test://c", { "Alt+1" }); + addShortcut("test://d", { "Alt+2" }); + + //! [WHEN] The shortcuts are loaded for the Russian layout + loadShortcuts(RUSSIAN_LAYOUT); + + //! [THEN] The characters are not registered as is + EXPECT_FALSE(isRegistered("!")); + EXPECT_FALSE(isRegistered("@")); + + //! [THEN] The Option shortcuts take precedence over the characters typed by the same keys + EXPECT_EQ(activate("Alt+1"), "Alt+1"); + EXPECT_EQ(activate("Alt+2"), "Alt+2"); + + //! [THEN] `!` is still available with Shift + EXPECT_EQ(activate("Shift+1"), "!"); +} + +/** + * @brief A character that is typed with Option activates its shortcut, if there is no Option shortcut for that key + */ +TEST_F(Shortcuts_MacOSShortcutsInstanceModelTests, Russian_CharacterTypedWithOption) +{ + //! [GIVEN] `@` is `Option+2` on the Russian layout + addShortcut("test://a", { "@" }); + + //! [WHEN] The shortcuts are loaded for the Russian layout + loadShortcuts(RUSSIAN_LAYOUT); + + //! [THEN] Pressing `Option+2` activates the shortcut + EXPECT_EQ(activate("Alt+2"), "@"); } From 707b4f6ae6f55947f4cbfa81742769404942d368 Mon Sep 17 00:00:00 2001 From: Elnur Ismailzada Date: Thu, 29 Oct 2026 14:50:17 +0200 Subject: [PATCH 4/4] fixed characters being translated to the keypad keys Problem: nativeKeycode() could find a character on a keypad key, while keyCodeToString() translates keypad keys to fixed characters regardless of the layout. E.g. on the Russian layout the keypad decimal key types `,`, so `Alt+,` was registered as `Alt+.` and made `Alt+Shift+7` ambiguous (`Shift+7` types `.`). Changes: - Keypad keys are skipped when searching for a character, as they have their own shortcuts (`Num+...`). --- .../macos/macosshortcutsinstancemodel.mm | 38 +++++++++++ .../macosshortcutsinstancemodel_tests.cpp | 65 ++++++++++++++++++- 2 files changed, 100 insertions(+), 3 deletions(-) diff --git a/framework/shortcuts_v2/qml/Muse/Shortcuts/platform/macos/macosshortcutsinstancemodel.mm b/framework/shortcuts_v2/qml/Muse/Shortcuts/platform/macos/macosshortcutsinstancemodel.mm index 78e597ab57..a21cfccd06 100644 --- a/framework/shortcuts_v2/qml/Muse/Shortcuts/platform/macos/macosshortcutsinstancemodel.mm +++ b/framework/shortcuts_v2/qml/Muse/Shortcuts/platform/macos/macosshortcutsinstancemodel.mm @@ -173,6 +173,33 @@ static UInt32 keyboardType() return LMGetKbdType(); } +static bool isKeypadKey(UInt32 keyNativeCode) +{ + switch (keyNativeCode) { + case kVK_ANSI_Keypad0: + case kVK_ANSI_Keypad1: + case kVK_ANSI_Keypad2: + case kVK_ANSI_Keypad3: + case kVK_ANSI_Keypad4: + case kVK_ANSI_Keypad5: + case kVK_ANSI_Keypad6: + case kVK_ANSI_Keypad7: + case kVK_ANSI_Keypad8: + case kVK_ANSI_Keypad9: + case kVK_ANSI_KeypadDecimal: + case kVK_ANSI_KeypadMultiply: + case kVK_ANSI_KeypadPlus: + case kVK_ANSI_KeypadClear: + case kVK_ANSI_KeypadDivide: + case kVK_ANSI_KeypadEnter: + case kVK_ANSI_KeypadMinus: + case kVK_ANSI_KeypadEquals: + return true; + default: + return false; + } +} + //! NOTE requiredModifiers receives the modifiers needed to type the character with the returned key static UInt32 nativeKeycode(UCKeyboardLayout* keyboard, Qt::Key keyCode, Qt::KeyboardModifiers& requiredModifiers, bool& found) { @@ -343,6 +370,12 @@ static UInt32 nativeKeycode(UCKeyboardLayout* keyboard, Qt::Key keyCode, Qt::Key UCKeyOutput* keyToChar = reinterpret_cast(data + charTable->keyToCharTableOffsets[tableNum]); for (UInt32 k = 0; k < charTable->keyToCharTableSize; k++) { + // The keypad keys have their own shortcuts (`Num+...`), and keyCodeToString doesn't translate them + // according to the layout (e.g. the keypad decimal key types `,` on the Russian layout, not `.`) + if (isKeypadKey(k)) { + continue; + } + bool keyMatches = false; if (keyToChar[k] & kUCKeyOutputTestForIndexMask) { long idx = keyToChar[k] & kUCKeyOutputGetIndexMask; @@ -403,6 +436,11 @@ static QKeySequence translateToKeyboardLayout(UCKeyboardLayout* keyboard, const const QKeyCombination keyCombination = sequence[0]; + // The keypad keys are not searched for characters (see nativeKeycode), so the keypad shortcuts are kept as is + if (keyCombination.keyboardModifiers() & Qt::KeypadModifier) { + return sequence; + } + const Qt::Key qKey = keyCombination.key(); bool found = false; diff --git a/framework/shortcuts_v2/tests/macosshortcutsinstancemodel_tests.cpp b/framework/shortcuts_v2/tests/macosshortcutsinstancemodel_tests.cpp index b7cc14d5e1..21a2417953 100644 --- a/framework/shortcuts_v2/tests/macosshortcutsinstancemodel_tests.cpp +++ b/framework/shortcuts_v2/tests/macosshortcutsinstancemodel_tests.cpp @@ -44,6 +44,7 @@ namespace muse::shortcuts { static const QString US_LAYOUT = "com.apple.keylayout.US"; static const QString GERMAN_LAYOUT = "com.apple.keylayout.German"; static const QString RUSSIAN_LAYOUT = "com.apple.keylayout.Russian"; +static const QString FRENCH_LAYOUT = "com.apple.keylayout.French"; class TestMacOSShortcutsInstanceModel : public MacOSShortcutsInstanceModel { @@ -198,9 +199,8 @@ TEST_F(Shortcuts_MacOSShortcutsInstanceModelTests, US_ShiftedCharactersDoNotOver const std::vector cases = { { "_", "Shift+-", "-" }, { "(", "Shift+9", "9" }, - // `+` is also on the keypad, so it is kept as is - { "+", "+", "=" }, - { "Alt++", "Alt++", "Alt+=" }, + { "+", "Shift+=", "=" }, + { "Alt++", "Alt+Shift+=", "Alt+=" }, { "{", "Shift+[", "[" }, { "}", "Shift+]", "]" }, }; @@ -315,3 +315,62 @@ TEST_F(Shortcuts_MacOSShortcutsInstanceModelTests, Russian_CharacterTypedWithOpt //! [THEN] Pressing `Option+2` activates the shortcut EXPECT_EQ(activate("Alt+2"), "@"); } + +/** + * @brief Characters are not translated to the keypad keys + * @details The keypad decimal key types `,` on the Russian layout, so `Alt+,` used to be translated to that key, + * i.e. to `Alt+.`, which clashed with `Alt+Shift+7` (`Shift+7` types `.` on the Russian layout) + */ +TEST_F(Shortcuts_MacOSShortcutsInstanceModelTests, Russian_CharactersAreNotTranslatedToKeypad) +{ + //! [GIVEN] Shortcuts with `,` and `.`, and with the keys that type `.` with Shift + addShortcut("test://a", { "Alt+," }); + addShortcut("test://b", { "Alt+." }); + addShortcut("test://c", { "Alt+Shift+7" }); + + //! [WHEN] The shortcuts are loaded for the Russian layout + loadShortcuts(RUSSIAN_LAYOUT); + + //! [THEN] `,` and `.` are translated to the `Б` and `Ю` keys, like the Latin letters + EXPECT_EQ(activate("Alt+Б"), "Alt+,"); + EXPECT_EQ(activate("Alt+Ю"), "Alt+."); + + //! [THEN] `Alt+.` is not registered, so `Alt+Shift+7` is not ambiguous + EXPECT_FALSE(isRegistered("Alt+.")); + EXPECT_EQ(activate("Alt+Shift+7"), "Alt+Shift+7"); +} + +/** + * @brief Keypad shortcuts are registered as is + * @details Characters are not searched on the keypad keys, so e.g. `+` would be translated to `Shift+=`, + * and `Num++` to `Shift+Num+=`, which Qt never produces for the keypad key + */ +TEST_F(Shortcuts_MacOSShortcutsInstanceModelTests, US_KeypadShortcutsAreNotTranslated) +{ + //! [GIVEN] Keypad shortcuts with characters that require Shift on the main keys + addShortcut("test://a", { "Num++" }); + addShortcut("test://b", { "Num+*" }); + addShortcut("test://c", { "Num+1" }); + + //! [WHEN] The shortcuts are loaded for the US layout + loadShortcuts(US_LAYOUT); + + //! [THEN] Pressing the keypad keys activates the shortcuts + EXPECT_EQ(activate("Num++"), "Num++"); + EXPECT_EQ(activate("Num+*"), "Num+*"); + EXPECT_EQ(activate("Num+1"), "Num+1"); +} + +TEST_F(Shortcuts_MacOSShortcutsInstanceModelTests, French_KeypadDigitsAreNotTranslated) +{ + //! [GIVEN] The digits require Shift on the main keys of the French layout (`1` is `Shift+&`) + addShortcut("test://a", { "Num+1" }); + addShortcut("test://b", { "1" }); + + //! [WHEN] The shortcuts are loaded for the French layout + loadShortcuts(FRENCH_LAYOUT); + + //! [THEN] The keypad digit is kept as is, while the digit on the main keys is translated + EXPECT_EQ(activate("Num+1"), "Num+1"); + EXPECT_EQ(activate("Shift+&"), "1"); +}