gui: Show current wallet name in window title

promag commented at 0:00 am on January 12, 2019: member

fanquake added the label GUI on Jan 12, 2019

in src/qt/bitcoingui.cpp:570 in aa513aec74 outdated

566@@ -567,10 +567,9 @@ void BitcoinGUI::setClientModel(ClientModel *_clientModel)
567 }
568 
569 #ifdef ENABLE_WALLET
570-bool BitcoinGUI::addWallet(WalletModel *walletModel)
571+void BitcoinGUI::addWallet(WalletModel *walletModel)

promag commented at 0:05 am on January 12, 2019:

nit, WalletModel* walletModel.

in src/qt/bitcoingui.h:83 in aa513aec74 outdated

79@@ -80,8 +80,8 @@ class BitcoinGUI : public QMainWindow
80         The wallet model represents a bitcoin wallet, and offers access to the list of transactions, address book and sending
81         functionality.
82     */
83-    bool addWallet(WalletModel *walletModel);
84-    bool removeWallet(WalletModel* walletModel);
85+    void addWallet(WalletModel *walletModel);

promag commented at 0:08 am on January 12, 2019:

Same as above.

jonasschnelli commented at 0:14 am on January 12, 2019: contributor

Yeah. Why not. Concept ACK. I suggest removing the text part “Wallet” | “Node”.

promag commented at 0:19 am on January 12, 2019: member

I suggest removing the text part “Wallet” | “Node”. @jonasschnelli I kind of agree, not very helpful I guess.

promag force-pushed on Jan 12, 2019

DrahtBot commented at 1:05 am on January 12, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15153 (gui: Add Open Wallet menu by promag)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

promag commented at 3:32 am on January 12, 2019: member

Now with Jonas suggestion.

benthecarman commented at 6:11 am on January 12, 2019: contributor

tACK 530ec6c screenshot from 2019-01-12 00-10-29

hebasto commented at 2:56 pm on January 13, 2019: member

Concept ACK.

in src/qt/bitcoingui.cpp:1213 in 530ec6ca06 outdated

1206+    QString window_title = tr(PACKAGE_NAME) + " - ";
1207+#ifdef ENABLE_WALLET
1208+    if (enableWallet) {
1209+        WalletView* const walletView = walletFrame ? walletFrame->currentWalletView() : nullptr;
1210+        if (walletView) window_title += walletView->getWalletModel()->getDisplayName() + " - ";
1211+    }

hebasto commented at 4:14 pm on January 13, 2019:

0    if (walletFrame) {
1        window_title += walletFrame->currentWalletView()->getWalletModel()->getDisplayName() + " - ";
2    }

hebasto commented at 8:00 am on January 14, 2019:

@promag from IRC

walletFrame can be empty right?

Exactly! That is why I suppose checking walletFrame is sufficient and checking enableWallet and walletView is redundant.

in src/qt/bitcoingui.h:164 in 530ec6ca06 outdated

160@@ -161,6 +161,7 @@ class BitcoinGUI : public QMainWindow
161     int spinnerFrame = 0;
162 
163     const PlatformStyle *platformStyle;
164+    const NetworkStyle *m_network_style;

hebasto commented at 4:19 pm on January 13, 2019:

0    const NetworkStyle* m_network_style;

hebasto changes_requested

promag force-pushed on Jan 13, 2019

luke-jr commented at 6:16 pm on January 13, 2019: member

~~IMO this doesn’t really make sense, since the same window is used for all loaded wallets currently.~~

promag commented at 6:23 pm on January 13, 2019: member

@luke-jr chrome shows the current tab title. code editors show current file. lots of other examples..

jonasschnelli commented at 7:16 pm on January 13, 2019: contributor

IMO this doesn’t really make sense, since the same window is used for all loaded wallets currently.

IMHO, window titles are okay to be context sensitive.

hebasto commented at 8:01 am on January 14, 2019: member

@promag from IRC

walletFrame can be empty right?

Exactly! That is why I suppose checking walletFrame is sufficient and checking enableWallet and walletView is redundant.

promag commented at 8:14 am on January 14, 2019: member

@hebasto dropped enableWallet check, walletFrame is indeed sufficient: https://github.com/bitcoin/bitcoin/blob/a9b71a09a0bbbdebc4c0b10d287bf2d53f628cf4/src/qt/bitcoingui.cpp#L101-L104

Still checking walletView for the case the last wallet is unloaded.

Let me know so I can squash.

hebasto commented at 8:42 am on January 14, 2019: member

tACK c1e8f74f326f862adbf5a326eb1b9a65e6eb1edd

Checked the case when all wallets are unloaded: screenshot from 2019-01-14 10-38-00

promag commented at 8:52 am on January 14, 2019: member

Thanks @hebasto.

laanwj commented at 1:39 pm on January 14, 2019: member

Concept ACK, agree it’s fine for window titles to be dynamic and context sensitive.

promag force-pushed on Jan 14, 2019

hebasto commented at 10:38 pm on January 14, 2019: member

re-tACK b0b2d112550817e31e469a19a2d69bcef97ae4be

Sjors commented at 12:08 pm on January 15, 2019: member

Concept ACK, code changes look fine, tested.

I don’t like the default behavior:

“- [default wallet]” is visual clutter. If there’s only one wallet loaded and it doesn’t have a custom name, I would just show nothing.

promag commented at 2:29 pm on January 15, 2019: member

If there’s only one wallet loaded and it doesn’t have a custom name, I would just show nothing

That would be the same as no wallet is loaded. Beside, we already use that wording for the default wallet name. And this won’t be an issue if the goal is to avoid creating the default wallet.

gui: Remove unused return type in some BitcoinGUI methods f411c8b35b

promag force-pushed on Jan 15, 2019

promag commented at 2:36 pm on January 15, 2019: member

Rebased after #14594 merge.

Sjors commented at 3:35 pm on January 15, 2019: member

That would be the same as no wallet is loaded.

That shouldn’t cause any confusion. Either a wallet is visible or not.

It’s not clear yet what we’re going to do about the default wallet, e.g. do we force users to give it a name? I expect only a fraction of users to care about the multi-wallet stuff, so a term like “default wallet” is confusing.

gui: Keep network style in BitcoinGUI 8a79261124

gui: Show current wallet name in window title fe7048b39b

promag force-pushed on Jan 15, 2019

promag commented at 4:26 pm on January 15, 2019: member

@Sjors now with your suggestion.

Sjors commented at 5:53 pm on January 15, 2019: member

tACK fe7048b

jonasschnelli commented at 7:01 pm on January 15, 2019: contributor

Tested ACK fe7048b39bc655d65557ca95dda37c364947ddbf

jonasschnelli merged this on Jan 15, 2019

jonasschnelli closed this on Jan 15, 2019

jonasschnelli referenced this in commit c7c84209bb on Jan 15, 2019

promag deleted the branch on Jan 15, 2019

Sjors commented at 3:06 pm on January 19, 2019: member

Oops, now we have a trailing - on mainnet.

laanwj referenced this in commit 7455ca2ae6 on Jan 21, 2019

PastaPastaPasta referenced this in commit 39bea9bbe6 on Sep 8, 2021

PastaPastaPasta referenced this in commit 1e8cb1116b on Sep 8, 2021

PastaPastaPasta referenced this in commit 062e065f4c on Sep 9, 2021

vijaydasmp referenced this in commit feba11e9c8 on Sep 13, 2021

vijaydasmp referenced this in commit c46ea0f427 on Sep 13, 2021

DrahtBot locked this on Dec 16, 2021

gui: Show current wallet name in window title #15149

Conflicts