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

Dash prefix for options & Prettier usage #17

Merged
merged 2 commits into from
Feb 2, 2024

Conversation

YourMJK
Copy link
Contributor

@YourMJK YourMJK commented Feb 2, 2024

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
Copy link
Owner

MonoS commented Feb 2, 2024

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

  • Number of subpic
  • Current subtitle size
  • maximum windows size (as you can't crop more than this)
  • object cropped flag set (until I understand how to handle it)
  • Maximum luma value in palettes
  • Maximum move allowed without clamping
  • AABB for all the subpic (as you can crop up to this without moving anything)
    Maybe something else, it's all I could think of right now

@MonoS MonoS merged commit 442352d into MonoS:master Feb 2, 2024
2 checks passed
@YourMJK YourMJK deleted the prettier-usage branch February 2, 2024 21:40
@YourMJK
Copy link
Contributor Author

YourMJK commented Feb 2, 2024

Good ideas! How about this for new options?

  • --info: Features you listed. Essentially a summary after file was read once.
  • --verbose: Log actions as they are performed, e.g. "deleted X segments between XX:XX:XX and XX:XX:XX" for Cut&Merge.
  • --trace: Print hierarchical output about structure+content (similar to mkvinfo) during reading. With timestamps, segment fields like coordinates etc. Useful for debugging or analyzing a file.

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
Copy link
Owner

MonoS commented Feb 5, 2024

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
Copy link
Contributor Author

YourMJK commented Feb 5, 2024

As it turns out, I did have some time yesterday and already started implementing an optional output file argument for my proposed --trace option:
YourMJK@451adba

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
Copy link
Owner

MonoS commented Feb 5, 2024

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.

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.

2 participants