Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gui: Show current wallet name in window title #15149

Merged
merged 3 commits into from Jan 15, 2019

Conversation

Projects
None yet
9 participants
@promag
Copy link
Member

commented Jan 12, 2019

screenshot 2019-01-11 at 23 58 26

@fanquake fanquake added the GUI label Jan 12, 2019

Show resolved Hide resolved src/qt/bitcoingui.cpp Outdated
Show resolved Hide resolved src/qt/bitcoingui.h Outdated
@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Jan 12, 2019

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

@promag

This comment has been minimized.

Copy link
Member Author

commented Jan 12, 2019

I suggest removing the text part "Wallet" | "Node".

@jonasschnelli I kind of agree, not very helpful I guess.

@promag promag force-pushed the promag:2019-01-updatewindowtitle branch 3 times, most recently Jan 12, 2019

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jan 12, 2019

Now with Jonas suggestion.

@benthecarman

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2019

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

@promag promag referenced this pull request Jan 12, 2019

Merged

gui: Add WalletController #15101

@hebasto

This comment has been minimized.

Copy link
Member

commented Jan 13, 2019

Concept ACK.

Show resolved Hide resolved src/qt/bitcoingui.cpp
Show resolved Hide resolved src/qt/bitcoingui.h Outdated

@promag promag force-pushed the promag:2019-01-updatewindowtitle branch Jan 13, 2019

@luke-jr

This comment has been minimized.

Copy link
Member

commented Jan 13, 2019

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

@promag

This comment has been minimized.

Copy link
Member Author

commented Jan 13, 2019

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

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Jan 13, 2019

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

This comment has been minimized.

Copy link
Member

commented Jan 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.

@promag

This comment has been minimized.

Copy link
Member Author

commented Jan 14, 2019

@hebasto dropped enableWallet check, walletFrame is indeed sufficient:

if(enableWallet)
{
/** Create wallet frame and make it the central widget */
walletFrame = new WalletFrame(_platformStyle, this);

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

Let me know so I can squash.

@hebasto

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

tACK c1e8f74f326f862adbf5a326eb1b9a65e6eb1edd

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

@promag

This comment has been minimized.

Copy link
Member Author

commented Jan 14, 2019

Thanks @hebasto.

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

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

@promag promag force-pushed the promag:2019-01-updatewindowtitle branch Jan 14, 2019

@hebasto

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

re-tACK b0b2d112550817e31e469a19a2d69bcef97ae4be

@Sjors

This comment has been minimized.

Copy link
Member

commented Jan 15, 2019

Concept ACK, code changes look fine, tested.

I don't like the default behavior:
schermafbeelding 2019-01-15 om 13 01 36

"- [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

This comment has been minimized.

Copy link
Member Author

commented Jan 15, 2019

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.

@promag promag force-pushed the promag:2019-01-updatewindowtitle branch Jan 15, 2019

@promag

This comment has been minimized.

Copy link
Member Author

commented Jan 15, 2019

Rebased after #14594 merge.

@Sjors

This comment has been minimized.

Copy link
Member

commented Jan 15, 2019

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.

@promag promag force-pushed the promag:2019-01-updatewindowtitle branch to fe7048b Jan 15, 2019

@promag

This comment has been minimized.

Copy link
Member Author

commented Jan 15, 2019

@Sjors now with your suggestion.

@Sjors

This comment has been minimized.

Copy link
Member

commented Jan 15, 2019

tACK fe7048b

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Jan 15, 2019

Tested ACK fe7048b

@jonasschnelli jonasschnelli merged commit fe7048b into bitcoin:master Jan 15, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

jonasschnelli added a commit that referenced this pull request Jan 15, 2019

Merge #15149: gui: Show current wallet name in window title
fe7048b gui: Show current wallet name in window title (João Barbosa)
8a79261 gui: Keep network style in BitcoinGUI (João Barbosa)
f411c8b gui: Remove unused return type in some BitcoinGUI methods (João Barbosa)

Pull request description:

  <img width="876" alt="screenshot 2019-01-11 at 23 58 26" src="https://user-images.githubusercontent.com/3534524/51065458-d7ebaf80-15fc-11e9-9162-e37e9a10d448.png">

Tree-SHA512: 5c43f615834983bc1c5045e07c6e119044dd78ca947fd2679d302b519d5ce1d08d29ca00b1c11e88c4bbc4d56f2e6f4a8adc42084f3503e751e642e8a13112dc

@promag promag deleted the promag:2019-01-updatewindowtitle branch Jan 15, 2019

@Sjors

This comment has been minimized.

Copy link
Member

commented Jan 19, 2019

Oops, now we have a trailing - on mainnet.

laanwj added a commit that referenced this pull request Jan 21, 2019

Merge #15210: gui: Fix window title update
1ed425e gui: Fix window title update (João Barbosa)

Pull request description:

  Removes trailing `-` from window title when running on mainnet.

  Reported by @Sjors in #15149 (comment).

Tree-SHA512: 22f13c361496720f30a4926d928851ed74456c0d70bd313b0ebaca91a9ebfde96991091ac3d1b094f33d3ce9afafd709eb1917f00d96fa3ca69751b6b14e1d2b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.