Help messages correctly formatted (79 chars)

lucayepa commented at 8:17 am on February 4, 2015: contributor

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.

paveljanik commented at 8:22 am on February 4, 2015: contributor

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

laanwj commented at 8:29 am on February 4, 2015: member

Concept ACK

in src/bitcoin-cli.cpp: in 6462a132d6 outdated

18@@ -19,22 +19,27 @@ using namespace json_spirit;
19 
20 std::string HelpMessageCli()
21 {
22+#define SCREEN_SIZE 132
23+#define INDENT 25
24+#define MESSAGE_SIZE SCREEN_SIZE - INDENT
25+#define OPT(option, message) option + FormatParagraph(message, MESSAGE_SIZE, INDENT) + "\n"

laanwj commented at 8:32 am on February 4, 2015:

Let’s use type-safe constants and inline functions. I’d like to avoid C-style macros unless there is no other choice. e.g.

0static const int screenWidth = 132;
1....
2static inline std::string opt(const std::string &option, const std::string &message)
3{
4    return option + FormatParagraph(...) + "\n";
5}

luke-jr commented at 8:36 am on February 4, 2015: member

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.

paveljanik commented at 8:38 am on February 4, 2015: contributor

Travis fails with:

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

laanwj commented at 8:43 am on February 4, 2015: member

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.

jonasschnelli commented at 8:47 am on February 4, 2015: contributor

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.

lucayepa commented at 8:25 am on February 8, 2015: contributor

@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.

  0Bitcoin Core Daemon version v0.10.99.0-994fa8e-dirty
  1
  2Usage:
  3  bitcoind [options]                     Start Bitcoin Core Daemon
  4
  5Options:
  6
  7  -?                     
  8       This help message
  9
 10  -alertnotify=<cmd>     
 11       Execute command when a relevant alert is received or we see a really
 12       long fork (%s in cmd is replaced by message)
 13
 14  -blocknotify=<cmd>     
 15       Execute command when the best block changes (%s in cmd is replaced by
 16       block hash)
 17
 18  -checkblocks=<n>       
 19       How many blocks to check at startup (default: 288, 0 = all)
 20
 21  -checklevel=<n>        
 22       How thorough the block verification of -checkblocks is (0-4, default: 3)
 23
 24  -conf=<file>           
 25       Specify configuration file (default: bitcoin.conf)
 26
 27  -daemon                
 28       Run in the background as a daemon and accept commands
 29
 30  -datadir=<dir>         
 31       Specify data directory
 32
 33  -dbcache=<n>           
 34       Set database cache size in megabytes (4 to 4096, default: 100)
 35
 36  -loadblock=<file>      
 37       Imports blocks from external blk000??.dat file on startup
 38
 39  -maxorphantx=<n>       
 40       Keep at most <n> unconnectable transactions in memory (default: 100)
 41
 42  -par=<n>               
 43       Set the number of script verification threads (-4 to 16, 0 = auto, <0 =
 44       leave that many cores free, default: 0)
 45
 46  -pid=<file>            
 47       Specify pid file (default: bitcoind.pid)
 48
 49  -reindex               
 50       Rebuild block chain index from current blk000??.dat files on startup
 51
 52  -sysperms              
 53       Create new files with system default permissions, instead of umask 077
 54       (only effective with disabled wallet functionality)
 55
 56  -txindex               
 57       Maintain a full transaction index, used by the getrawtransaction rpc
 58       call (default: 0)
 59
 60Connection options:
 61
 62  -addnode=<ip>          
 63       Add a node to connect to and attempt to keep the connection open
 64
 65  -banscore=<n>          
 66       Threshold for disconnecting misbehaving peers (default: 100)
 67
 68  -bantime=<n>           
 69       Number of seconds to keep misbehaving peers from reconnecting (default:
 70       86400)
 71
 72  -bind=<addr>           
 73       Bind to given address and always listen on it. Use [host]:port notation
 74       for IPv6
 75
 76  -connect=<ip>          
 77       Connect only to the specified node(s)
 78
 79  -discover              
 80       Discover own IP addresses (default: 1 when listening and no -externalip
 81       or -proxy)
 82
 83  -dns                   
 84       Allow DNS lookups for -addnode, -seednode and -connect (default: 1)
 85
 86  -dnsseed               
 87       Query for peer addresses via DNS lookup, if low on addresses (default: 1
 88       unless -connect)
 89
 90  -externalip=<ip>       
 91       Specify your own public address
 92
 93  -forcednsseed          
 94       Always query for peer addresses via DNS lookup (default: 0)
 95
 96  -listen                
 97       Accept connections from outside (default: 1 if no -proxy or -connect)
 98
 99  -maxconnections=<n>    
100       Maintain at most <n> connections to peers (default: 125)
101
102  -maxreceivebuffer=<n>  
103       Maximum per-connection receive buffer, <n>*1000 bytes (default: 5000)
104
105  -maxsendbuffer=<n>     
106       Maximum per-connection send buffer, <n>*1000 bytes (default: 1000)
107
108  -onion=<ip:port>       
109       Use separate SOCKS5 proxy to reach peers via Tor hidden services
110       (default: -proxy)
111
112  -onlynet=<net>         
113       Only connect to nodes in network <net> (ipv4, ipv6 or onion)
114
115...

paveljanik commented at 8:27 am on February 8, 2015: contributor

@lucayepa Thanks for investigation. Looks good!

jonasschnelli commented at 9:17 am on February 8, 2015: contributor

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: bildschirmfoto 2015-02-08 um 10 11 48

bildschirmfoto 2015-02-08 um 10 15 49

paveljanik commented at 9:27 am on February 8, 2015: contributor

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

laanwj commented at 1:05 pm on February 9, 2015: member

New style of options help looks good to me.

laanwj commented at 11:25 am on February 10, 2015: member

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?

lucayepa commented at 5:01 am on February 15, 2015: contributor

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

paveljanik commented at 8:49 am on February 15, 2015: contributor

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.:

0strUsage += 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:

0strUsage = 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).

lucayepa renamed this:
~~Help messages correctly formatted for SVGA text mode (132 chars)~~
Help messages correctly formatted (79 chars)
on Feb 15, 2015

lucayepa commented at 7:57 pm on February 15, 2015: contributor

@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).

lucayepa force-pushed on Feb 15, 2015

lucayepa commented at 8:37 pm on February 15, 2015: contributor

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

laanwj commented at 12:38 pm on February 16, 2015: member

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.

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

Except that bitcoind happens to have only one invocation mode.

laanwj commented at 12:53 pm on February 16, 2015: member

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

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

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

fanquake commented at 12:56 pm on February 16, 2015: member

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

laanwj commented at 12:59 pm on February 16, 2015: member

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

fanquake commented at 1:17 pm on February 16, 2015: member

@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. screen shot 1

lucayepa commented at 5:11 am on February 17, 2015: contributor

@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.

zander commented at 6:35 am on February 17, 2015: none

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.

zander commented at 6:54 am on February 17, 2015: none

@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.

lucayepa commented at 7:32 am on February 17, 2015: contributor

@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:

0bitcoin-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.

laanwj added the label Improvement on Feb 19, 2015

laanwj commented at 10:51 am on February 19, 2015: member

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.

jtimon commented at 7:38 pm on February 23, 2015: contributor

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.

laanwj commented at 2:40 pm on March 6, 2015: member

@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?

lucayepa commented at 9:56 am on March 9, 2015: contributor

@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.

Help messages correctly formatted (79 chars)

Help messages are formatted programmatically with FormatParagraph
in order not to break existing strings in Transifex.

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

Sqashed 6 commits in a single one.
Help messages correctly formatted for SVGA text mode (132 chars)

Help messages are formatted programmatically with FormatParagraph
in order not to break existing strings in Transifex.

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

Fix - syntax error

Correct formatting for 79 chars

Correctly based on C++ functions

Removed spare spaces from option strings

Fix - syntax error

1fdb9fa3f9

Fix - bitcoin-qt usage message

. Closes the bug from commit e179eb3d9bfec7e67908242c71c87b716a41c97c
("bitcoin-qt -help" did not show any message)
. Move all the options in init.cpp (there were already some
options related to bitcoin-qt)

f75470794b

lucayepa force-pushed on Mar 11, 2015

lucayepa commented at 5:23 am on March 11, 2015: contributor

rebased

laanwj merged this on Mar 11, 2015

laanwj closed this on Mar 11, 2015

laanwj referenced this in commit d734d87b28 on Mar 11, 2015

MarcoFalke locked this on Sep 8, 2021

Help messages correctly formatted (79 chars) #5749