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

Update flac.md: Reordering and updating options descriptions & more #769

Merged
merged 6 commits into from
Dec 18, 2024

Conversation

H2Swine
Copy link
Contributor

@H2Swine H2Swine commented Dec 3, 2024

I am not sure about the organization of the initial part of the document, to be honest. For example the way it promises examples before options, but before the examples sections it gives examples (on using stdin and redirects).

Anyway, here are the most significant changes made:

  • Some in the examples. Less confident about what should be there.
  • Options are reorganized to match the flac -h order
  • I used underscores for italics, avoiding some formatting issue that arose from triple asterisks
  • Some " " inserted - you will all too easily need them when using -T - but I dared not use too many of them.

... and some more text tweaks.

…me of the examples

I am not sure about the organization of the initial part of the document, to be honest. For example the way it promises examples before options, but before the examples sections it gives examples (on using stdin).

Anyway, here are the most significant changes I made: 
- Some in the examples
- Options are reorganized to match the flac -h order 
- I used _underscores_ for italics, avoiding some formatting issue that arose from triple asterisks 
- Some " " inserted - you will all too easily need them when using -T - but I dared not use too many of them. 

... and some more text tweaks.
@ktmf01
Copy link
Collaborator

ktmf01 commented Dec 3, 2024

Thanks! I'll take a look.

man/flac.md Outdated Show resolved Hide resolved
man/flac.md Outdated Show resolved Hide resolved
man/flac.md Outdated Show resolved Hide resolved
man/flac.md Show resolved Hide resolved
@ktmf01
Copy link
Collaborator

ktmf01 commented Dec 8, 2024

Thanks! Looks good. Please consider my comments.

One more thing:

* I used _underscores_ for italics, avoiding some formatting issue that arose from triple asterisks

So, previously, it didn't render correctly in github but did render correctly in pandoc (html and man page). With this change, it renders correctly in github but not in pandoc. I think pandoc is more important, so please revert this change.

@H2Swine
Copy link
Contributor Author

H2Swine commented Dec 9, 2024

A few brief questions, reverse order, before I give it a go:

  • Underscores etc. Does pandoc honour spacing in between the asterisks? (Or anything escaped?) Edit: Maybe more important: Would those cause harm?
  • "default": it was intentional to indicate. Is it the too-terse "(default!)" you have issues with, so that one could write it out to e.g. "(This option on by default.)"?
  • I have no strong feelings for "concatenated". Unless you think "joined" is better (describing what you do, "shortened" describes why) I'm going for "shortened".

@ktmf01
Copy link
Collaborator

ktmf01 commented Dec 9, 2024

Underscores etc. Does pandoc honour spacing in between the asterisks? (Or anything escaped?) Edit: Maybe more important: Would those cause harm?

Sorry, I don't understand your question.

"default": it was intentional to indicate. Is it the too-terse "(default!)" you have issues with, so that one could write it out to e.g. "(This option on by default.)"?

I think it is too terse, yes. I'd prefer (This option is on by default). I'd say, in a man page there is room enough to not be overly concise.

I have no strong feelings for "concatenated". Unless you think "joined" is better (describing what you do, "shortened" describes why) I'm going for "shortened".

Then shortened it is!

@H2Swine
Copy link
Contributor Author

H2Swine commented Dec 9, 2024

EDIT:

Underscores etc. Does pandoc honour spacing in between the asterisks? (Or anything escaped?) Edit: Maybe more important: Would those cause harm?

Sorry, I don't understand your question.

What I was thinking of would output e.g. --blocksize= 1024 and that is very bad, as the encoder objects to the space. Hence this edit.

Looking further into how to resolve it: the document is not consistent. Some instances it uses capital letters to describe what user must fill in, others do not:
--cuesheet=filename
--picture={FILENAME|SPECIFICATION}

So even when sorting out a policy for CAPITALS or not, we might think of one out of two-and-a-half:

  • Triple asterisks that look bad in github.
  • Drop the italics, and make choices on how to have it typeset: rather than --output-prefix=string , the we would have the choice between **--output-prefix=**string and **--output-prefix=**STRING .
  • Or maybe just drop some of the italics, where it doesn't do much anyway: --blocksize=# vs --blocksize=# , does it matter that the latter # is italics? And when there are braces, like --endian={big|little} , it is anyway given that it isn't supposed to be copied literally.

H2Swine added a commit to H2Swine/flac that referenced this pull request Dec 10, 2024
Following xiph#769 , and also: 
- Re-wrote the section on apodization functions
- Search-replaced dot space space by dot space
@H2Swine
Copy link
Contributor Author

H2Swine commented Dec 10, 2024

I did a new attempt locally at H2Swine@7125fa9 , but before I figure out how to update a PR with a new file (or should one open a new?), you can rather look at it to strike down whatever proposed changes are not that good. That in particular goes for the attempts at fixing the previous reply in this thread.

There are a couple of more changes proposed.

  • You see several lines changed by replacing double spacing between sentences (dot space space by dot space). Of course, the opposite could be done, but I went by majority vote.
  • Then I made an attempt to rewrite the apodization function section, that obviously had had "text added over time" rather than trying to write it as one section. Not sure if that text is good enough, but content-wise: I honestly think you did such a good job on subdivide_tukey that once the reader is past (1) this is what the option is about, and (2) if you want to try, these are your options, then go to (3) if you want to try heavier than -8, this is the first suggestion. Then afterwards explain what more can be fiddled around with. (You agree with my gut feeling that partial_tukey and subdivide_tukey are likely obsoleted?)
    -- I said something about escaping parentheses ... that is not solely for -A. But anyway, since subdivide_tukey uses parentheses, it could be said up there.
    -- You also see that I went all-out "use scientific notation" in that section. Two justifications: It is The Right Thing To Do to avoid locale-dependent interpretation, and: since it is now is at the end, after a recommended "power users' baby step: n", it shouldn't intimidate many who would want to try it.

Then not yet done:

  • I saw afterwards that linebreak 471 isn't good, but it isn't the final draft anyway. Also ... hm, linebreaks in .md? I've tried to stick to the seventysomething character count.
  • Raw options: yeah, channels interleaved - but in what order? I think that information should be there, but I haven't tried to look it up.
  • The overlap parameter ov in partial/punchout tukey: between -1 and 1, is that right? It does accept other values though, but "should be" is not as strong as saying "will only accept".
  • There is no longer the text that partial/punchout tukey from 2 to 6 was a "sane" range. Something like that could of course be added in - for subdivide_tukey, of course - but maybe then one could mention not only the diminishing return, but that at some stage it gets more expensive than -p.

@ktmf01
Copy link
Collaborator

ktmf01 commented Dec 13, 2024

So even when sorting out a policy for CAPITALS or not, we might think of one out of two-and-a-half:

* Triple asterisks that look bad in github.

* Drop the italics, and make choices on how to have it typeset: rather than **--output-prefix=**_string_ , the we would have the choice between **--output-prefix=**string and **--output-prefix=**STRING .

* Or maybe just drop some of the italics, where it doesn't do much anyway: **--blocksize=**# vs **--blocksize=**_#_ , does it matter that the latter _#_ is italics? And when there are braces, like --endian={big|little} , it is anyway given that it isn't supposed to be copied literally.

Lets get back to the beginning: this was and is, first and foremost, a manpage. It is a *nix concept that you might be familiar with. I'd like to stick with the convention there. The convention seems to be that everything a user has to change (i.e., that is not to be taken literal) is underlined and ALLCAPS. Now, markdown doesn't support underlining in any way, and manpages use a monospace font which doesn't support italics, so the underlined parts were converted to italics in the markdown/html.

Proposal: Don't apply any markup to symbols. So, **\--output-name=***filename* is converted to **\--output-name**=*filename*. That seems to work: --output-name=filename

but before I figure out how to update a PR with a new file (or should one open a new?), you can rather look at it to strike down whatever proposed changes are not that good.

Just add another commit to the branch that belongs to this pull request.

There are a couple of more changes proposed.

* You see several lines changed by replacing double spacing between sentences (dot space space by dot space). Of course, the opposite could be done, but I went by majority vote.

Yeah, I don't know where these came from, I think replacing with single spaces is fine.

* Then I made an attempt to rewrite the apodization function section, that obviously had had "text added over time" rather than trying to write it as one section. Not sure if that text is good enough, but content-wise: I honestly think you did such a good job on subdivide_tukey that once the reader is past (1) _this is what the option is about_, and (2) _if you want to try, these are your options_, then go to (3) _if you want to try heavier than -8, this is the first suggestion_. Then afterwards explain what more can be fiddled around with. (You agree with my gut feeling that partial_tukey and subdivide_tukey are likely obsoleted?)
  -- I said something about escaping parentheses ... that is not solely for -A. But anyway, since subdivide_tukey uses parentheses, it could be said up there.
  -- You also see that I went all-out "use scientific notation" in that section. Two justifications: It is The Right Thing To Do to avoid locale-dependent interpretation, and: since it is now is at the end, after a recommended "power users' baby step: n", it shouldn't intimidate many who would want to try it.

Thanks

Then not yet done:

* I saw afterwards that linebreak 471 isn't good, but it isn't the final draft anyway. Also ... hm, linebreaks in .md? I've tried to stick to the seventysomething character count.

* Raw options: yeah, channels interleaved - _but in what order?_ I think that information should be there, but I haven't tried to look it up.

In the order you want them to end up in the FLAC file. First channel first, second channel second etc. I don't think not adding this is a big deal, this is specific enough that people can just try and see what happens a few times if they don't know where to find the format specification. Raw input is messy anyway.

* The overlap parameter ov in partial/punchout tukey: between -1 and 1, is that right? It does accept other values though, but "should be" is not as strong as saying "will only accept".

No, between negative infinity and 1. I don't think it is necessary to document however, as I am not sure it is very useful. However, a negative overlap gives very small windows with gaps between them. So, when you specify partial_tukey(2/0.5) you get 2 windows that overlap somewhat: first goes from 0 to 66% while the second goes from 33% to 100%. At overlap 1, you get two windows that are the same. At overlap -3, the first window goes from 0% to 20%, while the second goes from 80% to 100%, so there's a gap in the middle. I am unsure whether it actually always works as intended.

* There is no longer the text that partial/punchout tukey from 2 to 6 was a "sane" range. Something like that could of course be added in - for subdivide_tukey, of course - but _maybe_ then one could mention not only the diminishing return, but that at some stage it gets more expensive than -p.

I don't know, experimenting beyond the presets is really that, experimenting. I'm not sure there should be guidelines on that in the manpage. When someone gets to this point in the manual, everything already works anyway, it is not some feature that can be enabled or disabled or something, it is just that maybe, perhaps, a tiny bit smaller file can be generated.

@H2Swine
Copy link
Contributor Author

H2Swine commented Dec 13, 2024

I'll have a look at the following shortly (object if I have misunderstood):

The convention seems to be that everything a user has to change (i.e., that is not to be taken literal) is underlined and ALLCAPS. Now, markdown doesn't support underlining in any way, and manpages use a monospace font which doesn't support italics, so the underlined parts were converted to italics in the markdown/html.

Proposal: Don't apply any markup to symbols. So, **\--output-name=***filename* is converted to **\--output-name**=*filename*. That seems to work: --output-name=filename

I am going to presume it shall be in all caps,
--output-name=FILENAME

  • Raw options: yeah, channels interleaved - but in what order? I think that information should be there, but I haven't tried to look it up.

In the order you want them to end up in the FLAC file. First channel first, second channel second etc. I don't think not adding this is a big deal, this is specific enough that people can just try and see what happens a few times if they don't know where to find the format specification.

This is the comprehensive help, so I am putting in a reference to the format specification there. I assume it is with the order as in the format spec section 9.1.3 table 16 - for example, the first one in the stream will go to "what the FLAC format by default (absent channel mask tag) thinks is" FL, the second to FR, and the third to FC except if channel count is 4?

I don't know, experimenting beyond the presets is really that, experimenting. I'm not sure there should be guidelines on that in the manpage.

I take that to mean that it is fine to remove the wording about partial/punchout 2 to 6, as it could look like such a recommendation. I agree we should be careful not to present "more functions as any bang for the buck", but it may still be a point that subdivide_tukey is developed to recycle calculations for more windows cheaper. I'll chew on a formulation that you can reject.

@H2Swine
Copy link
Contributor Author

H2Swine commented Dec 13, 2024

Edit:
When it comes to characters in the help file:
When is the equals sign mandatory? Say, --threads 4 works just as well as --threads=4 .

And this sentence ... is there supposed to be anything more comprehensive at all? Delete it?
"A summary of options is included below. For a complete description, see
the HTML documentation."

Following discussion up to xiph#769 up to replies 12-13.
@H2Swine
Copy link
Contributor Author

H2Swine commented Dec 14, 2024

I had another shot at it (did I commit it right between this comment and the previous?! Edit: Right now I discovered the "Convert to draft" option, which maybe would be the right thing for all that I know - I hope I didn't mess up anything by merely requesting review).

There are a few things I have not touched, not knowing completely what should be done to them all:

  • The items in the previous thirteenth reply: Is this supposed to take over for the HTML documentation? Cf. the text:
    "A summary of options is included below. For a complete description, see
    the HTML documentation."

  • Under ReplayGain specs, I find the following, which IMHO goes for the full man page. I don't think it has any business being there so far down in the document: either it is needed, and then move it up - or it isn't and can be removed?
    The <specification> is a shorthand notation for describing how to apply
    ReplayGain. All components are optional but order is important. '[]'
    means 'optional'. '|' means 'or'. '{}' means required. The format is:

  • Brings me to: I have not started on the early part, which I think could need some more thought. Yes likely it should say something about how piping works and why -o etc are more preferred than redirects. And maybe about the previous item, explaining what brackets and braces and bars mean in the man page. And maybe about whether/when the "=" is compulsory (cf. previous comment). And likely about the relationship between single-dash and double-dash options.

  • Did very little about --apply-replaygain-which-is-not-lossless and the Picture spec, I haven't used those much, but: Do I remember right that using URL in PICTURE is discouraged nowadays?

Edit2, forgot:

  • For --skip and until, I did change the minutes:seconds to capitals, and gave example with decimals; I think it was unfortunate to suggest that fractions of a second was mandatory. But then ... I do think MM:SS is unfortunate too in that it suggests it is a compulsory two digits : two digits format. Surely there could be 123:4.567 (presuming dot as decimal point). So maybe it is better to write it out as MINUTES:SECONDS or shortened to MIN:SEC ?

@H2Swine H2Swine requested a review from ktmf01 December 14, 2024 17:53
@ktmf01
Copy link
Collaborator

ktmf01 commented Dec 17, 2024

I am going to presume it shall be in all caps, --output-name=FILENAME

Yes

I assume it is with the order as in the format spec section 9.1.3 table 16 - for example, the first one in the stream will go to "what the FLAC format by default (absent channel mask tag) thinks is" FL, the second to FR, and the third to FC except if channel count is 4?

I think so yes. As far as I know flac doesn't reorder channels, ever.

I take that to mean that it is fine to remove the wording about partial/punchout 2 to 6

Yes

Edit: When it comes to characters in the help file: When is the equals sign mandatory? Say, --threads 4 works just as well as --threads=4 .

Okay. This is behaviour of getopt, I don't know all the nooks and crannies of it. I suppose the equals sign isn't necessary then. Still, probably better not to mention it, or it gets even more confusing.

And this sentence ... is there supposed to be anything more comprehensive at all? Delete it? "A summary of options is included below. For a complete description, see the HTML documentation."

You can remove that. Can you imagine, flac had 5 layers of docs: short help, help, explain, man page and HTML docs. I merged the HTML docs and man page in commit #80d064e but apparently I forgot this bit.

I had another shot at it (did I commit it right between this comment and the previous?! Edit: Right now I discovered the "Convert to draft" option, which maybe would be the right thing for all that I know - I hope I didn't mess up anything by merely requesting review).

This seems to have happened exactly as it should be. Not using the 'draft PR' option is also the proper way, because on draft PRs, reviewing is impeded/blocked. I only use the draft PR option to signal that nobody should care to look at it yet, which isn't the case here.

Under ReplayGain specs, I find the following, which IMHO goes for the full man page. I don't think it has any business being there so far down in the document: either it is needed, and then move it up - or it isn't and can be removed?
The is a shorthand notation for describing how to apply
ReplayGain. All components are optional but order is important. '[]'
means 'optional'. '|' means 'or'. '{}' means required. The format is:

You could move it up, but people don't read such a document top-to-bottom. At least, I don't. I agree that it applies to the whole man page, but similar specs elsewhere aren't as complicated. Also, the pipe symbol is literal in the picture specification, whereas it means or elsewhere in the document.

Brings me to: I have not started on the early part, which I think could need some more thought. Yes likely it should say something about how piping works and why -o etc are more preferred than redirects. And maybe about the previous item, explaining what brackets and braces and bars mean in the man page. And maybe about whether/when the "=" is compulsory (cf. previous comment). And likely about the relationship between single-dash and double-dash options.

I think it is wise to merge this as soon as there are no loose ends, and not wait for further improvements. It is better to have several smaller commits than one big commit, as long as the commit is 'self-contained'.

Did very little about --apply-replaygain-which-is-not-lossless and the Picture spec, I haven't used those much, but: Do I remember right that using URL in PICTURE is discouraged nowadays?
I don't know how widely supported such URLs are.

For --skip and until, I did change the minutes:seconds to capitals, and gave example with decimals; I think it was unfortunate to suggest that fractions of a second was mandatory. But then ... I do think MM:SS is unfortunate too in that it suggests it is a compulsory two digits : two digits format. Surely there could be 123:4.567 (presuming dot as decimal point). So maybe it is better to write it out as MINUTES:SECONDS or shortened to MIN:SEC ?

MM:SS is a rather standard way of writing I'd say. I'm not sure that needs any clarification.

man/flac.md Outdated Show resolved Hide resolved
@H2Swine
Copy link
Contributor Author

H2Swine commented Dec 17, 2024

New one in a minute, hoping I'm about to converge on what I've actually touched upon. Will describe changes relative to previous one here.

I think it is wise to merge this as soon as there are no loose ends, and not wait for further improvements. It is better to have several smaller commits than one big commit, as long as the commit is 'self-contained'.

I agree. Although I did a little bit on lines 44 to (new) 56, but any changes between that and the EXAMPLES section can be ... for later.

EXAMPLES:

  • Subsections for encoding examples and decoding examples.
  • Reworded the recompression example. Better than saying it is tricky, start by saying what it does.
  • nuked the "when when" you found.
  • New "last encoding example". I don't think it is a "common" task for an end-user to add tags by hand, so I don't want to keep focus on it.
    The explanation is maybe elaborate on how options can be written, but maybe that part can be removed when one is done with the part above EXAMPLES.
  • The -d -F: Here is a suggestion to be careful about it, and also - given what we know about old software throwing wrong data at libflac - to try different decoders and keep the offending files.
    Not sure if it is a good wording.

OPTIONS:

First this removal I did:

You can remove that. Can you imagine, flac had 5 layers of docs: short help, help, explain, man page and HTML docs. I merged the HTML docs and man page in commit #80d064e but apparently I forgot this bit.

As an aside, I actually suggest to consider: -H should not give an error, rather a short sentence that says "For long help, use man page / HTML doc / [URL]."
In case someone uses it, they better be told what to use instead. (In case nobody uses that one out of the five there were: no harm done.)

  • For the -0 to -8, I inserted a "Currently" synonymous in the previous version, but for some strange reason I left out doing so for -4 and -5. Did it now.
  • The --skip: Now clearer I think, what it must be.
  • The --picture: Italicized some ALLCAPS and minor rewording.
  • ReplayGain subsection: Removed the description on brackets and pipe (and also on "="!). I did not reinsert it further up; in addition to what you wrote about Picture spec, I see that description of --cue, --skip etc have everything in brackets, so there is no use in writing outright that it is all optional when there has to be at least one. Better not write anything wrong.
  • Picture spec subsection: Italicized many ALLCAPS. Hope it comes out reasonable.
  • Apodization: Minor rewording, reflecting that I am not completely happy with it. But I guess it is good enough.

@H2Swine H2Swine requested a review from ktmf01 December 17, 2024 18:29
@ktmf01
Copy link
Collaborator

ktmf01 commented Dec 18, 2024

I'd like to merge this now, if you are OK with that.

One n replaced by *N*
@H2Swine
Copy link
Contributor Author

H2Swine commented Dec 18, 2024

Oh, sorry for one little detail I overlooked: at a glance and scroll, I found one single "n" shining at my eyes, that now is CapItalic-ized.

Apart from that, I know that the .md file's linebreaks are not at consistent character width, but from your changes today I am assuming you have full control over what is needed, so ... merge if/when you want to.

Afterwards I'll scratch my head a little for possible changes to the "GENERAL USAGE". (That is not to say I will actually come up with any improvement.)

@ktmf01 ktmf01 merged commit 163e4a7 into xiph:master Dec 18, 2024
16 checks passed
@H2Swine H2Swine deleted the H2Swine-patch-flac.md branch December 18, 2024 15:23
@H2Swine H2Swine restored the H2Swine-patch-flac.md branch December 18, 2024 15:23
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