Skip to content
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

Refactor Hitag S operations into separate submenu #2472

Merged
merged 4 commits into from
Aug 26, 2024

Conversation

CiRIP
Copy link
Contributor

@CiRIP CiRIP commented Aug 25, 2024

Rationale: Hitag S is sufficiently different from other Hitags that it requires special treatment for every operation. Having a mutually-exclusive set of arguments (--ht2, --hts, etc.) for a command is counter-intuitive, so moving Hitag S, 1 and 2 operations into separate submenus makes sense (see also: lf em).

@CiRIP
Copy link
Contributor Author

CiRIP commented Aug 25, 2024

future work: unify hitags_config_t with struct hitagS_tag

@iceman1001
Copy link
Collaborator

Yeah, the twist everything into same command category "lf hitag" is ruff. So seperating it is a good thing. Makes easier to follow and change code. All good.
With that said, the choice of lf hitag s as its own sub category is less good from user perspective.
We use -s as a param for save file. lf hitag s dump -s would look like a typo.

With lf em it was easier since the card family has simple short names.

A. New second level category

   lf hitag2 
   lf hitag1 
   lf hitags  

B. longer third level names

lf hitag ht2
lf hitag ht1
lf hitag hts

@doegox @mwalker33 any suggestions?

client/src/cmdlfhitags.c Outdated Show resolved Hide resolved
client/src/cmdlfhitags.c Outdated Show resolved Hide resolved
@CiRIP
Copy link
Contributor Author

CiRIP commented Aug 25, 2024

Yeah, lf hitag s dump -s looks a bit dumb. My vote would be with lf hitag hts.

@iceman1001
Copy link
Collaborator

ok, go for alternative two. lf hitag ht2 style

@CiRIP
Copy link
Contributor Author

CiRIP commented Aug 26, 2024

fixed

@iceman1001 iceman1001 merged commit 3bf0f10 into RfidResearchGroup:master Aug 26, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants