Help messages correctly formatted (79 chars) #5749

Help messages are formatted programmatically with FormatParagraph in order not to break existing strings in Transifex (hint in #5734).

The new format should work even if the translation of the strings modifies the lenght of the message.

There will a war about the width of the terminal... 80, 132, ...

Concept ACK

Indeed, 79 or 80 are the only reasonable const widths I think. I suggest using COLUMNS environment var when defined, detecting it, and finally failing back to unformatted.

Travis fails with:

init.cpp:323:17: error: expected ‘)’ before ‘_’

OK - let's behave like adults here and NOT have an argument about what width to use. The code is general enough to work with any width. Let's first get it right, then later (as in, in a later pull) it can be improved to e.g. detect the screen width.

I also see this as a improvement. Reading COLUMNS would be nice but feels like a luxury version. The screen size probably should be 80. IMO it's less painful running a 80 output on a large terminal then vice versa.

conceptual ACK.

@paveljanik @luke-jr @laanwj @jonasschnelli

I looked at well known projects to better understand what is the standard. I found that 79 chars is considered the standard. And it does not change if COLUMNS changes, nor if xterm changes. Our problem is that the descriptions are long and do not fit in the screen together with the option. Thus I will put the description on the next line of the option. An example follows.

Bitcoin Core Daemon version v0.10.99.0-994fa8e-dirty

Usage:
  bitcoind [options]                     Start Bitcoin Core Daemon

Options:

  -?                     
       This help message

  -alertnotify=<cmd>     
       Execute command when a relevant alert is received or we see a really
       long fork (%s in cmd is replaced by message)

  -blocknotify=<cmd>     
       Execute command when the best block changes (%s in cmd is replaced by
       block hash)

  -checkblocks=<n>       
       How many blocks to check at startup (default: 288, 0 = all)

  -checklevel=<n>        
       How thorough the block verification of -checkblocks is (0-4, default: 3)

  -conf=<file>           
       Specify configuration file (default: bitcoin.conf)

  -daemon                
       Run in the background as a daemon and accept commands

  -datadir=<dir>         
       Specify data directory

  -dbcache=<n>           
       Set database cache size in megabytes (4 to 4096, default: 100)

  -loadblock=<file>      
       Imports blocks from external blk000??.dat file on startup

  -maxorphantx=<n>       
       Keep at most <n> unconnectable transactions in memory (default: 100)

  -par=<n>               
       Set the number of script verification threads (-4 to 16, 0 = auto, <0 =
       leave that many cores free, default: 0)

  -pid=<file>            
       Specify pid file (default: bitcoind.pid)

  -reindex               
       Rebuild block chain index from current blk000??.dat files on startup

  -sysperms              
       Create new files with system default permissions, instead of umask 077
       (only effective with disabled wallet functionality)

  -txindex               
       Maintain a full transaction index, used by the getrawtransaction rpc
       call (default: 0)

Connection options:

  -addnode=<ip>          
       Add a node to connect to and attempt to keep the connection open

  -banscore=<n>          
       Threshold for disconnecting misbehaving peers (default: 100)

  -bantime=<n>           
       Number of seconds to keep misbehaving peers from reconnecting (default:
       86400)

  -bind=<addr>           
       Bind to given address and always listen on it. Use [host]:port notation
       for IPv6

  -connect=<ip>          
       Connect only to the specified node(s)

  -discover              
       Discover own IP addresses (default: 1 when listening and no -externalip
       or -proxy)

  -dns                   
       Allow DNS lookups for -addnode, -seednode and -connect (default: 1)

  -dnsseed               
       Query for peer addresses via DNS lookup, if low on addresses (default: 1
       unless -connect)

  -externalip=<ip>       
       Specify your own public address

  -forcednsseed          
       Always query for peer addresses via DNS lookup (default: 0)

  -listen                
       Accept connections from outside (default: 1 if no -proxy or -connect)

  -maxconnections=<n>    
       Maintain at most <n> connections to peers (default: 125)

  -maxreceivebuffer=<n>  
       Maximum per-connection receive buffer, <n>*1000 bytes (default: 5000)

  -maxsendbuffer=<n>     
       Maximum per-connection send buffer, <n>*1000 bytes (default: 1000)

  -onion=<ip:port>       
       Use separate SOCKS5 proxy to reach peers via Tor hidden services
       (default: -proxy)

  -onlynet=<net>         
       Only connect to nodes in network <net> (ipv4, ipv6 or onion)

...

@lucayepa Thanks for investigation. Looks good!

Just tested this on OSX 10.10.3 and Windows 7. It doesn't look optimized on a 79x30 terminal.

Binaries to test are here: https://builds.jonasschnelli.ch/pulls/5749/

See screenshot:

@jonasschnelli IIUIC, "Thus I will put the description on the next line of the option." is not yet implemented.

New style of options help looks good to me.

The screenshot shows a different situation from the code. If you're working on reworking this and it shouldn't be merged as-is, can you please close the pull in the mean time?

@jonasschnelli @laanwj @paveljanik In fact it was still not implemented. Now it is. Please test it with Windows and OsX.

Please squash the commits to one so it can be compared in the Github UI to the master code.

I like this approach.

Can we get rid of the spaces in the option description? E.g.:

strUsage += HelpMessageOpt("delin=N                ", _("Delete input N from TX"));

The spaces after delin=N can probably be deleted (and the formatting function fixed accordingly). There are lot of them in the current code and this way, we can save some binary space/memory (a little bit though ;-).

In arguments to HelpMessageGroup:

strUsage = HelpMessageGroup(_("Register Commands:"));

can you delete double colons and add it back in the formatting function itself? It will touch the translation string, but that can be easily fixed in any decent localization tool (fuzzy matching).

@paveljanik I removed the spaces. The idea is not to touch the translation, thus I did not touch the ':'. Maybe there should be a policy about when to touch the translation (at the start of a new version, or something like that).

@paveljanik I squashed the 6 commits in a single one.

Screenshot looks very good to me.

The text "Start Bitcoin Core Daemon" looks a bit strange there

Meh. It is consistent with bitcoin-cli's output.

  bitcoin-cli [options] <command> [params]  Send command to Bitcoin Core
  bitcoin-cli [options] help                List commands
  bitcoin-cli [options] help <command>      Get help for a command

Except that bitcoind happens to have only one invocation mode.

Looks like this breaks bitcoin-qt --help at the moment:

(5749)orion@amethyst:~/projects/bitcoin/bitcoin$ src/qt/bitcoin-qt --help
(no output)

Also when selecting "command line options" in the help menu, there is a clear distinction between UI options and the core options: Now, the core options are both in their own column and on a new line, thats a bit overkill :-)

This is how it currently looks on OSX, and bitcoin-qt --help is indeed broken.

@fanquake that doesn't look like the most recent version of this code, option and text are still on the same line there

@laanwj Right, forgot the merge script doesn't actually build after merging. Change look fine now. Although there's still the issue with bitcoin-qt.

@laanwj @fanquake @zander The issue about bitcoin-qt not displaying the help message, seems to be there since commit e179eb3d9bfec7e67908242c71c87b716a41c97c.

If you agree, I think that the right workflow should be to revert e179eb3d9bfec7e67908242c71c87b716a41c97c, if possible, and then I will rebase my commit and try to fix the qt help message.

Then I think it should be better to use only the function HelpMessage in src/init.cpp, since HelpMessageDialog::HelpMessageDialog already call HelpMessage. So I will try to delegate the output of the last part of the QT help message to HelpMessage. But first solve the bug intorduced by e179eb3d9bfec7e67908242c71c87b716a41c97c.

wow, I only heard now for the first time my commit caused a regressions. Not nice to wake up to the suggestion to get it reverted. Please give me a chance to understand the issue before you revert anything.

@lucayepa to say that my commit (e179eb3) introduced a bug is not really true. It does the best it can with unstructured data. With your change in the unstructured data it no longer works.

Maybe the best solution is to make the data actually structured. Which means that instead of building a large string in the core and then using heuristics to split stuff again in the GUI (so it can be put in a table), we both work on structured data.

My suggestion; change HelpMessageCli() to return something like; typedef std::pair<std::string std::string> argPair; std::vector<argPair> HelpMessageCli();

then instead of calling HelpMessageOpt inside the HelpMessageCli() method, you move it to the place where HelpMessageCli() is called.

This removes the magic heuristics requirement in the GUI and would make it trivial to fix (I can help) the GUI dialog.

@zander I'll check your solution, but, in order to be sure that we are on the same page, please build e179eb3d9bfec7e67908242c71c87b716a41c97c, then write on the command line:

bitcoin-qt --help

Then build e5153095ea410dd07770c0327447856488bfd137 and do the same.

The bug is not about structured data, but about the fact that there is no output at all.

Well in any case if that was already broken beforehand, it is not the responsibility of this pull to fix it. Seemingly no one tested bitcoin-qt --help in the mean time.

So the only thing here that remains to be fixed is the help message dialog, which now shows the output in two styles.

Maybe it is better to declare the new functions in utilstrencodings.h instead of util.h? That way policy can use them without depending on util.o (see #5595). @luke-jr @theuni thoughts? Beyond that little nit, ut ACK.

@jtimon util.cpp seems the appropriate place due to their link with other option management functions which are there too. Is there any reason you need to use these functions and not Get*Arg?

@jtimon @laanwj @luke-jr @theuni As suggested, I left the new functions in util.h @zander @laanwj @fanquake I resolved the bug from e179eb3d9bfec7e67908242c71c87b716a41c97c . Now "bitcoin-qt -help" returns the correct output. @paveljanik @jonasschnelli I put all the options in a single place (init.cpp). There were already other options qt-related in init.cpp. Having all the options in a single place seems far more clean. This resolve the wrong format for UI options too. As in e179eb3d9bfec7e67908242c71c87b716a41c97c, the QT help message does not have a scroll bar.

rebased