search

keyboardio / Chrysalis

Graphical configurator for Kaleidoscope-powered keyboards
https://github.com/keyboardio/Chrysalis#chrysalis
GNU General Public License v3.0
496 stars 64 forks source link

changing keyboard layout preference doesn't update preview #1142

Open tlyu opened 2 years ago

tlyu commented 2 years ago

Describe the bug When changing the Keyboard Layout (OS keyboard layout) preference, going back to the Layout Editor shows the updated key map on the key picker, but not on the keyboard preview.

To Reproduce Steps to reproduce the behavior:

  1. Go to 'Preferences'
  2. Under Keyboard Layout, select 'German'
  3. Go back to 'Layout & Colormap Editor'
  4. Observe that the key picker has updated
  5. Observe that the keyboard preview still shows QWERTY
  6. Go to 'Select another keyboard.
  7. Disconnect, then reconnect.
  8. Observe that both the preview and the key picker show the German layout.

Expected behavior The keyboard preview should update its key labels without needing to disconnect and reconnect.

Desktop (please complete the following information):

Additional context This might be a regression introduced by some of the refactoring that was intended to reduce Focus traffic.

algernon commented 2 years ago

I think this is partially a result of caching in ActiveDevice, because the main SVG depends on keymap only. Prior to caching, we pulled the data from the keyboard, and that rebuilt the keymap on the fly, turning keycodes into the internal representation. Now we're caching the entire representation.

The easiest fix is to invalidate the keymap, but that'd pull it again for the keyboard, which would be unfortunate. We only want to invalidate the labels. Not sure yet how to solve that, but it's the caching that's the culprit.

algernon commented 2 years ago

This also affects the macro editor, by the way, because that stores the looked-up Key object too.

Not quite sure how to fix this well. My current ideas are:

  1. Adjust ActiveDevice to always refresh the keymap and macros results, map each key within through a db.lookup(key.code). This has the advantage of not being too invasive. We need to do it in ActiveDevice rather than focus, because of caching, we want to refresh the cache, and that's ActiveDevice's job. The downside is that it's not future-proof, if we end up with more commands that cache Keys, we'll need to refresh those too.
  2. Keep the accessors in ActiveDevice as-is, and provide a refresh() or setLayout() method instead, which remaps all the stored Keys in all the cached values where it is necessary. This has the advantage of not being too invasive, and not having to do the refresh on every access, but rather make the refresh explicit. The downside is that it's not future-proof, if we end up with more commands that cache Keys, we'll need to refresh those too.
  3. Just not store Key objects, but do a lookup on-demand. This has the advantage of covering every current and future case, at the cost of significant changes to internals, and at the additional cost of having to perform on-demand lookups, perhaps multiple times for the same value.