-
-
Notifications
You must be signed in to change notification settings - Fork 39.1k
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
Clean up #182's code #321
Clean up #182's code #321
Conversation
Looks fine to me! |
So what about enabling it by default? |
Hi Eric, Where do we stand on this? |
@jackhumbert It seems that some conflicts have arisen. Would you like me to resolve them? |
Sure! I share @ezuk's sentiments though, and would appreciate it if you could test it yourself in addition. |
@jackhumbert I'm now able to compile QMK with my own keymap thanks to @ezuk's urging, for which I'm very grateful (I don't know how long I would have put things off without him). I've tested these changes before for compilation. How would you like me to test them now? |
Describing what to test would take about as long as it would for me to test things ;) do what you see fit (using things on at least one board) for now, and we'll take it from there. In the end, I'm not too worried about this particular PR, as it just adjusts code that @vifon committed earlier, but it's a good habit to get into! Re enabling by default - probably not right now. We haven't had too many complaints about things this feature addresses, have we? |
Sounds good. I guessing that a simple test of the basic functionality should suffice. I know. You can expect many more PRs from me in the future, all tested (well, except for maybe one—you'll see why). I know there haven't been complaints from normal users, but it will make development a lot easier in some cases. |
@jackhumbert I've run into a bit of a problem. Is there a reason you're using keycodes rather than action codes in |
Yeah, it's very much done on purpose - can you describe the problem? |
Sorry for the delay. I took out |
Ah, I didn't notice that - yeah, that's not going to get merged, sorry. |
The code can be made to work the same without it. Do you absolutely need it? I'll push an update and let you take a look. |
How's that? This compiles, but I still need to test it on my board. |
I must admit that I didn't look at things too closely before commenting the first time - I assumed you were simply cleaning up things that were limited to @vifon's changes, and a glance at the code reflected that. I wouldn't classify modifying core layer behavior as "clean up." I'm not really seeing a benefit to merging things like this. @vifon mentioned that it was a subjective change. Can you explain more exactly how this would improve usability? |
Where are you seeing that I am changing the behavior? I'm not doing that; this is really only a cleanup that makes the code more readable. @vifon was talking about separating |
No, I meant it in general too. The thing is, I don't know qmk's core layer well enough to review it thoroughly, that's why I suggested you ask @jackhumbert. |
@vifon Whoops, I misunderstood that then. Which changes were you unsure about? |
/* scan from the highest layer to the lowest layer */ | ||
for (int8_t layer = 31; layer >= 0; --layer) { | ||
if (master_layer_state & 1UL << layer) { | ||
action_t action = action_for_key(layer, 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.
This should be checking the keymap for KC_TRNS with keymap_key_to_keycode
(which needs to be prototyped for overriding), rather than checking the action via action_for_key
. That allows us to use keycodes rather than actions here.
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.
That's going to skip the keymap configuration stuff in keymap_common.c
, though. Are you sure you want to do that? This was what the original code looked like.
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.
There's nothing in action_for_key
that modifies KC_TRNS
keys - it's mostly for the (boot)magic stuff.
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.
Yes, but other stuff is processed in keymap_common.c
, too. For example, the keycodes for the mouse keys will not be processed properly if their bare keycodes are piped to Actually, it's a lot more than that. process_record
.keycode_to_action
absolutely must be called, or else none of the aliases are going to work.
I'm gonna go back to the basics here - why is this needed? What does it help clarify? I'm much more interested in new features at this point, rather than code clean-up. Tbh, I would rather do all the clean-up by myself. |
It makes the code that handles the cache much clearer, and it also improves the API with the additions to |
Firmware size has been and will always be an issue - we're looking at bigger chips to handle this, but there are obvious concerns for current boards. If you'd like to just stick to cleaning-up #182 in this PR, we can look at your changes to the layer stuff in another. |
My new Could you please explain what you mean by "layer stuff"? I simply restored |
I hadn't realised that was changed then. To be fair, you are still changing things now. I'm gonna put this on hold until I have time to clean the layer handling up myself. |
All right. I haven't changed anything for a few hours, but I'll leave this branch up for reference. I was about to open a few other PRs. Does this mean that I should wait on those too? |
Feel free to open them - they'll at least be up here, then. |
Nice. :) |
I could try to fix this PR if you told me what you didn't like about it. I'm still not too sure about that. |
New entries: - claw44 (defaults to claw44/rev1) - treadstone48 (defaults to treadstone48/rev1)
* add ozone tactical * Remove trash * Update copyright header
@vifon has approved the changes.
@jackhumbert @ezuk Could this feature be enabled by default sometime? It would have allowed #308's code to be much simpler, for example.