rpc: Do not accept command while executing another one

[hebasto]

On master (3f512f3) 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.

[jonasschnelli]

Tested ACK 0b8b653 - 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).

[hebasto]

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?

[promag]

One way to abort running command is to run quitstop. With this change this is not possible.

[hebasto]

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?

[promag]

I just mean it won't work anymore. I happen to use it, but can quit from the GUI.

[hebasto]

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.

[promag]

s/quit/stop 🤦 updated above.

[jonasschnelli]

@promag

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?

• No parallel commands are possible in a single shell window
• Ctrl-C abort the call
• Clear visual sign that something is "working.." (no prompt visible)
• Allow entering a next command (but impossible to execute before old has been finished)

This would translate that into our Qt world with:

• Once a command has been started, forbid executing a new one until the old has "returned" (done in this PR)
• Have a specific "abort" button or support something like Ctrl-C if it looks silly (should be added)
• • If the "abort" button looks bad (quick show/hide) for commands taking only a few milliseconds, maybe only show the abort button after 1s of execution time
• Show that a command is executed (done in this PR, but maybe should be done differently to allow entering a next command).
• Allow entering a followup command but can't execute (should be added)

[hebasto]

@jonasschnelli Spotted your comment too late, so, probably, the last commit in the latest push should be dropped, right?

[promag]

Another sensible approach would be to disable the line edit while keep the 2nd command there and execute it when the 1st ends.

[hebasto]

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.

[hebasto]

Updated 980b8e9 -> e8753bf (pr123.02 -> pr123.03, diff):

• addressed @promag's comment:

nit, could trim.

• addressed @luke-jr's comment:

If the user has scrolled up, we shouldn't ignore that and reset scroll to the bottom...

[jarolrod]

ACK 08d599e, 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:

• I want to enter the gettxout command
• I mistakingly autocomplete to the gettxoutsetinfo which takes some time to run
• It would be nice to be able to cancel the gettxoutsetinfo command ran by accident, and not have to wait until it is finished to accomplish what I set out to accomplish -> run gettxout

Perhaps something like this: (rough mockup)

[Talkless]

tACK 08d599e, 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.

[hebasto]

Updated 08d599e -> b87a4cd (pr123.04 -> pr123.05, diff):

• addressed @Talkless' comment

• translated user-faced string

[Talkless]

re-ACK b87a4cd after tweaks.

[leonardojobim]

Tested b87a4cd 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.

connect(executor, &RPCExecutor::reply, this, [this](int category, const QString& command) {
    //  ui->messagesWidget->undo();
    message(category, command);
    m_is_executing = false;
});

[hebasto]

Rebased 7161395 -> cb48c59 (pr123.06 -> pr123.07) due to the conflict with #257.

[Talkless]

re-utACK 7161395, range-diff does not show anything to actually review or test.

[Talkless]

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

[hebasto]

@Talkless

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.

[Talkless]

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?

[hebasto]

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.

[hebasto]

@Talkless

In #123 (comment)

re-utACK 7161395, range-diff does not show anything to actually review or test.

Did you mean the old top 7161395 or the new cb48c59?

[Talkless]

@Talkless

In #123 (comment)

re-utACK 7161395, range-diff does not show anything to actually review or test.

Did you mean the old top 7161395 or the new cb48c59?

Ugh, not again... I did range-diff for review, but forgot to actually switch to new tip before copying latest commit hash..

re-ACK cb48c59

[hebasto]

@jarolrod @promag @leonardojobim

Do you mind having another (final?) look into at this PR?

[promag]

Tested ACK cb48c59.

[jarolrod]

ACK cb48c59

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:

• 9d9f7f1
  • Tested that "Executing..." message is displayed and removed upon response
• 614cc38
  • Concept ACK on refactoring the logic here to return early if the command is empty
• d4e7fcc
  • Tested that we can call stop, although true testing of this commit can only be done with the next commit applied. I think we should have kept this commit after qt, rpc: Do not accept command while executing another one
  • ACK on using the trimmed Qstring function % nit
• cb48c59
  • Tested that we cannot input another command while another is being executed
  • Tested that we can still call the stop command
    • It's annoying that this command just quits the application. It would be nice to change this in a followup.

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

[hebasto]

Updated cb48c59 -> 38eb37c (pr123.07 -> pr123.08):

• rebased to fix the silent merge conflict with test: Use QSignalSpy instead of QEventLoop #335
• addressed the recent @jarolrod's comments

[jarolrod]

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, 933c646, 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:

$ git diff pr123.07 pr123.08
diff --git a/src/qt/rpcconsole.cpp b/src/qt/rpcconsole.cpp
index 803946dcf..c762f2b9c 100644
--- a/src/qt/rpcconsole.cpp
+++ b/src/qt/rpcconsole.cpp
@@ -969,7 +969,7 @@ void RPCConsole::on_lineEdit_returnPressed()
 #endif // ENABLE_WALLET
 
     message(CMD_REQUEST, QString::fromStdString(strFilteredCmd));
-    //: A message in the GUI console while an entered command being executed.
+    //: A console message indicating an entered command is currently being executed.
     message(CMD_REPLY, tr("Executing…"));
     m_is_executing = true;
     Q_EMIT cmdRequest(cmd, m_last_wallet_model);
diff --git a/src/qt/test/apptests.cpp b/src/qt/test/apptests.cpp
index c1d5f84be..cb3dbd226 100644
--- a/src/qt/test/apptests.cpp
+++ b/src/qt/test/apptests.cpp
@@ -40,7 +40,7 @@ void TestRpcCommand(RPCConsole* console)
     QTest::keyClicks(lineEdit, "getblockchaininfo");
     QTest::keyClick(lineEdit, Qt::Key_Return);
     QVERIFY(mw_spy.wait(1000));
-    QCOMPARE(mw_spy.count(), 2);
+    QCOMPARE(mw_spy.count(), 4);
     QString output = messagesWidget->toPlainText();
     UniValue value;
     value.read(output.right(output.size() - output.lastIndexOf(QChar::ObjectReplacementCharacter) - 1).toStdString());

[hebasto]

@Talkless @promag

Hoping on your re-reviewing.

[promag]

Tested ACK 38eb37c.

Added a dummy sleep in some call just to simplify testing.