donniebreve / touchcursor-linux

TouchCursor style keyboard remapping for Linux.
GNU General Public License v2.0
133 stars 28 forks source link

Add in a bunch of aliases, so that `KEY_` is never needed, and some more you might expect like `-` for `MINUS`.. #60

Closed magnus-ISU closed 1 year ago

magnus-ISU commented 1 year ago

There is a problem, and I don't really care if this doesn't get merged: I rewrote the string checking with C++ features. The reason I did this was so we can quickly hash the string and use a fast switch statement, rather than having 800 calls to strcmp in a row.

If the goal is to work in places that don't have a C++ compiler, this should probably be rewritten back to a giant chain of ifs. I tried to see if I could write the macros with pure C, but don't think the compiler is smart enough.

donniebreve commented 1 year ago

My goal is not to use C++, so if you can figure out how to accomplish what you want in C, I'll probably merge it. The original code in question only happens when reading the config file, so I don't think performance is much of a concern.

Adda0 commented 1 year ago

The original code in question only happens when reading the config file, so I don't think performance is much of a concern.

Due to this, I think the performance improvements are insignificant if merging this would require the project to switch to C++. I will probably try to merge this with my own C++ port and see how the changes perform. However, if no one finds a way to port this functionality back to C, I would refrain from merging the PR into the master branch here unless there is an ongoing attempt to port to C++ anyway.

magnus-ISU commented 1 year ago

Okay, I rewrote the macros to use pure C for this PR.

They no longer hash the strings either, so there's not really a point to the macros anymore - however, they are more readable I think, so I think they're fine to keep) - some people online say you shouldn't implement anything in macros though, and there is some small merit to that view, so I can just remove them if you like.

magnus-ISU commented 1 year ago

Also @Adda0 I'm fairly certain it is impossible to do string hashing and matching in that way in C. You cannot switch on a statement like

switch (s) {
case "hello"[0]:
  do_something();
}

So there is no conceivable way for the compiler to use string constants to do this.

You can switch on

switch (n) {
case 1+1:
  do_something();
}

So it is maybe possible that a macro like

S_SWITCH (s) {
S_CASE(m,y,_,s,t,r,i,n,g) do_something()
}

Could be written - however, this is ugly and I have no motivation to do so...

Adda0 commented 1 year ago

Also @Adda0 I'm fairly certain it is impossible to do string hashing and matching in that way in C.

Yeah, you cannot. You would have to find a solution to hash the string constant and the compared string and compare the hashes, as in your previous implementation, only in C now. It is fine with me that we have to use strcmp().

As for readability, I am OK with this approach of using macros for switch-like structure encapsulating the call to strcmp(), but I consider this a matter of personal preference. Hence, I am not sure if the added readability is sufficient reason to introduce macros. I would vote for yes, it is enough, personally.

I quite like the idea of being able to omit KEY_ or use the literal symbols (', etc.) instead of their names KEY_APOSTROPHE. Nevertheless, if we agree on merging this, we should definitely update the default config comments and README.md to inform the users about the possibility of using ', KEY_APOSTROPHE and APOSTROPHE interchangeably to label specific keys. Furthermore, I would consider showing an example of using the shortened version as well as the literal symbol in the default config, but maybe kept using KEY_ names in the default config for all key bindings except for the example.

magnus-ISU commented 1 year ago

have you decided this isn't something you want, or any input? I personally think writing

C=LEFTCTRL C
J=UP
K=DOWN
K=LEFT

etc is much easier than prefixing everything with KEY_, and there isn't a point to making this harder for users. But I guess if it isn't something you want, I don't care too much about getting it merged. I can open another PR without the macros though if it is something you are open to still.

donniebreve commented 1 year ago

Sorry, I rewrote the commit history in an attempt to remove some collaborators, this was inadvertently closed. I'm interested in the aliases, but not in the switch macros or trying to write something to hash strings to improve performance (at least not without some performance investigation).

If you'd like to create a new PR (using the latest main branch) with just the aliases I'll take it. If that's too much to ask, I will add the aliases on my own so you can still get that functionality.

magnus-ISU commented 1 year ago

I'm unsure how easy it is for me to make a new PR off the master branch, considering I already have a fork. I'll make an attempt here though

magnus-ISU commented 1 year ago

Okay. Here is the file for keys.c you want. But github isn't letting me make a PR.

https://raw.githubusercontent.com/magnus-ISU/touchcursor-linux/extra-aliases-all-ifs/src/keys.c

It's probably easiest if you just copy this over your version of the file.

donniebreve commented 1 year ago

Thanks for the source, I was able to set you as the author of the commit.