Skip to content

Firngrod/refactor key actions#1512

Merged
kareltucek merged 1 commit intoUltimateHackingKeyboard:masterfrom
firngrod:firngrod/refactor_key_actions
Mar 23, 2026
Merged

Firngrod/refactor key actions#1512
kareltucek merged 1 commit intoUltimateHackingKeyboard:masterfrom
firngrod:firngrod/refactor_key_actions

Conversation

@firngrod
Copy link
Contributor

Refactored key_action_t to a key_definition_t and a key_action_t because it seemed like a reasonable thing to do before allocating a whole bunch of instances of a class with members which I had no intention of using. Also, what really put me over the edge was my intention to add an isChorded member to the class I would otherwise have been using to represent chord actions, and that just seemed silly to me.

I have made a number of judgement calls on whether to go with the larger or smaller class across the code, feel free to disagree with me on those. Also on naming.

I can build a release package, and I am running rightv1 on my board right now.

@firngrod
Copy link
Contributor Author

Oops, didn't go from master. Will rebase and all that.

@firngrod firngrod force-pushed the firngrod/refactor_key_actions branch from 06d59ff to 08f6190 Compare March 17, 2026 20:50
@firngrod
Copy link
Contributor Author

Still builds, now running this version.

@firngrod firngrod marked this pull request as ready for review March 18, 2026 07:23
@firngrod
Copy link
Contributor Author

Ever since I rebased this onto master, my VSCode has had this thing where any time I switch to a file, I cannot input anything for a few seconds. @kareltucek, have you added anything to the project lately which may explain that?
I removed the two SDKs (zephyr and the other one) from the workspace again after building the release to test that this refactor built properly.
To be fair, my computer is a 7-8 year old laptop, but it's an old workstation, it still shouldn't be this slow. It's not out of RAM.

@kareltucek
Copy link
Collaborator

There were some refactors regarding VSCode stuff, but I think on the c2usb branch only. Brief diffing doesn't turn up any .json changes since 16.2.0

@kareltucek
Copy link
Collaborator

I feel a bit apprehensive about the additional visual clutter caused by adding another nesting level to things, but memory-wise and abstraction-wise, I can't deny that this is justified.

Should we merge it now?

@firngrod
Copy link
Contributor Author

I get the concern. Can you think of a way to do this without that issue? One does kind of miss inheritance once in a while. I can think of hacky ways to do it, by mirroring the member layouts across two structs, one being an expanded version of the other, then casting pointers, but it just gets, well, hacky!
I have tried to use the appropriate types where possible to reduce the visual clutter, maybe more could be done in that direction. There was a bit of code in some color parsing where I think some refactoring might be beneficial, but I'm not sure.
A simple improvement could be to rename key_definition_t.action to key_definition_t.a?
I could also just continue the chords project without this refactor, that code doesn't care at all, suck up the additional memory usage, and then delay the refactor until we really need it?

@kareltucek
Copy link
Collaborator

kareltucek commented Mar 23, 2026

I don't think this can be done better. This is the correct way to abstract it.

A simple improvement could be to rename key_definition_t.action to key_definition_t.a?

I am not sure that is an improvement.


Let's use this as is.

Should I merge it now?

@kareltucek
Copy link
Collaborator

(I mean, is it ready for merge from your side @firngrod ?)

@firngrod
Copy link
Contributor Author

Well. it works on my 60v1, so I am happy. I will leave it up to you to verify it on an 60v2 and 80.

@kareltucek kareltucek merged commit 01c3b26 into UltimateHackingKeyboard:master Mar 23, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants