Save/restore RPCConsole geometry only for window by hebasto · Pull Request #194 · bitcoin-core/gui

hebasto · 2021-01-21T14:32:28Z

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

maflcko · 2021-01-21T18:14:22Z

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

hebasto · 2021-01-21T20:51:19Z

Fixed.

luke-jr

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

Code review ACK b5523f1.

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

luke-jr · 2021-01-27T15:56:46Z

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 · 2021-01-27T17:07:16Z

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

bool IsWalletEnabled()
{
#ifdef ENABLE_WALLET
    return !gArgs.GetBoolArg("-disablewallet", DEFAULT_DISABLE_WALLET);
#else
    return false;
#endif
}

luke-jr · 2021-01-27T17:39:19Z

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

hebasto · 2021-01-27T21:00:24Z

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

jarolrod

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 · 2021-02-21T08:57:04Z

@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 · 2021-03-07T16:10:56Z

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

tACK 01d9586, 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

ACK 01d9586, 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

promag

Code review ACK 01d9586.

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 · 2021-05-10T20:50:57Z

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

… window 01d9586 qt: Save/restore RPCConsole geometry only for window (Hennadii Stepanov) Pull request description: 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` ACKs for top commit: Talkless: tACK 01d9586, tested on Debian Sid with Qt 5.15.2. I've managed to reproduce issue using bitcoin-core/gui#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: ACK 01d9586, tested on macOS 11.2 Qt 5.15.2 promag: Code review ACK 01d9586. Tree-SHA512: 9934cf04d4d5070dfc4671ea950e225cda9988858227e5481dad1baafa14af477bdbf4f91307ca687fde0cad6e4e605a3a99377e70d67eb115a19955ce2516f5

hebasto force-pushed the 210121-window branch from 8440fbf to b5523f1 Compare January 21, 2021 20:50

luke-jr reviewed Jan 21, 2021

View reviewed changes

promag reviewed Jan 27, 2021

View reviewed changes

DrahtBot added the Needs rebase label Jan 27, 2021

qt: Save/restore RPCConsole geometry only for window

01d9586

hebasto force-pushed the 210121-window branch from b5523f1 to 01d9586 Compare January 27, 2021 20:58

DrahtBot removed the Needs rebase label Jan 27, 2021

jarolrod reviewed Feb 19, 2021

View reviewed changes

Talkless approved these changes Mar 7, 2021

View reviewed changes

jarolrod reviewed Mar 7, 2021

View reviewed changes

jarolrod mentioned this pull request Mar 7, 2021

Independent Geometry for Node and Main Windows #241

Open

hebasto added the Feature label Mar 25, 2021

hebasto mentioned this pull request Apr 25, 2021

doc: Naming convention for new QSettings setting names #295

Open

promag reviewed May 10, 2021

View reviewed changes

hebasto merged commit a2bdbdb into bitcoin-core:master May 10, 2021

hebasto deleted the 210121-window branch May 10, 2021 20:57

This was referenced May 10, 2021

Make row color alternating in the Peers tab optional #307

Closed

Save/restore column sizes of the tables in the Peers tab #256

Merged

jarolrod mentioned this pull request May 24, 2021

qt: peers-tab resizeColumnsToContents #338

Closed

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge a2bdbdb into merged_master (Bitcoin PR bitcoin-core/gui#194)

b3ee198

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

Conversation

hebasto commented Jan 21, 2021

Uh oh!

maflcko commented Jan 21, 2021

Uh oh!

hebasto commented Jan 21, 2021

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Jan 27, 2021

Uh oh!

promag commented Jan 27, 2021

Uh oh!

luke-jr commented Jan 27, 2021

Uh oh!

hebasto commented Jan 27, 2021

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Master

PR

Uh oh!

hebasto commented Feb 21, 2021

Uh oh!

Talkless commented Mar 7, 2021

Uh oh!

Talkless left a comment

Choose a reason for hiding this comment

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented May 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants