Add a progress bar for the Verification progress attribute in -getinfo when verification progress < 99%.
Motivation:
-getinfowatch -n 1 bitcoin-cli -getinfo(suggested by theStack below)< 99%)Reviewing
If your verification progress is > 99% you can restart the verification progress with
0$ ./src/bitcoind -reindex
1$./src/bitcoin-cli -getinfo
Also address #17314
Alternative, more human-friendly, format besides JSON? Colors, progress bars 😄?
Motivation: Improve user-friendliness of
-getinfo
Not sure if bitcoin-cli is dedicated to casual users.
Not sure if bitcoin-cli is dedicated to casual users. @hebasto thanks for the feedback! I can see where you are coming from but from #17314 I was under the impression that we want to make
bitcoin-climore friendly to casual users. Will be happy to close this PR if this additional feature is deemed unnecessary.
Now that getinfo is no longer part of the RPC API, and client-side implementation is not subject to API stability, it’s possible to start thinking about making it more user friendly.
905+
906+ for (int i = 0; i < (1 - progress) / INCREMENT; i++) {
907+ progress_bar += INCOMPLETE_BAR;
908+ }
909+
910+ progress_bar += "|";
0 std::string progress_bar = "";
1 static constexpr double INCREMENT{0.05};
2 static const std::string COMPLETE_BAR{"\u2588"};
3 static const std::string INCOMPLETE_BAR{"\u2591"};
4
5 for (int i = 0; i < progress / INCREMENT; i++) {
6 progress_bar += COMPLETE_BAR;
7 }
8
9 for (int i = 0; i < (1 - progress) / INCREMENT; i++) {
10 progress_bar += INCOMPLETE_BAR;
11 }
12
13 progress_bar += "";
It looks okay even without |
I’m weak NACK on this. I’m concept 0 on this
First, thanks for all your work on improving -getinfo 🥃
I think a good way to think about command line tools is how they fit into scripting work flows. We shouldn’t add elements that make it harder for someone to extract the needed information as part of their workflow.
The progress bar introduced here is an example of an element that makes it slightly more difficult to extract the needed information; The verification progress value in this case.
Additionally, it is my opinion that the inclusion of this progress bar adds very little value. It doesn’t give me any better insight into the verification progress than the number value does.
Concept ACK
Since “getinfo is no longer part of the RPC API, and client-side implementation is not subject to API stability” (quoting the description in #17314) , its output is meant only to be consumed by humans (“the eye”), not for machines and hence using it ever in a script would obviously be a bad idea that shouldn’t be encouraged. For those purposes we have the RPC API.
I think CLI users deserves to have a nice looking output as well, I can imagine it’s nice to see the verification progress also visually via watch -n 1 bitcoin-cli -getinfo :) that said, I agree of course that this PR is absolutely not high priority.
./src/bitcoin-cli -netinfo help states at the top: This human-readable interface will change regularly and is not intended to be a stable API. Now, -getinfo is IIRC the oldest CLI command, as indeed it previously was an RPC, but when it became a client-side command and now has been fully revamped in presentation, anyone who was depending on scraping it with a script may have received the message that it’s better to use the RPC API instead.
Concept ACK, nice UX addition for the CLI.
Especially neat with watch -n 1 bitcoin-cli -getinfo as @theStack mentioned above.
Thanks, reviewers for the feedback and insights!
It seems there is a mixed response to this feature. I will leave this PR open for a few more days to get more feedback and close it afterward if there is still no strong demand for it.
If you do keep it, maybe a muted grey instead and not displayed if > 99%. @jonatack thanks for the suggestion! Implemented it in e75ee3e to a94d991
< 99%
Progress bar will only be displayed when verification progress < 99% Change filled bar to make it less blinding
No progress bar for >= 99% verification makes sense.
0-\u2588
1+\u2592
New Unicode for COMPLETE BAR looks good.
Tested with command: watch -c -t -n 1 bitcoin-cli -getinfo (Don’t see colors using watch but the progress bar and other things LGTM.
tACK https://github.com/bitcoin/bitcoin/commit/a94d99199f17c667ba76fdb77c48ce6136afa81f
Tested ACK a94d99199f17c667ba76fdb77c48ce6136afa81f 📊
Agree that the new (in)complete characters look way better and are less blinding, also displaying the bar only on <99% progress makes sense.
-color=always
tACK a94d99199f17c667ba76fdb77c48ce6136afa81f
Nice update. However, @klementtan I’d strongly suggest adding @jonatack’s suggestions. For example, I just noticed the lack of space between the progress bar and the percentage value. Additionally, I think the other suggestions there would greatly help readability and simplify future code reviews.
Note: I also wasn’t getting color from watch --color -n 1 so I prefixed bitcoin-cli -getinfo with unbuffer (from the expect package) to get it working.
883@@ -884,6 +884,29 @@ static void GetWalletBalances(UniValue& result)
884 result.pushKV("balances", balances);
885 }
886
887+/**
888+ * GetProgressBar contructs a progress bar with 5% intervals.
889+ *
890+ * @param[in] progress The proportion of the progress bar to be filled between 0 and 1.
891+ * @params[out] progress_bar String representation of the progress bar.
0 * [@param](/bitcoin-bitcoin/contributor/param/)[out] progress_bar String representation of the progress bar.
crACK https://github.com/bitcoin/bitcoin/commit/71c1556f1476abb61f3faca239e2f2eb857b72cb. Will test this week.
Changes that I see since last review:
GetProgressBar() function from static to void. Reason is explained by @jonatack in this comment0-static std::string GetProgressBar(const double progress)
1+void GetProgressBar(double progress, std::string& progress_bar)
2
3-return progress_bar;
progress is less than zero(min) or greater than 1(max) instead of printing empty string.0-if (progress < 0 || progress > 1) return "";
1+if (progress < 0 || progress > 1) return;
std::string initializes an empty string ("") by default. In few other languages, devs prefer to initialize it to avoid null exceptions.0-std::string progress_bar = "";
1+std::string ibd_progress_bar;
++i should be used.0-for (int i = 0; i < progress / INCREMENT; i++) {
1+for (int i = 0; i < progress / INCREMENT; ++i) {
2
3-for (int i = 0; i < (1 - progress) / INCREMENT; i++) {
4+for (int i = 0; i < (1 - progress) / INCREMENT; ++i) {
++i increments then return
i++ returns then increment
I had recently read one post on reddit and found it interesting because of 2 reasons:
a. Helped me understand different operators
b. This comment: i -=- 1
0-verification_progress_bar = GetProgressBar(verification_progress);
1+GetProgressBar(ibd_progress, ibd_progress_bar);
2+ibd_progress_bar += " ";
a94d991 … dba61d0
Note: I also wasn’t getting color from watch –color -n 1 so I prefixed bitcoin-cli -getinfo with unbuffer (from the expect package) to get it working. @Zero-1729 Thanks for testing! I think you will need to use
watch -c -t -n 1 ./src/bitcoin-cli -signet -color=always -getinfo. Currently-getinfowill not print color if the stdout is not connected to a terminal(watch).
re-tACK dba61d0cd1226fedaef194b6851c34674fda4bd6 🧪
LGTM, great work @klementtan!
Note: Yeah, appending -color=always to bitcoin-cli did the trick, no need for unbuffer again.
@prayank23 Thanks for testing it! I think you’re not seeing colour because your stdout is not connected to the terminal. You can get colour if you set -color=always
Yes this works. I forgot the option that you added in last PR.
watch -c -t -n 1 bitcoin-cli -getinfo color=always
Some nits after the latest change:
static attribute from GetProgressBar; the function still only needs to be visible in the same compilation unitprogress_bar, but never initializes (could either clear the string in the beginning or assert that it is empty?)Not any blockers though. Will re-test later.
there is no reason to remove the
staticattribute fromGetProgressBar; the function still only needs to be visible in the same compilation unit
Agree!
the new interface implictely assumes now that the passed string is already empty, as it only appends to
progress_bar
IIUC this seems fine, insofar that one might want to append a progress bar to a result string that already has some content.
I don’t think returning a string by value necessarily leads to a copy – this is a typical case where move semantics potentially kick in and a copy is avoided (see e.g. stackoverflow.com/questions/48160292/in-c11-does-returning-a-stdstring-in-a-function-move-or-copy-it); there are also techniques like copy elision and (N)RVO (fluentcpp.com/2016/11/28/return-value-optimizations), though I’m not sure if we have any guarantees here with C++17; maybe someone deeper in C++ can shed some light here
I hadn’t noticed this codebase depending on the compiler for it, though perhaps we already do somewhere.
Co-authored-by: jonatack <jon@atack.com>
watching verification from 0 to 100% on signet)
static attribute to GetProgressBar
there is no reason to remove the static attribute from GetProgressBar; the function still only needs to be visible in the same compilation unit
Agree. It can be changed to non-static if required later.
reACK https://github.com/bitcoin/bitcoin/commit/b851a92c06cd1d6a40a5527ae39d60ae7fa33fb9
re-tACK b851a92c06cd1d6a40a5527ae39d60ae7fa33fb9 (re-tested, works as expected 🍾)
It’s a clean push 🪄✨
0$ git range-diff dba61d0...b851a92
01: dba61d0cd ! 1: b851a92c0 cli: Add progress bar for -getinfo
1 ...
2 -+void GetProgressBar(double progress, std::string& progress_bar)
3 ++static void GetProgressBar(double progress, std::string& progress_bar)
ACK b851a92c06cd1d6a40a5527ae39d60ae7fa33fb9
Tested by manually setting progress inside the new function with values between 0 and 1 and also with
0$ ./src/bitcoind -signet -reindex -daemon
1$ watch -c -t -n 1 ./src/bitcoin-cli -signet -color=always -getinfo
Tested ACK https://github.com/bitcoin/bitcoin/pull/22547/commits/b851a92c06cd1d6a40a5527ae39d60ae7fa33fb9 on mainnet and signet on Ubuntu 20.04.
Good UX improvement.
No strong opinion Tested b851a92c06cd1d6a40a5527ae39d60ae7fa33fb9 on Ubuntu 20.04
This PR intends on adding a new progress bar in the output of command ./bitcoin-cli -signet -daemon. By adding new functions that create a string of greyscale Unicode characters to give a perception of the progress bar. The ratio of lighter and darker grey tones of these characters is a function of the ratio of the verification process that has been completed.
Tested the PR by the command bitcoind -signet -reindex followed by ./src/bitcoin-cli -signet -getinfo. The PR is successful in adding the changes it intends to add.
Attaching a screenshot to show the proper working of the progress bar in the result of -getinfo
I neither support nor disagree with the changes intended by this PR. Adding a progress bar does make the result of -getinfo command more eye-pleasing, but it doesn’t feel like an important or crucial addition to the result. As the purpose of bitcoin-cli is to be rather highly efficient than being eye-pleasing. If there is some way of adding the progress bar as a suboption of -getinfo then I am ACKed on that, because then the bitcoin-cli user has the choice to rather display the progress bar or not while calling -getinfo.
@ShaMan239 Thanks for the detailed review! 👍
I agree that this feature is definitely not essential but from the feedback of other reviewers, it seems that it would make the user experience of using bitcoin-cli better.
adding the progress bar as a suboption
For now, I am not really inclined towards adding that because it would make -getinfo more complicated than necessary but if there is enough demand for it I would be more than happy to add it.