Don't clear console prompt when font resizing

jarolrod commented at 8:23 am on April 5, 2021: member

On master, a console resize event will clear the prompt. To fix this, we store the content of the prompt and re-set it upon a resize. This preserves the prompt text throughout resizes. The text will still clear when you click the clear button, as it should.

Master

Before Resize	After Resize

PR

Before Resize	After Resize

Closes #269

jonatack commented at 4:06 pm on April 5, 2021: contributor

Tested ACK dcd7a8c3db4a037c1085616b136267e7059980f0

Naming nit, I’m not sure this applies to the GUI but per doc/developer-notes.md:

Symbol naming conventions. These are preferred in new code, but are not required when doing so would need changes to significant pieces of existing code.
- Variable (including function arguments) and namespace names are all lowercase and may use _ to separate words (snake_case).

jarolrod commented at 5:30 pm on April 5, 2021: member

Naming nit, I’m not sure this applies to the GUI but per doc/developer-notes.md: @jonatack Good point, I was following the style of the surrounding code as well as the Qt Coding Style. @hebasto what do you think? This is a good topic for the Qt Dev Notes

hebasto commented at 9:04 am on April 10, 2021: member

Concept ACK.

This preserves the prompt text throughout resizes.

Why not to implement this idea directly: https://github.com/hebasto/gui/commits/pr271-alt ?

hebasto added the label Feature on Apr 10, 2021

promag commented at 10:55 am on April 16, 2021: contributor

Concept ACK.

promag commented at 11:38 am on April 17, 2021: contributor

Concept ACK.

This preserves the prompt text throughout resizes.

Why not to implement this idea directly: https://github.com/hebasto/gui/commits/pr271-alt ?

I prefer this approach.

Bosch-0 commented at 5:14 am on April 20, 2021: none

This doesn’t appear to be an issue on Windows 10

hebasto commented at 4:17 pm on April 20, 2021: member

@Bosch-0

This doesn’t appear to be an issue on Windows 10

Do you mind elaborating?

Testing bitcoin-0.21.1rc1 (not master) on Windows Pro 20H2 (build 19042.928), and clicking on the “A+” or “A-” buttons indeed clears the console prompt. I see this PR as a UX improvement, no?

Bosch-0 commented at 0:00 am on April 21, 2021: none

Apologies I originally misread, thought this was resizing the window not font. Yes I can replicate this, will test this PR today

qt: Untie irrelevant signal-slot parameters

QAbstractButton::clicked signal has the `checked` parameter that is
irrelevant to RPCConsole::clear slot parameter.

4f0ae472e2

qt, refactor: Drop redundant history cleaning in RPC console

The default clearHistory=true argument is passed in the RPCConsole ctor
only. This is needless, as the history and historyPtr members are
initialized properly.

d2cc339005

qt: Do not clear console prompt when font resizing 7962e0dde8

jarolrod force-pushed on Apr 23, 2021

jarolrod commented at 3:23 am on April 23, 2021: member

updated from dcd7a8c -> 7962e0d

Changes:

use @hebasto’s version from branch pr271-alt. This version is the superior implementation.

hebasto approved

hebasto commented at 1:36 pm on April 23, 2021: member

ACK 7962e0dde8bbd0fa3dd702e2224774f1edaadcb6

in src/qt/rpcconsole.cpp:779 in 7962e0dde8

775@@ -776,20 +776,15 @@ void RPCConsole::setFontSize(int newSize)
776 
777     // clear console (reset icon sizes, default stylesheet) and re-add the content
778     float oldPosFactor = 1.0 / ui->messagesWidget->verticalScrollBar()->maximum() * ui->messagesWidget->verticalScrollBar()->value();
779-    clear(false);
780+    clear(/* keep_prompt */ true);

Talkless commented at 4:06 pm on April 25, 2021:

Offtopic: I feel it would be better to use enums, avoiding /* comments explaining what are we doing */ I’ve seen here and there. clear(CearMode::KeePrompt) would be self-docummenting, less error prone. But again, that’s offtopic for this PR.

hebasto commented at 4:23 pm on April 25, 2021:

The relevant discussion is here https://github.com/bitcoin/bitcoin/issues/21684.

Talkless approved

Talkless commented at 4:11 pm on April 25, 2021: none

tACK 7962e0dde8bbd0fa3dd702e2224774f1edaadcb6, tested on Debian Sid with Qt 5.15.2

hebasto commented at 4:19 pm on April 25, 2021: member

@Talkless

tACK d2cc339, tested on Debian Sid with Qt 5.15.2

Could you use the top commit hash which is 7962e0dde8bbd0fa3dd702e2224774f1edaadcb6 for the current branch state? Just edit your comment will be ok :)

Talkless commented at 5:26 pm on April 26, 2021: none

@Talkless

tACK d2cc339, tested on Debian Sid with Qt 5.15.2

Could you use the top commit hash which is 7962e0d for the current branch state? Just edit your comment will be ok :)

Ouch, sorry, fixed.

hebasto added this to the milestone 22.0 on May 8, 2021

hebasto removed the label Feature on May 10, 2021

hebasto added the label UX on May 10, 2021

laanwj commented at 8:59 am on May 11, 2021: member

Code review ACK 7962e0dde8bbd0fa3dd702e2224774f1edaadcb6

I think the PR is named confusedly though and would suggest renaming it: I assumed it was about window resizing too and was really surprised why that would clear anything.

hebasto renamed this:
~~Don't clear console prompt when resizing~~
Don't clear console prompt when font resizing
on May 11, 2021

laanwj merged this on May 11, 2021

laanwj closed this on May 11, 2021

sidhujag referenced this in commit d15780f68e on May 11, 2021

jarolrod deleted the branch on May 20, 2021

in src/qt/rpcconsole.cpp:784 in 7962e0dde8

780+    clear(/* keep_prompt */ true);
781     ui->messagesWidget->setHtml(str);
782     ui->messagesWidget->verticalScrollBar()->setValue(oldPosFactor * ui->messagesWidget->verticalScrollBar()->maximum());
783 }
784 
785-void RPCConsole::clear(bool clearHistory)

luke-jr commented at 7:59 pm on June 12, 2021:

In the future, please keep the existing param so calls to eg clear(true) don’t silently change behaviour…

hebasto commented at 8:26 pm on June 12, 2021:

I don’t follow… In d2cc3390054616c73f72a59f864700f6de14067b RPCConsole::clear() has no parameters. Why keep the existing useless param?

luke-jr commented at 8:51 pm on June 12, 2021:

In case another PR were to use the bool param, this change would silently result in different behaviour when the two are merged together. Using an enum class would also solve it.

hebasto commented at 9:03 pm on June 12, 2021:

Using an enum class would also solve it.

Agree. https://github.com/bitcoin/bitcoin/issues/21684

gwillen referenced this in commit 87cb26b9f4 on Jun 1, 2022

bitcoin-core locked this on Aug 16, 2022

Don’t clear console prompt when font resizing #271