MonoS / SupMover

SupMover - Shift timings and Screen Area of PGS/Sup subtitle
GNU Affero General Public License v3.0
39 stars 4 forks source link

Dash prefix for options & Prettier usage #17

Closed YourMJK closed 5 months ago

YourMJK commented 5 months ago

Since the "modes" act as options on the CLI rather than subcommands (or values), I added the standard "--" prefix to them and added a prettier multiline usage help. It's backward compatible, the old syntax without the prefixes still works.

I initially thought of adding a --verbose/-v and/or --trace flag with a different PR to make it easier to analyze the structure of a particular PGS, debug and get feedback of what was changed in the various modes. Hence this idea to first change the others options/flags as well.

What do you think?

MonoS commented 5 months ago

Thank you, seems better than what I've initially developed (when only crop and delay modes existed).

Regarding the verbose/trace mode i was thinking more of an info mode which would output information like

YourMJK commented 5 months ago

Good ideas! How about this for new options?

Of course, --info and --trace wouldn't require an output file, so you could think about adding an --output option instead or just making the current positional argument optional, i.e. SupMover <input.sup> [<output.sup>] [OPTIONS ...]. Or even SupMover [OPTIONS ...] <input.sup> [<output.sup>] if you don't care about backward compatibility.

Unfortunately, I'm busy with something else for the next two weeks, but after that I would like to work on these things as well as fixing Cut&Merge (unless you want to of course, lol).

MonoS commented 4 months ago

I had to think a bit about that, personally i'm ok in deprecating the backward compatibility mode that specified only the delay parameter, but i'm not so positive in deprecating the whole positional arguments thing because, as far as i know, there are other people using it and don't want to change neither mine nor their workflow, but I'm curious in how would you handle making the output optional, maybe that one can be implemented, i was thinking about looking if the second parameter ends with an extension, if so treat it as output file, otherwise it's an option and must be parsed, what do you think?

If you are interested in working in those implementation, i'll be happy to have you take a look at it when you have time, i'm not in a hurry :)

YourMJK commented 4 months ago

As it turns out, I did have some time yesterday and already started implementing an optional output file argument for my proposed --trace option: https://github.com/YourMJK/SupMover/commit/451adbac3218af16a7bfe755cc9385ca9e22148f

I went with the easiest solution, which was SupMover [OPTIONS ...] <input.sup> [<output.sup>], so parsing the options first. As you mentioned, this is not something you want, since it breaks backwards-compatibility with the current v2.4.1 syntax, which I can understand.

But I can easily change it back to SupMover <input.sup> [<output.sup>] [OPTIONS ...], since another straight-forward idea would be to just start parsing the options at index 2 (where the output argument might be) and if the first argument isn't a recognized option, it's assumed to be the output file.

MonoS commented 4 months ago

Yeah, i'd prefer, thank you :)

For the --trace option i'd suggest doing something like --index N to print only the subpic with compositionNumber N.

Also, in my repo there is a kaitai structure file, you can use its webide to explore the structure of the subtitle.