Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Restructure command line utilities to support long options without short form #2

Closed
sandreas opened this issue Feb 13, 2022 · 7 comments

Comments

@sandreas
Copy link

I came across the issue, that in the command line utilities the whole implementation is based on using options by having a single char as short option and a string as long option is a MUST, which limits the addable options to (readable) ASCII chars, but in fact they are even more limited to a-z A-Z 0-9, since -$ would not be very intuitive. And there are not many available chars for mp4tags left.

Not having to provide a short option for every parameter, so that existing API stuff could be wrapped into the utilities would be great.

So what I basically did in my branch is to take existing API functions and wrap them around a new option to support them to be set via command line and chose any available char instead of one that really made sense.

Here is an example:

#define OPT_SORT_NAME    'f'
#define OPT_SORT_ARTIST   'F'
#define OPT_SORT_ALBUM_ARTIST   'J'
#define OPT_SORT_ALBUM   'k'
#define OPT_SORT_COMPOSER   'K'
#define OPT_SORT_TV_SHOW   'W'

#define OPT_STRING  "r:A:a:b:c:C:d:D:e:E:g:G:H:i:I:j:l:L:m:M:n:N:o:O:p:P:B:R:s:S:t:T:x:X:w:y:z:Z:f:F:J:k:K:W:"

  "  -f, -sortname    STR  Set the sort name\n"
  "  -F, -sortartist  STR  Set the sort artist\n"
  "  -k, -sortalbum   STR  Set the sort album\n"
  "  -W, -sorttvshow  STR  Set the sort tv show\n"
  "  -J, -sortalbumartist STR  Set the sort album artist\n"
  "  -K, -sortcomposer    STR  Set the sort composer\n" 

  { "sortname",    prog::Option::REQUIRED_ARG, 0, OPT_SORT_NAME    },
  { "sortartist",  prog::Option::REQUIRED_ARG, 0, OPT_SORT_ARTIST  },
  { "sortalbum",   prog::Option::REQUIRED_ARG, 0, OPT_SORT_ALBUM   },
  { "sorttvshow",  prog::Option::REQUIRED_ARG, 0, OPT_SORT_TV_SHOW },
  { "sortalbumartist",   prog::Option::REQUIRED_ARG, 0, OPT_SORT_ALBUM_ARTIST },
  { "sortcomposer",      prog::Option::REQUIRED_ARG, 0, OPT_SORT_COMPOSER     },

  case OPT_SORT_NAME:
      MP4TagsSetSortName( mdata, tags[i] );
      break;
  case OPT_SORT_ARTIST:
      MP4TagsSetSortArtist( mdata, tags[i] );
      break;
  case OPT_SORT_ALBUM_ARTIST:
      MP4TagsSetSortAlbumArtist( mdata, tags[i] );
      break;
  case OPT_SORT_ALBUM:
      MP4TagsSetSortAlbum( mdata, tags[i] );
      break;
  case OPT_SORT_COMPOSER:
      MP4TagsSetSortComposer( mdata, tags[i] );
      break;
  case OPT_SORT_TV_SHOW:
      MP4TagsSetSortTVShow( mdata, tags[i] );
      break;
@enzo1982
Copy link
Owner

Long options without short form are already supported.

For utilities not using the mp4v2::util::Utility class (e.g. mp4tags), look at how the --help and --version options are implemented in mp4tags.

For utilities using the Utility class (e.g. mp4chaps), you would just pass 0 as the first argument to Group::add().

At some point I would like to make all utilities use the Utility class in order to make option handling uniform across all utilities. Not for the initial release, though.

It would be great if you could open a pull request to add the options listed above (after getting rid of the short forms). I'll then include that in the 2.1.0 release.

@sandreas
Copy link
Author

mp4v2::util::Utility class

Are you not planning to go for C compatibility again? The C++ additions from TechSmith raised some critics in the community?! (nullptr, see cmus/cmus#1112 (comment))

It would be great if you could open a pull request to add the options listed above (after getting rid of the short forms). I'll then include that in the 2.1.0 release.

Ok, I'm pretty busy atm but I'll try.

@enzo1982
Copy link
Owner

Are you not planning to go for C compatibility again? The C++ additions from TechSmith raised some critics in the community?! (nullptr, see cmus/cmus#1112 (comment))

That refers to the public headers in include/mp4v2 only. The public headers in this fork are and will stay C compatible. The rest of the code is C++ anyway.

However, there was some critique about using nullptr in the C++ code as well, as that needs C++11 and locks out older compilers. To address that, my goal for this port is to keep the code compatible with C++98 for now.

Ok, I'm pretty busy atm but I'll try.

👍

@enzo1982
Copy link
Owner

enzo1982 commented Feb 20, 2022

I added options for sort tags and the purchase date to mp4tags in the mp4tags branch. Please take a look.

While long options without a short equivalent were already possible in theory, mp4tags' option parsing, especially for the --remove option, was not compatible with that. So I've reworked that and added a new syntax for the --remove option to allow removing tags that don't have a short option. E.g. --remove sortartist,sortname. This also works with all the other tags and should really be the preferred way to remove tags.

@sandreas
Copy link
Author

You sir, deserve a cookie.

Really nice work (I can't test in the next days - but it looks good to me). If this should go main, I could finally delete my messy mp4v2 fork (which of course I won't, because there are already relying projects on it, but I will definitely mark it as archived and refer to your new one).

I really appreciate it.

BTW, this should go to a new issue, but if you would like to add some more atoms and parameters, you could refer to:

Here are some examples for additions, that I thought about integrating:

  • publisher -> ©pub
  • bpm -> tmpo
  • compilation -> cpil
  • conductor -> ©con
  • narrator -> ©nrt
  • movementname -> ©mvn
  • movement -> ©mvi
  • movementtotal -> ©mvc

@sandreas
Copy link
Author

So, works for me. As soon as this branch goes into main, there is no need to maintain my fork any more and I will point it to yours.

enzo1982 added a commit that referenced this issue Feb 23, 2022
See GitHub issue #2 for details: #2
@enzo1982
Copy link
Owner

Great! The changes are now merged into main.

I opened a new issue for the additional tag options. Most of them are not supported by the library yet, so I'll add them after the 2.1.0 release together with some more suggested by @bergert.

Closing this issue.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants