Save/restore RPCConsole geometry only for window

hebasto commented at 2:32 pm on January 21, 2021: member

After using the GUI with -disablewallet the “Node window” inherits the geometry of the main window, that could be unexpected for users.

This PR provides independent geometry settings for RPCConsole in both modes:

window sizes and QSplitter sizes when -disablewallet=0
only QSplitter sizes when -disablewallet=1

MarcoFalke commented at 6:14 pm on January 21, 2021: contributor

0qt/rpcconsole.cpp:(.text+0x77ed): undefined reference to `WalletModel::isWalletEnabled()'

hebasto force-pushed on Jan 21, 2021

hebasto commented at 8:51 pm on January 21, 2021: member

Fixed.

luke-jr commented at 11:58 pm on January 21, 2021: member

Concept ACK, though the code seems uglier than I would like (ignore this nit if we can’t think of a better way)

Perhaps add a version number or comment to the splitter settings while you’re at it, since adding/changing columns likely shouldn’t use old sizes… Or at least a comment reminding us to do so later when/if we change the columns again.

promag commented at 10:58 am on January 27, 2021: contributor

Code review ACK b5523f1aaa526b49a0678be6abe7270bdb2fc497.

I think this #ifdef madness could go away by moving WalletModel::isWalletEnabled() to another place where it always compiled in.

luke-jr commented at 3:56 pm on January 27, 2021: member

I think this #ifdef madness could go away by moving WalletModel::isWalletEnabled() to another place where it always compiled in.

But then the code here will always be compiled in… the whole point of not building wallet support is to omit the code for it :p

promag commented at 5:07 pm on January 27, 2021: contributor

@luke-jr I understand that but my proposal doesn’t mean wallet support is compiled in, only downside is a runtime check:

0bool IsWalletEnabled()
1{
2#ifdef ENABLE_WALLET
3    return !gArgs.GetBoolArg("-disablewallet", DEFAULT_DISABLE_WALLET);
4#else
5    return false;
6#endif
7}

luke-jr commented at 5:39 pm on January 27, 2021: member

But with this, everything conditional on IsWalletEnabled() (in this case, the settings code) is being compiled in to a non-wallet build.

(Unless perhaps IsWalletEnabled is carefully made always-inlined so the compiler knows to remove the branch?)

DrahtBot added the label Needs rebase on Jan 27, 2021

qt: Save/restore RPCConsole geometry only for window 01d9586ae8

hebasto force-pushed on Jan 27, 2021

hebasto commented at 9:00 pm on January 27, 2021: member

Rebased b5523f1aaa526b49a0678be6abe7270bdb2fc497 -> 01d9586ae85e49efaa00f11c1f26c24c9b82b278 (pr194.02 -> pr194.03) due to the conflict with #180.

DrahtBot removed the label Needs rebase on Jan 27, 2021

jarolrod commented at 9:56 pm on February 19, 2021: member

Can you show a screenshot of the behavior this PR is supposed to exhibit and what it is supposed to fix? Comparing with master, I don’t see what this is supposed to do.

Testing on macOS 11.1 with Qt 5.15.2

Master

Wallet Enabled Run bitcoin-qt with wallet enabled. The main window is resized to be wider.

Main Window	Node Window

Wallet Disabled run bitcoin-qt with disablewallet=1. The Node Window has inherited the geometry of the Main Window when bitcoin-qt was run with wallet enabled.

Node Window

Now, I’ll resize the Node Window to be smaller.

Node Window

Restart With Wallet Re-Enabled The Main Window inherits the geometry of the Node Window when I resized the Node Window while bitcoin-qt was run with the -disablewallet=1.

Main Window

PR

Wallet Enabled Run bitcoin-qt with wallet enabled. The main window is resized to be wider.

Main Window	Node Window

Wallet Disabled run bitcoin-qt with disablewallet=1. The Node Window has inherited the geometry of the Main Window when bitcoin-qt was run with wallet enabled. This is the same behavior as master.

Node Window

Now, I’ll resize the Node Window to be smaller.

Node Window

Restart With Wallet Re-Enabled The Main Window inherits the geometry of the Node Window when I resized the Node Window while bitcoin-qt was run with the -disablewallet=1. This is the same behavior as master.

Main Window

hebasto commented at 8:57 am on February 21, 2021: member

@jarolrod

Comparing with master, I don’t see what this is supposed to do.

Initially, I tested this pr in the Cinnamon DE only. Here are the steps to reproduce the issue on macOS BigSur 11.2.1 (20D74) on the master branch:

run bitcoin-qt -disablewallet=0, go to the menu -> Window -> Information, note the size/location of the “Node window”, quit
run bitcoin-qt -disablewallet=1, resize/move the window and note its size/location, quit
run bitcoin-qt -disablewallet=0, go to the menu -> Window -> Information, note the size/location of the “Node window” is inherited from the the step 2

Talkless commented at 4:10 pm on March 7, 2021: none

I think this #ifdef madness could go away by moving WalletModel::isWalletEnabled() to another place where it always compiled in.

isWalletEnabled could be constexpr function (that implies inline) that returns false if ENABLE_WALLET=0, and out-of-line function checking program arguments at runtime otherwise.

Talkless approved

Talkless commented at 4:28 pm on March 7, 2021: none

tACK 01d9586ae85e49efaa00f11c1f26c24c9b82b278, tested on Debian Sid with Qt 5.15.2. I’ve managed to reproduce issue using #194 (comment) instructions, and I see that this PR does detach main window and information window sizes. Built with --enable-wallet and --disable-wallet.

jarolrod commented at 4:43 pm on March 7, 2021: member

ACK 01d9586ae85e49efaa00f11c1f26c24c9b82b278, tested on macOS 11.2 Qt 5.15.2

Reproduced the steps thanks to @hebasto’s instructions. Confirming the behavior described on master and this PR fixes that

hebasto added the label Feature on Mar 25, 2021

promag commented at 9:11 am on May 10, 2021: contributor

Code review ACK 01d9586ae85e49efaa00f11c1f26c24c9b82b278.

This is a straightforward solution to handle both cases (with and without wallet support).

A different path would be to move load/save window state out of RPCConsole and also use QSettings::beginGroup to distinguish each case.

hebasto commented at 8:50 pm on May 10, 2021: member

@promag

Thank you for your review!

A different path would be to move load/save window state out of RPCConsole and also use QSettings::beginGroup to distinguish each case.

While working on this PR I did consider both variants you mentioned. The current implementation was chosen due to a smaller diff.

hebasto merged this on May 10, 2021

hebasto closed this on May 10, 2021

hebasto deleted the branch on May 10, 2021

sidhujag referenced this in commit 79ec91628e on May 11, 2021

gwillen referenced this in commit b3ee198bfb on Jun 1, 2022

bitcoin-core locked this on Aug 16, 2022

Save/restore RPCConsole geometry only for window #194

Master

PR