Add danish and finnish layout #42
Conversation
Signed-off-by: José Daniel Montoya Salazar <jd.montoya.s8@gmail.com> Signed-off-by: Andrea Ricchi <andrea.ricchi@amarulasolutions.com>
Summary of ChangesHello @AndreaRicchi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request expands the application's internationalization capabilities by introducing support for Danish and Finnish keyboard layouts. This enhancement allows users who prefer these languages to have a native and efficient typing experience, making the application more accessible to a broader audience. The changes involve adding new layout definitions and updating core components to recognize and utilize them. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for Danish and Finnish keyboard layouts. The implementation is mostly correct, but there are a few areas for improvement. My review includes suggestions to improve code organization in CMakeLists.txt and DeclarativeInputEngine.cpp for better long-term maintainability. More importantly, I've identified significant code duplication in the new QML layout files (DaLayout.qml and FiLayout.qml) and have proposed a refactoring to a shared base component, which would make the code much easier to maintain.
| import QtQuick 2.0 | ||
| import QtQuick.Layouts 1.12 | ||
|
|
||
| ColumnLayout { | ||
| property var inputPanel | ||
|
|
||
| RowLayout { | ||
| property real keyWeight: 160 | ||
|
|
||
| Key { | ||
| btnKey: Qt.Key_Q | ||
| btnText: "q" | ||
| inputPanelRef: inputPanel | ||
| } | ||
|
|
||
| Key { | ||
| btnKey: Qt.Key_W | ||
| btnText: "w" | ||
| inputPanelRef: inputPanel | ||
| } | ||
|
|
||
| Key { | ||
| btnKey: Qt.Key_E | ||
| btnText: "e" | ||
| alternativeKeys: "éèêë" | ||
| inputPanelRef: inputPanel | ||
| } | ||
|
|
||
| Key { | ||
| btnKey: Qt.Key_R | ||
| btnText: "r" | ||
| inputPanelRef: inputPanel | ||
| } | ||
|
|
||
| Key { | ||
| btnKey: Qt.Key_T | ||
| btnText: "t" | ||
| inputPanelRef: inputPanel | ||
| } | ||
|
|
||
| Key { | ||
| btnKey: Qt.Key_Y | ||
| btnText: "y" | ||
| inputPanelRef: inputPanel | ||
| } | ||
|
|
||
| Key { | ||
| btnKey: Qt.Key_U | ||
| btnText: "u" | ||
| alternativeKeys: "úùûü" | ||
| inputPanelRef: inputPanel | ||
| } | ||
|
|
||
| Key { | ||
| btnKey: Qt.Key_I | ||
| btnText: "i" | ||
| alternativeKeys: "íìîï" | ||
| inputPanelRef: inputPanel | ||
| } | ||
|
|
||
| Key { | ||
| btnKey: Qt.Key_O | ||
| btnText: "o" | ||
| alternativeKeys: "óòôö" | ||
| inputPanelRef: inputPanel | ||
| } | ||
|
|
||
| Key { | ||
| btnKey: Qt.Key_P | ||
| btnText: "p" | ||
| inputPanelRef: inputPanel | ||
| } | ||
|
|
||
| Key { | ||
| btnKey: Qt.Key_Aring | ||
| btnText: "å" | ||
| inputPanelRef: inputPanel | ||
| } | ||
|
|
||
| BackspaceKey { | ||
| inputPanelRef: inputPanel | ||
| } | ||
|
|
||
| } | ||
|
|
||
| RowLayout { | ||
| property real keyWeight: 160 | ||
|
|
||
| Key { | ||
| weight: 56 | ||
| functionKey: true | ||
| showPreview: false | ||
| btnBackground: "transparent" | ||
| } | ||
|
|
||
| Key { | ||
| btnKey: Qt.Key_A | ||
| btnText: "a" | ||
| alternativeKeys: "áàâä" | ||
| inputPanelRef: inputPanel | ||
| } | ||
|
|
||
| Key { | ||
| btnKey: Qt.Key_S | ||
| btnText: "s" | ||
| inputPanelRef: inputPanel | ||
| } | ||
|
|
||
| Key { | ||
| btnKey: Qt.Key_D | ||
| btnText: "d" | ||
| inputPanelRef: inputPanel | ||
| } | ||
|
|
||
| Key { | ||
| btnKey: Qt.Key_F | ||
| btnText: "f" | ||
| inputPanelRef: inputPanel | ||
| } | ||
|
|
||
| Key { | ||
| btnKey: Qt.Key_G | ||
| btnText: "g" | ||
| inputPanelRef: inputPanel | ||
| } | ||
|
|
||
| Key { | ||
| btnKey: Qt.Key_H | ||
| btnText: "h" | ||
| inputPanelRef: inputPanel | ||
| } | ||
|
|
||
| Key { | ||
| btnKey: Qt.Key_J | ||
| btnText: "j" | ||
| inputPanelRef: inputPanel | ||
| } | ||
|
|
||
| Key { | ||
| btnKey: Qt.Key_K | ||
| btnText: "k" | ||
| inputPanelRef: inputPanel | ||
| } | ||
|
|
||
| Key { | ||
| btnKey: Qt.Key_L | ||
| btnText: "l" | ||
| inputPanelRef: inputPanel | ||
| } | ||
|
|
||
| Key { | ||
| btnKey: Qt.Key_Odiaeresis | ||
| btnText: "ö" | ||
| inputPanelRef: inputPanel | ||
| } | ||
|
|
||
| Key { | ||
| btnKey: Qt.Key_Adiaeresis | ||
| btnText: "ä" | ||
| inputPanelRef: inputPanel | ||
| } | ||
|
|
||
| EnterKey { | ||
| weight: 283 | ||
| inputPanelRef: inputPanel | ||
| } | ||
|
|
||
| } | ||
|
|
||
| RowLayout { | ||
| property real keyWeight: 156 | ||
|
|
||
| ShiftKey { | ||
| } | ||
|
|
||
| Key { | ||
| btnKey: Qt.Key_Z | ||
| btnText: "z" | ||
| inputPanelRef: inputPanel | ||
| } | ||
|
|
||
| Key { | ||
| btnKey: Qt.Key_X | ||
| btnText: "x" | ||
| inputPanelRef: inputPanel | ||
| } | ||
|
|
||
| Key { | ||
| btnKey: Qt.Key_C | ||
| btnText: "c" | ||
| inputPanelRef: inputPanel | ||
| } | ||
|
|
||
| Key { | ||
| btnKey: Qt.Key_V | ||
| btnText: "v" | ||
| inputPanelRef: inputPanel | ||
| } | ||
|
|
||
| Key { | ||
| btnKey: Qt.Key_B | ||
| btnText: "b" | ||
| inputPanelRef: inputPanel | ||
| } | ||
|
|
||
| Key { | ||
| btnKey: Qt.Key_N | ||
| btnText: "n" | ||
| inputPanelRef: inputPanel | ||
| } | ||
|
|
||
| Key { | ||
| btnKey: Qt.Key_M | ||
| btnText: "m" | ||
| inputPanelRef: inputPanel | ||
| } | ||
|
|
||
| Key { | ||
| btnKey: Qt.Key_Comma | ||
| btnText: "," | ||
| inputPanelRef: inputPanel | ||
| } | ||
|
|
||
| Key { | ||
| btnKey: Qt.Key_Period | ||
| btnText: "." | ||
| inputPanelRef: inputPanel | ||
| } | ||
|
|
||
| Key { | ||
| btnKey: Qt.Key_Minus | ||
| btnText: "-" | ||
| inputPanelRef: inputPanel | ||
| } | ||
|
|
||
| ShiftKey { | ||
| weight: 204 | ||
| } | ||
|
|
||
| } | ||
|
|
||
| RowLayout { | ||
| property real keyWeight: 154 | ||
|
|
||
| SymbolKey { | ||
| weight: availableLanguageLayouts.length === 1 ? 217 : 108.5 | ||
| } | ||
|
|
||
| LanguageKey { | ||
| visible: availableLanguageLayouts.length > 1 | ||
| weight: 108.5 | ||
| } | ||
|
|
||
| SpaceKey { | ||
| weight: 1168 | ||
| inputPanelRef: inputPanel | ||
| } | ||
|
|
||
| Key { | ||
| btnKey: Qt.Key_Apostrophe | ||
| btnText: "'" | ||
| inputPanelRef: inputPanel | ||
| } | ||
|
|
||
| HideKey { | ||
| weight: 205 | ||
| } | ||
|
|
||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
This file is almost an exact copy of DaLayout.qml, with only two keys being different. This introduces significant code duplication, which will make future maintenance harder. Any change to the common layout parts would need to be applied to both files.
To avoid this, I recommend refactoring by creating a reusable base layout component. For example, you could create a NordicQwertyLayout.qml that contains the common layout structure, and then use it in both DaLayout.qml and FiLayout.qml, passing the specific keys as properties.
For example, a base layout could expose properties for the keys that differ:
// NordicQwertyLayout.qml
// ...
property alias specialKey1: specialKey1_id
property alias specialKey2: specialKey2_id
RowLayout {
// ... middle row keys
Key { id: specialKey1_id; ... }
Key { id: specialKey2_id; ... }
// ...
}Then FiLayout.qml could be simplified to:
// FiLayout.qml
NordicQwertyLayout {
specialKey1.btnKey: Qt.Key_Odiaeresis
specialKey1.btnText: "ö"
specialKey2.btnKey: Qt.Key_Adiaeresis
specialKey2.btnText: "ä"
}This would greatly reduce code duplication and improve maintainability.
| "qml/DaLayout.qml" | ||
| "qml/FiLayout.qml" |
There was a problem hiding this comment.
For better maintainability, it's good practice to keep this list of files sorted alphabetically. The newly added files break the existing (partial) alphabetical order.
DaLayout.qml should be placed after CySrBsLayout.qml.
FiLayout.qml should be placed after EsLayout.qml.
Consider sorting the entire list alphabetically to make it easier to navigate.
| {DeclarativeInputEngine::Da, {"DaLayout", "Dansk"}}, | ||
| {DeclarativeInputEngine::Fi, {"FiLayout", "Suomi"}}, |
There was a problem hiding this comment.
For consistency and readability, it would be better to group the Nordic languages together in this map, similar to how they are grouped in the InputLayouts enum in DeclarativeInputEngine.h. The Sv (Swedish) layout is defined elsewhere in this map. Consider moving this and the Sv entry together to improve maintainability.
AndreaRicchi
changed the title
Fixes #41