-
Notifications
You must be signed in to change notification settings - Fork 67
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
Add option to show key under transparent keys, and find the layer name during parsing #67
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.
Thanks! Looks good to me in general, a few comments and questions noted.
@@ -237,6 +237,9 @@ class ParseConfig(BaseSettings, env_prefix="KEYMAP_", extra="ignore"): | |||
# legend to output for transparent keys | |||
trans_legend: str | dict = {"t": "▽", "type": "trans"} | |||
|
|||
# whether to show the keycode this transparent key will activate | |||
trans_show_lower_key: bool = False |
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.
Can you add an item to CONFIGURATION.md as well?
self.layer_activated_from: dict[int, set[int]] = {} # layer to key positions | ||
self.layer_activated_from: dict[ | ||
int, set[tuple[int, int]] | ||
] = {} # layer to [layer index, key position] from keys |
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.
] = {} # layer to [layer index, key position] from keys | |
] = {} # layer to (layer index, key position) from keys |
layer_name = self.layer_names[layer_idx] | ||
from_layer = layers[layer_name] |
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.
layer_name = self.layer_names[layer_idx] | |
from_layer = layers[layer_name] | |
from_layer = layers[self.layer_names[layer_idx]] |
layer_name = self.layer_names[layer_idx] | ||
from_layer = layers[layer_name] | ||
lower_key = from_layer[key] | ||
if lower_key == self.trans_key: |
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.
Is this comparison why the hash function is necessary?
@@ -17,7 +17,7 @@ class QmkJsonParser(KeymapParser): | |||
_tog_re = re.compile(r"(TG|TO|DF)\((\d+)\)") | |||
_mts_re = re.compile(r"([A-Z_]+)_T\((\S+)\)") | |||
_mtl_re = re.compile(r"MT\((\S+), *(\S+)\)") | |||
_lt_re = re.compile(r"LT\((\d+), *(\S+)\)") | |||
_lt_re = re.compile(r"LT\((\S+), *(\S+)\)") |
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.
I am not sure I understand how this happens, can you actually have layer name defines instead of indices in keymap.json? Do you use it alongside e.g. config.h
that contains layer defines?
@@ -50,23 +50,32 @@ def mapped(key: str) -> LayoutKey: | |||
key = self._prefix_re.sub("", key) | |||
return LayoutKey.from_key_spec(self.cfg.qmk_keycode_map.get(key, key.replace("_", " "))) | |||
|
|||
def parse_layer(layer: str) -> int: | |||
assert self.layer_names is not None |
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.
If I am not wrong, the only way we know layer names is still via user input, i.e. --layer-names
. If so, I think it would be nice to have an informative error message here, perhaps like below?
assert self.layer_names is not None | |
assert self.layer_names is not None, "Please provide `--layer-names` if you use layer defines in keymap.json" |
Pinging on this, see my questions above. |
afbdb42
to
140ca43
Compare
Hi, I needed these two features and implemented them. I think others might want them as well
Let me know if you have any questions!