Option to look in a custom directory for keyboard layouts#623
Option to look in a custom directory for keyboard layouts#623d-perl wants to merge 4 commits intolxqt:masterfrom
Conversation
|
Hi @tsujan, very sorry to ping you directly about this, but just wondering if you might have time to look at this sometime? I would like to avoid keeping my fork open forever if possible :) |
|
It should be reviewed by someone who has mastery over this part of the code. Understandably, all changes/fixes I've done so far have been in those parts that I fully understand; sorry, this isn't one of them. I'm afraid waiting for a reviewer may be the only choice. @yan12125? |
|
Ah, I see. Sorry, I just saw your name a lot in the PRs so I didn't know who else to tag. Thanks a lot for passing the message on! |
|
Let me also call a contributor who has done a nice work recently… @liweitianux, any thought on this? |
yan12125
left a comment
There was a problem hiding this comment.
Yeah, I touched this part several years ago 😀. Here are some comments based on my old memory.
Also, https://github.com/lxqt/qtermwidget/blob/master/pyqt/sip/qtermwidget.sip should be updated to include new APIs.
9fb3aaf to
69fb8b6
Compare
69fb8b6 to
ad536dc
Compare
Ah, thanks! Feel free to merge it if you're satisfied with the changes. |
|
Thanks for the comments @yan12125 ! I addressed them as best I could and rebased on master :) |
liweitianux
left a comment
There was a problem hiding this comment.
One more thing, I find pyqt/pyproject.toml lists PyQt6 in the dependencies. Since we're adding QTermWidget to PySide6, a hard dependency on PyQt6 doesn't look good to me. Maybe better just drop PyQt6 here, or can we list both with OR-condition?
| { | ||
| if (_translatorBaseDir.has_value() && _translatorBaseDir.value().exists()) { | ||
| return _translatorBaseDir.value().filePath(name + QLatin1String(".keytab")); | ||
| } |
There was a problem hiding this comment.
Do we need to further check the existence of file = _translatorBaseDir.value().filePath(name + QLatin1String(".keytab")), and fallback to the bundled layout directory if it doesn't exist?
There was a problem hiding this comment.
That's a good idea but it could also lead to some weird behaviour, if you had similarly named ones for example. In my use case, the default path doesn't actually exist because it is not installed as a system package, and the default path is compiled in as an absolute path from a cmake variable. I'd love it if someone else would weigh in on this.
There was a problem hiding this comment.
the default path doesn't actually exist because it is not installed as a system package, ...
It seems the PR's main concern is to make qtermwidget able to find layouts instead of overriding the system/bundled layouts. Because qtermwidget currently can only find layouts from the fixed KB_LAYOUT_DIR that's set by CMake at compile-time, so the Python binding must provide a way to set the correct keyboard layout directory as the Python package can be installed anywhere.
If my above understanding is correct, I think the better API to add might be setKBLayoutDir() and patch get_kb_layout_dir(). One more thing, we can also update get_kb_layout_dir() to prefer an environment variable like QTERMWIDGET_KB_LAYOUT_DIR.
|
@liweitianux thank you for the comments! I think that for a |
Hi!
I'm working on trying to package QTermWidget as a pip-installable PySide6 package. For this, since the installation location can't be known at compile time, it would be very useful to be able to change the location searched for the keybindings. This solution is working for me but I'm happy to make any changes required.