search

enzo1982 / mp4v2

Reviving the MP4v2 project...
https://mp4v2.org
Other
140 stars 50 forks source link

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

Closed sandreas closed 2 years ago

sandreas commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 https://github.com/cmus/cmus/issues/1112#issuecomment-940380997)

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 commented 2 years ago

Are you not planning to go for C compatibility again? The C++ additions from TechSmith raised some critics in the community?! (nullptr, see https://github.com/cmus/cmus/issues/1112#issuecomment-940380997)

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.

:thumbsup:

enzo1982 commented 2 years ago

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 commented 2 years ago

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:

sandreas commented 2 years ago

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 commented 2 years ago

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.