Clean up #182's code #321

eltang · 2016-05-12T05:33:59Z

@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.

jackhumbert · 2016-05-12T05:39:15Z

Looks fine to me!

eltang · 2016-05-12T05:45:26Z

So what about enabling it by default?

ezuk · 2016-05-12T11:03:04Z

Hi Eric,

Where do we stand on this?

78bd31f#commitcomment-17390915

eltang · 2016-05-12T14:13:12Z

@ezuk I haven't gotten around to that yet. I plan to redo some of my features, and that's going to take some time. Don't worry, I tested if this code compiled and @vifon tested if it worked.

eltang · 2016-05-16T03:53:47Z

@jackhumbert It seems that some conflicts have arisen. Would you like me to resolve them?

jackhumbert · 2016-05-16T03:59:40Z

Sure! I share @ezuk's sentiments though, and would appreciate it if you could test it yourself in addition.

eltang · 2016-05-16T04:06:32Z

@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?

jackhumbert · 2016-05-16T04:36:46Z

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?

eltang · 2016-05-16T04:43:41Z

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.

eltang · 2016-05-16T15:47:12Z

@jackhumbert I've run into a bit of a problem. Is there a reason you're using keycodes rather than action codes in quantum.c?

jackhumbert · 2016-05-16T15:49:37Z

Yeah, it's very much done on purpose - can you describe the problem?

eltang · 2016-05-16T16:54:36Z

Sorry for the delay. I took out layer_switch_get_layer and it's impossible to get back a uint16_t variable from calling keymap_key_to_keycode like you're doing now. I'm pretty sure that the code member of the action_t variable that layer_switch_get_action returns will behave exactly the same, though. Would using that be okay?

jackhumbert · 2016-05-16T16:56:12Z

Ah, I didn't notice that - yeah, that's not going to get merged, sorry.

eltang · 2016-05-16T16:59:59Z

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.

eltang · 2016-05-16T17:12:02Z

How's that? This compiles, but I still need to test it on my board.

jackhumbert · 2016-05-16T17:12:44Z

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?

eltang · 2016-05-16T17:16:00Z

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 find_layer and layer_switch_get_action.

vifon · 2016-05-16T17:26:22Z

@vifon was talking about separating find_layer and layer_switch_get_action.

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.

eltang · 2016-05-16T17:28:48Z

@vifon Whoops, I misunderstood that then. Which changes were you unsure about?

jackhumbert · 2016-05-16T19:02:57Z

tmk_core/common/action_layer.c

+ /* 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);


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.

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's nothing in action_for_key that modifies KC_TRNS keys - it's mostly for the (boot)magic stuff.

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 process_record.~~ Actually, it's a lot more than that. keycode_to_action absolutely must be called, or else none of the aliases are going to work.

jackhumbert · 2016-05-16T19:20:59Z

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.

eltang · 2016-05-16T19:36:40Z

It makes the code that handles the cache much clearer, and it also improves the API with the additions to action_util.c. I felt responsible for cleaning up at least this code because I was the one who sent the original code, which was quite messy, to @vifon. I know you might be more interested in new features, but I think some code cleanup might also be beneficial—QMK's gotten a lot more features recently, and I've seen that you've had to disable some lesser-used ones so that the firmware wouldn't be too big to flash.

jackhumbert · 2016-05-16T19:45:10Z

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.

eltang · 2016-05-16T19:51:39Z

My new matrix.c should shave a good bit of the size of the firmware. :)

Could you please explain what you mean by "layer stuff"? I simply restored layer_switch_get_action to the way it was before #182—I'm not changing anything. I understood the problem, I would gladly fix it.

jackhumbert · 2016-05-16T19:57:07Z

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.

eltang · 2016-05-16T20:00:50Z

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?

jackhumbert · 2016-05-16T20:02:45Z

Feel free to open them - they'll at least be up here, then.

eltang · 2016-05-16T20:04:34Z

Nice. :)

eltang · 2016-05-16T23:22:54Z

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

eltang mentioned this pull request May 12, 2016

Clean up code vifon/qmk_firmware#4

Closed

Clean up code

ca61ed5

jackhumbert reviewed May 16, 2016
View reviewed changes

jackhumbert closed this May 16, 2016

eltang deleted the source_layers_cache_fix branch June 21, 2016 02:19

BlueTufa pushed a commit to BlueTufa/qmk_firmware that referenced this pull request Aug 6, 2021

Updated Keyboard Exclusion List: 2019-04-25 (qmk#321)

8a8cad4

New entries: - claw44 (defaults to claw44/rev1) - treadstone48 (defaults to treadstone48/rev1)

Jpe230 pushed a commit to Jpe230/qmk_firmware that referenced this pull request Jul 12, 2023

add ozone tactical (qmk#321)

b1647a6

* add ozone tactical * Remove trash * Update copyright header

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up #182's code #321

Clean up #182's code #321

eltang commented May 12, 2016

jackhumbert commented May 12, 2016

eltang commented May 12, 2016

ezuk commented May 12, 2016

eltang commented May 12, 2016 •

edited

Loading

eltang commented May 16, 2016

jackhumbert commented May 16, 2016

eltang commented May 16, 2016 •

edited

Loading

jackhumbert commented May 16, 2016

eltang commented May 16, 2016 •

edited

Loading

eltang commented May 16, 2016

jackhumbert commented May 16, 2016

eltang commented May 16, 2016

jackhumbert commented May 16, 2016

eltang commented May 16, 2016 •

edited

Loading

eltang commented May 16, 2016

jackhumbert commented May 16, 2016

eltang commented May 16, 2016 •

edited

Loading

vifon commented May 16, 2016

eltang commented May 16, 2016 •

edited

Loading

jackhumbert May 16, 2016

eltang May 16, 2016 •

edited

Loading

jackhumbert May 16, 2016

eltang May 16, 2016 •

edited

Loading

jackhumbert commented May 16, 2016

eltang commented May 16, 2016

jackhumbert commented May 16, 2016

eltang commented May 16, 2016 •

edited

Loading

jackhumbert commented May 16, 2016

eltang commented May 16, 2016 •

edited

Loading

jackhumbert commented May 16, 2016

eltang commented May 16, 2016

eltang commented May 16, 2016

Clean up #182's code #321

Clean up #182's code #321

Conversation

eltang commented May 12, 2016

jackhumbert commented May 12, 2016

eltang commented May 12, 2016

ezuk commented May 12, 2016

eltang commented May 12, 2016 • edited Loading

eltang commented May 16, 2016

jackhumbert commented May 16, 2016

eltang commented May 16, 2016 • edited Loading

jackhumbert commented May 16, 2016

eltang commented May 16, 2016 • edited Loading

eltang commented May 16, 2016

jackhumbert commented May 16, 2016

eltang commented May 16, 2016

jackhumbert commented May 16, 2016

eltang commented May 16, 2016 • edited Loading

eltang commented May 16, 2016

jackhumbert commented May 16, 2016

eltang commented May 16, 2016 • edited Loading

vifon commented May 16, 2016

eltang commented May 16, 2016 • edited Loading

jackhumbert May 16, 2016

Choose a reason for hiding this comment

eltang May 16, 2016 • edited Loading

Choose a reason for hiding this comment

jackhumbert May 16, 2016

Choose a reason for hiding this comment

eltang May 16, 2016 • edited Loading

Choose a reason for hiding this comment

jackhumbert commented May 16, 2016

eltang commented May 16, 2016

jackhumbert commented May 16, 2016

eltang commented May 16, 2016 • edited Loading

jackhumbert commented May 16, 2016

eltang commented May 16, 2016 • edited Loading

jackhumbert commented May 16, 2016

eltang commented May 16, 2016

eltang commented May 16, 2016

eltang commented May 12, 2016 •

edited

Loading

eltang commented May 16, 2016 •

edited

Loading

eltang commented May 16, 2016 •

edited

Loading

eltang commented May 16, 2016 •

edited

Loading

eltang commented May 16, 2016 •

edited

Loading

eltang commented May 16, 2016 •

edited

Loading

eltang May 16, 2016 •

edited

Loading

eltang May 16, 2016 •

edited

Loading

eltang commented May 16, 2016 •

edited

Loading

eltang commented May 16, 2016 •

edited

Loading