-
Notifications
You must be signed in to change notification settings - Fork 105
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
Refactor to support extra layouts #61
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me! Thanks! Is there more you want to do?
@kattni This triggers duplicate-code too. :-( I don't think it makes sense to deduplicate it. Do we have a way to disable it locally? I wish pylint-dev/pylint#214 was implemented. |
Space is very tight on the CPX and other boards where this library is frozen in. We might need to split off other layouts into separate libraries. |
@tannewt I'd like to fully test the FR layout and add a few extra accentuated chars before release, then I'll try to move on German, at least a first prototype. With small flash boards in mind, would it make sense to only have the ancestor layout class included in the base lib, then an extra lib per layout? |
@AngainorDev Ya, many files is the best way to minimize RAM cost. |
Hello, (sorry for bad english translated) I am a French user, I have been working on the Fr keyboard for a while now, which is why I allow myself a few comments on this project. To have the French keyboard, you must have a table of 255 characters, the first 6 bits will identify the key and the last 2 bits for the modifiers (normal 0x00, shift 0x80, altgr 0x40), there is only one problem for the key 0x64 (Europe key () that will have to be renamed to 0x03 for example. So it is not necessary to treat Altgr separately, you can include it in the table. This is a start, but it is not enough, you need to double the bytes to process the dead keys, i.e. a total of 512 bytes but that could be reduced by starting the array at the value 32 and doubling the bytes starting from the value 128 (is to be studied) for example, which would give an array of (128 * 2) + 96 = 352 bytes The good news is that it will be easy to implement an Fr Mac version, all useful bridge modifiers will be in the array (normal, shift, alt and shift + alt) It will then be easy to add the altcodes to have special features for windows, to print characters independently of the keyboard and unicode characters. I did this in c ++ for Arduino Leonardo and Adafruit QT PY (tiny usb) boards, you can see here: If the implementation is not complete for keyboards, it will force us to modify the table and associated functions anyway. I made a Fr translation and tested on the raspberry pi pico with an array of 512 bytes (no problem on the Pico), the file takes up space because the array, also I added some functions like the altcodes for windows |
Thanks a lot for the feedback @nico7885, much appreciated. There are different angles I think:
With that in mind, I tried to
As for encoding altgr with 7th bit 0x40, is this compatible with US mapping? German one? Other iso charsets? Same I'd say for dead keys or chars requiring 2 scan codes, like ï for instance, needing 2 key strokes on the keyboard.. Not pretending to have a definitive answer there, I'm experimenting. Media keys are something I did not consider at all for the moment. |
I think there is no problem, only two keys that change 0x32(no problem) and 0x64 (need to remap) Am I making an error where the array is stored in the flash memory of the card currently? In general, the choice to make a keyboard will limit the number of external library because it is a specific use. I would like to point out that the ability to add the altgr in the array is only valid for Circuitpython and is not true for the Arduino lib, due to their processing procedure. I am providing you with the library that I use with the Raspberry pi Pico that I tested with the Pimonori RGB Keypad test board, it may be useful to you or give you ideas. it is not final, this is my first code in python, please be indulgent. In any case, it was easier and faster than I would have thought. Functions for displaying text: -> To display text with all possible characters in Windows -> To display text with all the possible characters but independently of the keyboard under Windows |
@tannewt We could make another exception and increase the |
Checked the dup portion: As a workaround, would adding a different comment on these "dup" lines help pass the CI? |
@AngainorDev Unfortunately, we have it set to ignore comments when checking for duplicated code, so in this case, that wouldn't work. If it is unreasonable to change the duplicated code in this specific case, then we will increase the Are you running Pylint locally? Does it fail the same way for you locally? |
Not running pylint locally, but I can see why it gives that warning. I can also see the reason for that dup test. |
@AngainorDev I was asking if you were running it locally because I was going to have you test to see to what we would need to increase the threshold to avoid the duplication issue, but if you're not running it locally, that would take ages. I'll sort out getting it done. Please be patient while I sort out someone to test it or do it myself in the next couple of days. Thanks! |
Ok @FoamyGuy is going to see to what we need to change the threshold to get this passing. Thanks! |
@kattni
I am unsure of the proper fix for this one. Perhaps make it into a static method, or move it outside of the class into it's own individual helper function? If there is a preferred way to deal with it let me know and I can take care of it as well when I come back to merge master to this and resolve the conflicts. |
It could be made a |
Agreed with @dhalbert, @staticmethod can do for the moment. I'll likely refactor again with @nico7885 hints later on anyway. |
Thanks Nico! Feel free to reply in French (or both) and either us English speakers can use Google Translate or the bilingual folks can help bridge the conversation back to us. |
# Conflicts: # .pylintrc
This is updated now with a large enough duplication threshold, and has had master branch merged in to resolve the conflicts. I've added the pylint ignore as well to make this passing. I'm not confident that I understand it's usage enough to successfully swap it to a static method after looking into it a bit further. And I'm not sure how to test it afterward if I did try. Feel free to add further commits to change that. Thanks for all of your work on this @AngainorDev and @nico7885. This is great stuff! |
@tannewt, French: Les exemples fournis pour les raccourcis ne fonctionneront pas avec les autres langues, envisagez vous des solutions (mettre à jour les définitions des touches, créer une fonction dédiée ou autres choses?) Google Translate: The examples provided for shortcuts will not work with other languages, do you see any solutions (update key definitions, create ** a dedicated function ** or other things?) |
@nico7885 Pourrais-tu donner un exemple concret de raccourcis et de comment ça se traduit coté scancode ? |
Bonjour AngainorDev, Comment ça se traduit coté scancode: |
Ok. Comment tu verrais cet encodage des raccourcis ? Il existe des formats déjà définis ? |
Le soucis est que pour faire ctrl-A indépendamment du layout on doit faire par exemple: # ctrl-A:
kbd.press(Keycode.CONTROL)
kbd.press(*layout.keycodes('a'))
kbd.release_all() Il serait utile de convertir les keycodes et pouvoir faire quelque chose comme Comme compromis j'avais pensé dupliquer les fonctions de class KeyboardLayout:
def press(self, *keycodes):
for keycode in keycodes:
if isinstance(keycode,str):
self.keyboard.press(*self.keycodes(keycode))
else:
self.keyboard.press(keycode)
def release(self, *keycodes):
for keycode in keycodes:
if isinstance(keycode,str):
self.keyboard.release(*self.keycodes(keycode))
else:
self.keyboard.release(keycode)
def send(self, *keycodes):
self.press(*keycodes)
self.keyboard.release_all() Permettant: layout.send(Keycode.CONTROL,"a") |
@Neradoc , c'est exactement l'idée, c'est un ajout non négligeable même si la façon de le faire peut en dérouter certains; cela dit même avec une redéfinition des keycodes, cela n'aurait pas été plus simple pour les caractères en dehors de A-Z sans consulter la table. |
Ok, je vois mieux, merci pour l'exemple ! Je pensais à un encodage spécifique sous forme de chaine pour aussi pouvoir définir ces raccourcis en dehors du code, par l'utilisateur. Pour rebondir sur l'exemple de @Neradoc, je dirais qu'on peut partir sur l'interface qu'il propose, avec un petit tweak. Pour gérer aussi des raccourcis plus tordus en une fois, je suggère qu'on ajoute la chaine vide comme indicateur de "release_all". @tannewt @dhalbert I can translate should Google not be precise enough |
Je comprends assez bien le français, mai pardonnez svp tous mes petites fautes. Il y a longtemps...
Pour ça, nous pouvons créer une autre librarie "add-on" for chaque autre langue. Pour les raccourcis (nouveau mot vocabulaire pour moi 🙂 ), suffit-il de créer une classe
etc.
Cette idée est intigrante, bien qu'elle ajoute à la taille du code dans la librarie de base. |
AngainorDev, Quelque soit la solution adoptée, @Neradoc, @AngainorDev ou de @dhalbert; c'est une bonne nouvelle, si vous décidez de le mettre en place. Sinon si je peux me permettre une interrogation, quel est la problématique qui empêche d'avoir les notifications leds (caps lock, num lock, scoll lock etc...) avec circuitpython sachant qu'elle est basée sur tinyusb qui le gère parfaitement, est ce que cela pourrait être intégré dans le futur éventuellement? |
Did you see this recent merged PR? #62 |
@dhalbert, |
Is this ready to merge? I didn't follow the last discussion. |
I think this is left to do or discuss:
The USB spec is chauvinistically US-centric, unfortunately. I would have liked to see them just label the keys in rows in some neutral way, but they didn't do that. |
Third option, unsure this makes sense in your philosophy of easing things up for the user:
As for merging, I'd like to fully explore the altgr encoding as 7th bit by @nico7885, that will remove the need for the "altgr" list, and likely give a try at the shortcut things as well. |
@AngainorDev have you worked in the factor as previously mentioned in your comments? |
So, I am using a modified version of the KeyboardLayout class in my layouts bundle, I'm not sure how to propose the changes on a PR, I think they are a little too spread out to use the review system. It's over there: https://github.com/Neradoc/Circuitpython_Keyboard_Layouts/blob/main/libraries/keyboard_layout.py
|
Superseded by #84. Thank you for initial work on this! |
Working, but I don't think this is ready to merge yet.
Opening a PR for discussion.
Common KeyboardLayout class, US and FR layouts so far.
I opted to keep the main packed table with SHIFT modifier encoded as first bit, for space reasons.
Delaing with accentuated chars means
The result is a very hybrid approach that may be space efficient but not that elegant.
I used a function to deal with the rare above ascii chars, and a string to list the very few chars needing the ALTGR modifier.
So, that's kinda 3 different methods at once, for space savings sake.
I'd like to clean that up, but not at the expanse of space.