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 -h and man page as per jamuluswebsite/pull/714 #2448

Merged
merged 7 commits into from
Mar 24, 2022
Merged

Update -h and man page as per jamuluswebsite/pull/714 #2448

merged 7 commits into from
Mar 24, 2022

Conversation

gilgongo
Copy link
Member

@gilgongo gilgongo commented Mar 2, 2022

Short description of changes

Align wording and vocab with jamulussoftware/jamuluswebsite#714

CHANGELOG: Documentation: Made man page and help output more consistent.

Status of this Pull Request

I think -h is OK, but the man page is a bit of a rat's nest so needs some review.

Checklist

  • I've verified that this Pull Request follows the general code principles
  • I tested my code and it does what I want
  • My code follows the style guide
  • I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
  • I've filled all the content above

@gilgongo gilgongo added this to the Release 3.9.0 milestone Mar 2, 2022
@gilgongo gilgongo changed the title Update -f and main page as per jamuluswebsite/pull/714 Update -f and man page as per jamuluswebsite/pull/714 Mar 2, 2022
@pljones
Copy link
Collaborator

pljones commented Mar 2, 2022

The PR title says "-f"?

@gilgongo gilgongo changed the title Update -f and man page as per jamuluswebsite/pull/714 Update -h and man page as per jamuluswebsite/pull/714 Mar 2, 2022
@ann0see ann0see requested a review from pljones March 4, 2022 20:11
distributions/Jamulus.1 Outdated Show resolved Hide resolved
distributions/Jamulus.1 Outdated Show resolved Hide resolved
to the server, receiving back a mixed stream.
in Server mode, ideally on a dedicated Server (virtual) machine;
all participants start the (graphical) Client which transmits audio
to the Server, receiving back a mixed stream.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use of a metronome is recommended.

By whom?

Use of a metronome is recommended.
Clients should be connected using ethernet, not wireless, and use
proper headphone and microphone connections, not Bluetooth.
The server should run on a low-latency system, ideally not a VM.
The Server should run on a low-latency system, ideally not a VM.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ideally not a VM.

So why say "(virtual)" on line 63?

distributions/Jamulus.1 Outdated Show resolved Hide resolved
@@ -29,7 +29,7 @@
.Op Fl P | Fl \-delaypan
.Op Fl p | Fl \-port Ar number
.Op Fl Q | Fl \-qos Ar value
.Op Fl R | Fl \-recording Ar directory
.Op Fl R | Fl \-recording Ar Directory
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do these appear? Single dash for the short option, clearly a double dash (not a long dash) for the long options?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose this means I'm going to have to work out how to compile a man page locally... Sigh.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need, man will do it for you. Just do man distributions/Jamulus.1 to view it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fl x renders as -x (Fl means flag). GNU --long-options must be Fl \-long\-option (manpages are a BSD thing, GNU uses texinfo instead, so there’s no provision for GNU long options in manpages).

@softins man -l distributions/Jamulus.1 (-l = local) might be needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@softins man -l distributions/Jamulus.1 (-l = local) might be needed.

It wasn't needed on my Pi running Raspbian 11.

In fact looking at man man, I see that it automatically tries again with an implicit -l, if it can't match the supplied argument with an installed man page, such as when giving a pathname instead of a simple name and section.

distributions/Jamulus.1 Outdated Show resolved Hide resolved
distributions/Jamulus.1 Outdated Show resolved Hide resolved
.Pp
Running
.Nm
without any extra options launches the full graphical client.
without any extra options launches the full graphical Client.
.Pp
The options are as follows:
Copy link
Collaborator

@pljones pljones Mar 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The options are all a bit mixed up here.

In the --help output, they're now "Common" first, then "Server only", followed by "Client only". It might be easier reading if the man page were the same, using separate headings for each section.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last we spoke about this man page business, we hoped we'd automate it all out of -h (dropping all the stuff about recommending metromes and whatnot in the process) so that we don't manually maintain things in three places. It's only really there to satisfy Debian. Also doesn't help that I don't understand the syntax.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pljones options in manpages are supposed to be alphabetically sorted, case-insensitive (uppercase -Y before lowercase -y but after lowercase -x). Doing it a different way is very much not helpful.

@gilgongo manpages are supposed to be much more than just an option summary (you can get that from --help). It should also precisely explain how to use, concepts necessary, etc.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pljones options in manpages are supposed to be alphabetically sorted, case-insensitive (uppercase -Y before lowercase -y but after lowercase -x). Doing it a different way is very much not helpful.

They are alphabetical. They're just in separate sections that aren't related.

and the actual server are situated behind the same NAT, so that
clients can connect
.Pq Server mode only
configure public legacy IP address when both the Directory Server
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the word "legacy" here at all?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

“Legacy IP” is the contemporary wording for IPv4 (IP is IPv6). This was introduced into networking terminology a small number of years ago.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So that all us old timers feel old...

A private server is unlisted, clients can only connect if given
knows four modes of operation: Client mode and three kinds of Server
.Pq Unregistered, Registered, Directory.
A Unregistered Server is unlisted, Clients can only connect if given
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the address (IP address and port).

Not necessarily "IP address" - the host name is perfectly acceptable, if available. Port number is only necessary when it's not the default value. This can be covered here. Or link to the website for further discussion of "Server Address".

distributions/Jamulus.1 Outdated Show resolved Hide resolved
servers are categorised into genres.
A Registered Server will contact a Directory Server (whose address must be
given at Server startup) and show up in that Server's list; Clients
can retrieve a list of Registered Servers from the Directory Server.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Directory Server

And anywhere else.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's mostly mirabilos work?

Copy link
Member Author

@gilgongo gilgongo Mar 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is. He said he'd be able to help with formatting, but not content.

src/main.cpp Show resolved Hide resolved
@pljones
Copy link
Collaborator

pljones commented Mar 4, 2022

Yup, looks like the man page needs an overhaul. I am so glad I don't write in man page format any more. Despite it being essential.

@ann0see
Copy link
Member

ann0see commented Mar 5, 2022

@mirabilos just tagging you here since you wrote the man page and might want to comment

distributions/Jamulus.1 Outdated Show resolved Hide resolved
@@ -29,7 +29,7 @@
.Op Fl P | Fl \-delaypan
.Op Fl p | Fl \-port Ar number
.Op Fl Q | Fl \-qos Ar value
.Op Fl R | Fl \-recording Ar directory
.Op Fl R | Fl \-recording Ar Directory
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fl x renders as -x (Fl means flag). GNU --long-options must be Fl \-long\-option (manpages are a BSD thing, GNU uses texinfo instead, so there’s no provision for GNU long options in manpages).

@softins man -l distributions/Jamulus.1 (-l = local) might be needed.

distributions/Jamulus.1 Outdated Show resolved Hide resolved
all participants start the (graphical) client which transmits audio
to the server, receiving back a mixed stream.
in Server mode, ideally on a dedicated Server (virtual) machine;
all participants start the (graphical) Client which transmits audio
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lowercase

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong. The point of the PR is to capitalise "Server", "Client" and other relevant terms where referring to a running instance of Jamulus.

to the server, receiving back a mixed stream.
in Server mode, ideally on a dedicated Server (virtual) machine;
all participants start the (graphical) Client which transmits audio
to the Server, receiving back a mixed stream.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lowercase

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "Server" here is the Jamulus Server instance, not the machine it's running on.

.Pq server mode only
disconnect all clients on quit
.Pq Server mode only
disconnect all Clients on quit
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lowercase

make the server public and set its genre by setting the address
of the directory server (formerly central server) to use to
.Pq Server mode only
make the Server Registered and set its genre by setting the address
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lowercase

enable server list persistence, storing to and loading from
.Ar file
.Pq Directory Server mode only
remember registered Servers even if the Directory is restarted
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lowercase (also most of the others above here)…

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've completely missed the point.

and the actual server are situated behind the same NAT, so that
clients can connect
.Pq Server mode only
configure public legacy IP address when both the Directory Server
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

“Legacy IP” is the contemporary wording for IPv4 (IP is IPv6). This was introduced into networking terminology a small number of years ago.

.It Pa https://jamulus.io/wiki/Server\-Linux#running\-in\-public\-mode
current list of directory servers operated by the Jamulus project,
documentation on Server configuration and types
.It Pa https://jamulus.io/wiki/Server\-Linux#running\-in\-registered\-mode
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

404 Not Found

Don’t break the hyperlink.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It won't get released until the website is updated.

@mirabilos
Copy link
Contributor

mirabilos commented Mar 5, 2022 via email

@pljones
Copy link
Collaborator

pljones commented Mar 6, 2022

especially as it hurts searchability.

If you're searching, the ordering has no effect. "/string" will find "string" wherever it is. Contextualising the result, however, remains important - so having the section heads is important in the manual page.

Manual pages should be useful, not follow some pointless rules.

@ann0see
Copy link
Member

ann0see commented Mar 16, 2022

@gilgongo I think you should add JSON-RPC to the man page with this PR.

@gilgongo
Copy link
Member Author

@gilgongo I think you should add JSON-RPC to the man page with this PR.

If can work out the syntax.

@mirabilos
Copy link
Contributor

mirabilos commented Mar 16, 2022 via email

@ann0see
Copy link
Member

ann0see commented Mar 23, 2022

@gilgongo can this be merged soon?

@gilgongo
Copy link
Member Author

@ann0see once the JSON-RPC stuff gets added I think, I yes. I'll have another look at that.

@gilgongo
Copy link
Member Author

on second thoughts...

@mirabilos

Write the text and indicate to me what should be formatted how

--jsonrpcport
Enables JSON-RPC API server to control the app, set TCP port number (EXPERIMENTAL, APIs might change; only accessible from localhost). Please see the JSON-RPC API Documentation file.

--jsonrpcsecretfile
Required when using --jsonrpcport. Sets a path to a text file containing an authentication string for getting access to the JSON-RPC API.

@mirabilos
Copy link
Contributor

mirabilos commented Mar 24, 2022 via email

@gilgongo gilgongo merged commit 5609296 into jamulussoftware:master Mar 24, 2022
pgScorpio pushed a commit to pgScorpio/jamulus that referenced this pull request Mar 27, 2022
pgScorpio pushed a commit to pgScorpio/jamulus that referenced this pull request Mar 28, 2022
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.

5 participants