On master (3f512f3d563954547061ee743648b57a900cbe04) it is possible to enter another command while the current command is still being executed. That makes a mess in the output.
With this PR:
Some previous context: #59 (comment)
It is still possible to enter and execute the stop command any time.
Tested ACK 0b8b65392a590dc0ac04818b4933676e3f2c5a7c - works as intended.
A follow up would be to add a cancel/abort button. Especially for things like “waitforblock 99999999” (would even be useful for gettxoutsetinfo).
A follow up would be to add a cancel/abort button. Especially for things like “waitforblock 99999999” (would even be useful for
gettxoutsetinfo).
Combine cancel/abort functionality into already present Clear button?
887@@ -890,8 +888,10 @@ void RPCConsole::on_lineEdit_returnPressed()
888 {
889 QString cmd = ui->lineEdit->text();
890
891- if(!cmd.isEmpty())
892- {
893+ if (!cmd.isEmpty()) {
894+ ui->lineEdit->setEnabled(false);
enter won’t clear the line, etc.). The executing... text would need to be somewhere else though…
quitstop. With this change this is not possible.
One way to abort running command is to run
quit.
When did the quit command become a thing in the GUI console?
Or is it a suggestion?
I just mean it won’t work anymore.
I’m going to address your concerns.
I happen to use it, but can quit from the GUI.
I cannot find the quit command now.
One way to abort running command is to run
stop. With this change this is not possible.
This would shutdown bitcoin-qt. Right? I don’t think this is an adequate action for stopping a single RPC call. It’s already possible with Ctrl-Q.
Why not just emulate the shell (bitcoin-cli usage) as good as possible?
This would translate that into our Qt world with:
891@@ -892,53 +892,53 @@ void RPCConsole::on_lineEdit_returnPressed()
892 {
893 QString cmd = ui->lineEdit->text();
Another sensible approach would be to disable the line edit while keep the 2nd command there and execute it when the 1st ends.
This deprives the user of the ability to change his mind when the 1st command output is arrived.
841+ } else {
842 out += GUIUtil::HtmlEscape(message, false);
843+ }
844 out += "</td></tr></table>";
845 ui->messagesWidget->append(out);
846+ scrollToEnd();
ACK 08d599e4f3b2e3a1143863d3cf1e64a8215a59b1, Tested on macOS 11.1 with Qt 5.15.2
Master: Allowed to input several commands at once which will lead to messy output
PR: Can only execute one command at a time
Additional Thoughts (for a followup pr) As others have stated, it would be nice to have a cancel button on the command. This should be done in a follow up PR. An example of when this is useful:
gettxout commandgettxoutsetinfo which takes some time to rungettxoutsetinfo command ran by accident, and not have to wait until it is finished to accomplish what I set out to accomplish -> run gettxoutPerhaps something like this: (rough mockup)
917+ } catch (const std::exception& e) {
918+ QMessageBox::critical(this, "Error", QString("Error: ") + QString::fromStdString(e.what()));
919+ return;
920+ }
921+
922+ if (cmd.startsWith("stop")) {
nit: QString::startsWith() has overload taking QLatin1String which is recommended to use if exists. It will not allocate new QString, and comparing to latin1 instead of utf-16 is cheaper memory-bandwith-wise.
Also, maybe startsWith is not needed at all as the cmd string was trimmed beforehand? i.e. cmd == QLatin1String("stop") would suffice?
958+ }
959+#endif // ENABLE_WALLET
960
961- cmd = QString::fromStdString(strFilteredCmd);
962+ message(CMD_REQUEST, QString::fromStdString(strFilteredCmd));
963+ message(CMD_REPLY, "Executing...");
QStringLiteral("Executing...")
tACK 08d599e4f, tested on Debian Sid with Qt 5.15.2.
I’ve generated bunch of transactions on regtest using generatetoaddress and later used listunspent as long-running command to reproduce issue on master, invoking multiple commands while first one is still running, and I see that PR code works as expecting afterwards.
Tested https://github.com/bitcoin-core/gui/pull/123/commits/b87a4cd6bb58a6458dea7527a19433ead0ea848d on Ubuntu 20.04 Qt 5.12.8.
When executing commands enough to fill the entire result dialog, instead of scrolling down, it scrolls up to the top (“Welcome to the Bitcoin Core RPC console.” line). On the master branch, the scrolling apparently works OK.
Edit: I commented the line ui->messagesWidget->undo(); and the scrolling has returned to working normally.
0connect(executor, &RPCExecutor::reply, this, [this](int category, const QString& command) {
1 // ui->messagesWidget->undo();
2 message(category, command);
3 m_is_executing = false;
4});
905@@ -906,6 +906,16 @@ void RPCConsole::on_lineEdit_returnPressed()
906 return;
907 }
908
909+ if (cmd == QLatin1String("stop")) {
b87a4cd6bb58a6458dea7527a19433ead0ea848d
Could add a comment explaining this case.
Tested ACK b87a4cd6bb58a6458dea7527a19433ead0ea848d.
if new output is appended and its not visible (user scrolled) then it could show some feedback/alert so user knows command finished and can scroll down.
nit, could make “qt, rpc: Accept stop RPC even another command is executing” before “qt, rpc: Do not accept command while executing another one” to avoid adding || m_is_executing and then removing.
Note: I’ve originally ACK https://github.com/promag/bitcoin/commit/328efdcb69eedbd3bce6d6de865cca0e3ee38374 which is b87a4cd6bb58a6458dea7527a19433ead0ea848d rebased on f385ad765174afb02e60900581612a19c143cf83.
@leonardojobim @Talkless @jarolrod @luke-jr @promag @jonasschnelli
Thanks you for your reviewing and testing!
Updated b87a4cd6bb58a6458dea7527a19433ead0ea848d -> 716139584ddf7ef554962e0188ae8b8a74e310e4 (pr123.05 -> pr123.06, diff):
stop command (@promag)
Wait @hebasto, a bit off topic, should I have had to make actual GitHub review in this interface, or comment is enough? Maybe sticking to always checking all files and producing GitHub review would be “better”?
Not sure if I understand your question in part “producing GitHub review”…
Anyway, your #123 (comment) is straightforward. While reviewing pulls after rebasing I do the same locally git range-diff master pr-before-rebase pr-after-rebase. If rebasing is not trivial it could be valuable re-check all changes.
Not sure if I understand your question in part “producing GitHub review”…
In Github web interface, we can go to “Files changed” tab, check “Viewed” checkboxes on the files, and click “Review changes” -> “Submit review”, which produces sort of “special” kind of “comment”. Maybe it’s “better” for maintainers to check for these kind of “reviews”, instead of “just” a comment?
Not sure if I understand your question in part “producing GitHub review”…
In Github web interface, we can go to “Files changed” tab, check “Viewed” checkboxes on the files, and click “Review changes” -> “Submit review”, which produces sort of “special” kind of “comment”. Maybe it’s “better” for maintainers to check for these kind of “reviews”, instead of “just” a comment?
In this project we strive to rely on GitHub as little as possible :) Ofc, a reviewer could use any GitHub feature he like. Features you mentioned make no difference for maintainers.
re-utACK 7161395, range-diff does not show anything to actually review or test.
Ugh, not again… I did range-diff for review, but forgot to actually switch to new tip before copying latest commit hash..
re-ACK cb48c5964c901bccf5d2ffcd9f3f3e4552ae2fb1
@jarolrod @promag @leonardojobim
Do you mind having another (final?) look into at this PR?
981+#endif // ENABLE_WALLET
982
983- // Scroll console view to end
984- scrollToEnd();
985+ message(CMD_REQUEST, QString::fromStdString(strFilteredCmd));
986+ //: A message in the GUI console while an entered command being executed.
nit
0 //: A console message indicating an entered command is currently being executed.
896@@ -897,57 +897,71 @@ void RPCConsole::setMempoolSize(long numberOfTxs, size_t dynUsage)
897
898 void RPCConsole::on_lineEdit_returnPressed()
899 {
900- QString cmd = ui->lineEdit->text();
901+ QString cmd = ui->lineEdit->text().trimmed();
nit
we should document that we are using this function in the commit message for proper documentation
ACK cb48c5964c901bccf5d2ffcd9f3f3e4552ae2fb1
Edit: Retracting ACK because of a silent merge conflict with #335. The signal count which is expected has changed with this PR, this signal count should be adjusted in the tests to reflect the introduction of the "Executing..." message.
Tested functionality on Arch Linux Qt 5.15.2 and macOS 11.3 Qt 5.15.2. Tested each commit individually, cherry-picking each on top of master. This is a nice patch. A couple of nits, no need to address unless you absolutely have to retouch. 🥃
Notes on commits:
Screenshots: Below are screenshots showing the behavior of master vs pr. Note that one can continue to input commands on master, but cannot with this pr:
| Master | Description |
|---|---|
 |
 |
While here, clean up the command input by calling the trimmed function
on the input from the command prompt.
ACK 38eb37c
re-tested functionality. Ran tests to confirm this fixes the silent merge conflict with #335.
Rebased pr123.07 on top of the commit pr123.08 is based on, 933c6466c2bda3a06d3c5da0de0d8f05111c9f4c, and got the following diff which shows the only changes from my last review are; changes to translator comment to address my review comment and update to apptests to reflect the expected signal count:
0$ git diff pr123.07 pr123.08
1diff --git a/src/qt/rpcconsole.cpp b/src/qt/rpcconsole.cpp
2index 803946dcf..c762f2b9c 100644
3--- a/src/qt/rpcconsole.cpp
4+++ b/src/qt/rpcconsole.cpp
5@@ -969,7 +969,7 @@ void RPCConsole::on_lineEdit_returnPressed()
6 #endif // ENABLE_WALLET
7
8 message(CMD_REQUEST, QString::fromStdString(strFilteredCmd));
9- //: A message in the GUI console while an entered command being executed.
10+ //: A console message indicating an entered command is currently being executed.
11 message(CMD_REPLY, tr("Executing…"));
12 m_is_executing = true;
13 Q_EMIT cmdRequest(cmd, m_last_wallet_model);
14diff --git a/src/qt/test/apptests.cpp b/src/qt/test/apptests.cpp
15index c1d5f84be..cb3dbd226 100644
16--- a/src/qt/test/apptests.cpp
17+++ b/src/qt/test/apptests.cpp
18@@ -40,7 +40,7 @@ void TestRpcCommand(RPCConsole* console)
19 QTest::keyClicks(lineEdit, "getblockchaininfo");
20 QTest::keyClick(lineEdit, Qt::Key_Return);
21 QVERIFY(mw_spy.wait(1000));
22- QCOMPARE(mw_spy.count(), 2);
23+ QCOMPARE(mw_spy.count(), 4);
24 QString output = messagesWidget->toPlainText();
25 UniValue value;
26 value.read(output.right(output.size() - output.lastIndexOf(QChar::ObjectReplacementCharacter) - 1).toStdString());
Tested ACK 38eb37c0bd29b4cb825de905e8eec87636a5221b.
Added a dummy sleep in some call just to simplify testing.