Overall this PR does the following:
This removes the access to address book from the File menu.
With wallet support:
Without wallet support:
386- file->addSeparator();
387 }
388 file->addAction(quitAction);
389
390- QMenu *settings = appMenuBar->addMenu(tr("&Settings"));
391+ QMenu *settings = appMenuBar->addMenu(tr("&Wallet"));
if(walletFrame)
Concept ACK.
Could move “Backup Wallet…” from File menu to Wallet menu? Agree with @Sjors’s comment
From Apple’s Human Interface Guidelines:
Provide a Window menu even if your app has only one window. Include the Minimize and Zoom menu items so people using Full Keyboard Access can invoke these functions using their keyboard.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
I don’t know what the conventions around this are, nowadays, but Concept ACK.
Edit: I don’t understand why “address book” would belong in the oh, because it’s an auxiliary windowWindow menu, it doesn’ seem window related functionality but wallet related functionality!
Thanks @jonasschnelli, I’ll see if others also toggle.
I have to test in windows.
@jonasschnelli on macos, if the active window is minimized then “Minimize” action is disabled. And “Zoom” action toggles between maximized and normal.
Also, some software add the main window to this menu, and I think we could too.
@hebasto wrote:
Could move “Backup Wallet…” from File menu to Wallet menu?
Why? Loading, saving and backing up the wallet seem like document / File operations.
Moving message signing and verification from File to Window would make more sense to me.
Even more clear IMO, but haven’t thought very deeply about it yet, is to have a Wallet top level menu. In that case I’d be more in favour of adding Backup into that, as well as sign/verify, Sending/Receiving address. The Window menu would be used for Minimize, Debug Window.
I would also be fine with dropping the “Address Book” level and just having direct menu items for Sending and Receiving addresses.
Definitely out of scope, but I would still like to see each wallet represented as a window (rather than the little dropdown we have now). However I wouldn’t want closing a window to cause unloading a wallet, so it’s not obvious how this would work.
IMO, it would be better to avoid using native widgets (Native Widgets vs Alien Widgets) and related methods (
@SjorsQWidget::windowHandle()) and objects (QWindow).
Why? Loading, saving and backing up the wallet seem like document / File operations.
Agree. @jonasschnelli
I think the “minimize” entry should toggle to “maximise” once minimized or at least maximises when selecting “minimize” in already minimized state. @promag on macos, if the active window is minimized then “Minimize” action is disabled. And “Zoom” action toggles between maximized and normal.
On macOS, if the active window is minimized the both “Minimize” and “Zoom” actions should be disabled. That is not the case for 3f779b3b99b674b7beecc9a30fa37bda1d722387.
393@@ -396,11 +394,32 @@ void BitcoinGUI::createMenuBar()
394 }
395 settings->addAction(optionsAction);
396
397- QMenu *help = appMenuBar->addMenu(tr("&Help"));
398+ QMenu* window_menu = appMenuBar->addMenu(tr("&Window"));
399+ QAction* minimize_action = window_menu->addAction(tr("Minimize"), this, &QMainWindow::showMinimized, QKeySequence(Qt::CTRL + Qt::Key_M));
400+ QObject::connect(windowHandle(), &QWindow::windowStateChanged, [this, minimize_action] {
401+ minimize_action->setEnabled(windowHandle()->windowState() != Qt::WindowMinimized);
From Qt docs:
The window state is a OR’ed combination of
Qt::WindowState:Qt::WindowMinimized,Qt::WindowMaximized,Qt::WindowFullScreen, andQt::WindowActive.
IMO, comparing windowState() without bitmasking is error prone.
Could it be just:
minimize_action->setEnabled(isMinimized());
QWindow now.
400+ QObject::connect(windowHandle(), &QWindow::windowStateChanged, [this, minimize_action] {
401+ minimize_action->setEnabled(windowHandle()->windowState() != Qt::WindowMinimized);
402+ });
403+#ifdef Q_OS_MAC
404+ window_menu->addAction(tr("Zoom"), [this] {
405+ if (windowHandle()->windowState() == Qt::WindowMaximized) {
if (isMaximized()) {
tACK 6c72f91
A few nits, but don’t let that stop anyone from merging:
./configure --disable-wallet or disablewallet=1 you could still show Information, Console, Network Traffic and Peers in the Window menu. There may be some accessibility advantage to that.
- you could move Sign & Verify Message to Window (they do need ellipses)
I’ll leave that for later after we decide about top level menu “Wallet”.
- I don’t find the Zoom item very useful, but don’t mind it either (I’m just not a Bitcoin Core Window Maximalist)
Minimize and Zoom operate on the focus window, which can be the main window, address book dialogs, debug window..
- when
./configure --disable-walletordisablewallet=1you could still show Information, Console, Network Traffic and Peers in the Window menu.
I’ll update with this suggestion, thanks @Sjors.
Considering https://stackoverflow.com/a/637708 the ellipsis in these
menu actions should be removed.
tACK 9ea38d0
0git range-diff `git merge-base --all HEAD 6c72f91`...6c72f91 HEAD~3...HEAD
Tested ACK https://github.com/bitcoin/bitcoin/pull/14573/commits/9ea38d022281713e7f79a219b37651ac5648d695 on windows 10
I’m a little confused about the “Main Window” option, isn’t the menu part of the main window implying that it is already focused? Also perhaps “Restore” should toggle between “Maximise” & “Restore”?
@MeshCollider thanks!
This was inspired on macos since it has a menu bar per application, not per window. Forgot that in windows it has a menu bar in each window..
So next changes will be:
Hello, I also have a question on that button Main Window on windows bitcoin. If it has no purpose please remove that button before the official release to avoid confusions for windows users.
Thank you.
Please remove this button:
@molxyz from the above #14573 (comment)
- then comes the main windows — we only allow 1 main window for now — and this is used to bring it to front (I’ll remove in windows, maybe linux too?)
Sorry for forgetting about this, but at this stage I’m afraid it won’t be removed for 0.18 (unless we have to do RC5).