search

sergei-mironov / xkb-switch

Switch your X keyboard layouts from the command line
MIT License
345 stars 37 forks source link

Use standard C procedures for handling command-line arguments #53

Closed sergei-mironov closed 3 years ago

sergei-mironov commented 3 years ago

We need replace homegrown command-line arguments parser with getopt or alike. The goal is to make xkb-switch CLI handling more standard. For example, now it doesn't process 'tightly coupled' one-letter flags, like -ld instead of -l -d. Getopt family of functions could do it.

Contributions are welcome here.

  1. Locate the loop which parses command line arguements in XKbSwitch.cpp
  2. Rewrite it using getopt family of C functions
  3. Adjust linking libraries in CMakeLists.txt if required
  4. Make sure that ./test.sh still prints OK
  5. Send me a pull request

Please, do not introduce non-standard dependencies like boost. Use stdlib C or C++ < 11 tools.

sonuishaq67 commented 3 years ago

I can try this @grwlf

sergei-mironov commented 3 years ago

@sonuishaq67 OK, then take it. Feel free to post questions here.

sergei-mironov commented 3 years ago

I have added few words to the description

sonuishaq67 commented 3 years ago

Im using getopt3 and i have a doubt even though I specified no_argument in options it asks for argument in small option ex : ./xkb-switch --version works

but ./xkb-switch -v doesnt work but gives an error of ./xkb-switch: option requires an argument -- 'v'

./xkb-switch -v. works nvm I figured it out

sonuishaq67 commented 3 years ago

How to check which test is failing ? @grwlf Figured it out All test are passing now Ill send a pr

sergei-mironov commented 3 years ago

@sonuishaq67 Thank you for help!

sergei-mironov commented 3 years ago

Hi @sonuishaq67 ! I just found an issue in your implementation. Currently xkb-switch doesn't accept --arg=value format for set. So, ./xkb-switch --set=us doesn't work. But getopts's manpage says that it does support this format. This is likely because you read argument of -s from argv[i++] explicitly, while the correct way should be to use getopt-API. I think one should try reading the values from optarg.

Could you please correct this?

sonuishaq67 commented 3 years ago

Okay sure I'll try to correct it

sonuishaq67 commented 3 years ago

Hey @grwlf A few functions are not required because getopt already covers them Should I remove those ?

Edit : For now I didn't remove those

sergei-mironov commented 3 years ago

Hey @grwlf A few functions are not required because getopt already covers them Should I remove those ?

Edit : For now I didn't remove those

Yep, I would prefer not to use temp variable, but the change is trivial so I removed it already and also made a few small adjustments. Thank you!