Don't clear console prompt when font resizing by jarolrod · Pull Request #271 · bitcoin-core/gui

jarolrod · 2021-04-05T08:23:18Z

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

Tested ACK dcd7a8c

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 · 2021-04-05T17:30:27Z

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 · 2021-04-10T09:04:46Z

Concept ACK.

This preserves the prompt text throughout resizes.

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

promag

Concept ACK.

promag · 2021-04-17T11:38:22Z

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 · 2021-04-20T05:14:15Z

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

hebasto · 2021-04-20T16:17:56Z

@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 · 2021-04-21T00:00:30Z

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

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

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

jarolrod · 2021-04-23T03:23:56Z

updated from dcd7a8c -> 7962e0d

Changes:

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

hebasto

ACK 7962e0d

Talkless

tACK 7962e0d, tested on Debian Sid with Qt 5.15.2

Talkless · 2021-04-25T16:06:18Z

 // clear console (reset icon sizes, default stylesheet) and re-add the content
 float oldPosFactor = 1.0 / ui->messagesWidget->verticalScrollBar()->maximum() * ui->messagesWidget->verticalScrollBar()->value();
- clear(false);
+ clear(/* keep_prompt */ true);


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.

The relevant discussion is here bitcoin/bitcoin#21684.

hebasto · 2021-04-25T16:19:47Z

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

Talkless · 2021-04-26T17:26:57Z

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

laanwj · 2021-05-11T08:59:01Z

Code review ACK 7962e0d

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.

luke-jr · 2021-06-12T19:59:08Z

 ui->messagesWidget->verticalScrollBar()->setValue(oldPosFactor * ui->messagesWidget->verticalScrollBar()->maximum());
 }

-void RPCConsole::clear(bool clearHistory)


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

I don't follow... In d2cc339RPCConsole::clear() has no parameters.
Why keep the existing useless param?

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.

Using an enum class would also solve it.
Agree. bitcoin/bitcoin#21684

jarolrod mentioned this pull request Apr 5, 2021
Don't Clear Console Prompt When Resizing Text #269
Closed

jonatack reviewed Apr 5, 2021
View reviewed changes

hebasto added the Feature label Apr 10, 2021

promag reviewed Apr 16, 2021
View reviewed changes

hebasto added 3 commits April 22, 2021 22:29

qt: Untie irrelevant signal-slot parameters
4f0ae47
QAbstractButton::clicked signal has the `checked` parameter that is irrelevant to RPCConsole::clear slot parameter.

qt, refactor: Drop redundant history cleaning in RPC console
d2cc339
The default clearHistory=true argument is passed in the RPCConsole ctor only. This is needless, as the history and historyPtr members are initialized properly.

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

jarolrod force-pushed the dont-clear-console branch from dcd7a8c to 7962e0dCompare April 23, 2021 03:22

hebasto approved these changes Apr 23, 2021
View reviewed changes

RandyMcMillan mentioned this pull request Apr 24, 2021
Network Graph layout - debug window improvement. #291
Closed

Talkless approved these changes Apr 25, 2021
View reviewed changes

hebasto added this to the 22.0 milestone May 8, 2021

hebasto added UX All about "how to get things done" and removed Feature labels May 10, 2021

hebasto changed the title ~~Don't clear console prompt when resizing~~Don't clear console prompt when font resizingMay 11, 2021

laanwj merged commit 39e3060 into bitcoin-core:masterMay 11, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 11, 2021
Mergebitcoin-core/gui#271: Don't clear console prompt when font resi…
d15780f

jarolrod deleted the dont-clear-console branch May 20, 2021 21:20

luke-jr reviewed Jun 12, 2021
View reviewed changes

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
Merge39e3060into merged_master (Bitcoin PRbitcoin-core/gui#271)
87cb26b

bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022

Uh oh!

Conversation

jarolrod commented Apr 5, 2021

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jarolrod commented Apr 5, 2021

Uh oh!

hebasto commented Apr 10, 2021

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag commented Apr 17, 2021

Uh oh!

Bosch-0 commented Apr 20, 2021

Uh oh!

hebasto commented Apr 20, 2021

Uh oh!

Bosch-0 commented Apr 21, 2021

Uh oh!

jarolrod commented Apr 23, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Talkless left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TalklessApr 25, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebastoApr 25, 2021

Choose a reason for hiding this comment

Uh oh!

hebasto commented Apr 25, 2021

Uh oh!

Talkless commented Apr 26, 2021

Uh oh!

laanwj commented May 11, 2021

Uh oh!

luke-jrJun 12, 2021

Choose a reason for hiding this comment

Uh oh!

hebastoJun 12, 2021

Choose a reason for hiding this comment

Uh oh!

luke-jrJun 12, 2021

Choose a reason for hiding this comment

Uh oh!

hebastoJun 12, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

jarolrod commented Apr 23, 2021•
edited
Loading

Talkless left a comment •
edited
Loading

TalklessApr 25, 2021•
edited
Loading