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

qt: Add Window menu #14573

Merged
merged 3 commits into from Dec 16, 2018

Conversation

Projects
None yet
@promag
Copy link
Member

promag commented Oct 25, 2018

Overall this PR does the following:

  • add top level menu Window
  • add Minimize and Zoom actions to Window menu
  • move Sending/Receiving address to Window
  • remove Help->Debug window
  • add one menu entry for each debug window tab

This removes the access to address book from the File menu.

With wallet support:
screenshot 2018-12-11 at 00 33 05

Without wallet support:
screenshot 2018-12-11 at 12 55 21

@fanquake fanquake added the GUI label Oct 25, 2018

@ken2812221
Copy link
Member

ken2812221 left a comment

Concept ACK

Show resolved Hide resolved src/qt/bitcoingui.cpp Outdated
@hebasto

This comment has been minimized.

Copy link
Contributor

hebasto commented Oct 28, 2018

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.

@promag promag changed the title qt: Add Wallet and Window menus qt: Add Window menus Oct 30, 2018

@promag promag changed the title qt: Add Window menus qt: Add Window menu Oct 30, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Oct 30, 2018

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #14879 (qt: Add warning messages to the debug window by hebasto)
  • #9849 (Qt: Network Watch tool by luke-jr)

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 promag referenced this pull request Oct 30, 2018

Open

Qt: Network Watch tool #9849

@promag promag force-pushed the promag:2018-10-overhaul-menus branch 2 times, most recently from 039c146 to 3f779b3 Oct 31, 2018

@promag

This comment has been minimized.

Copy link
Member

promag commented Oct 31, 2018

Removed the changes to the "Settings" menu.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Nov 12, 2018

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 Window menu, it doesn' seem window related functionality but wallet related functionality! oh, because it's an auxiliary window

@MeshCollider

This comment has been minimized.

Copy link
Member

MeshCollider commented Nov 12, 2018

Concept ACK, I would slightly prefer the name "View" but this is fine with me too

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Nov 13, 2018

Tested a bit.
I think the "minimize" entry should toggle to "maximise" once minimized or at least maximises when selecting "minimize" in already minimized state.
Rest looks fine.

@promag

This comment has been minimized.

Copy link
Member

promag commented Nov 13, 2018

Thanks @jonasschnelli, I'll see if others also toggle.

I have to test in windows.

@promag

This comment has been minimized.

Copy link
Member

promag commented Nov 14, 2018

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

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Nov 19, 2018

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

@hebasto

This comment has been minimized.

Copy link
Contributor

hebasto commented Nov 22, 2018

IMO, it would be better to avoid using native widgets (Native Widgets vs Alien Widgets) and related methods (QWidget::windowHandle()) and objects (QWindow).

@Sjors

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 3f779b3.

Show resolved Hide resolved src/qt/bitcoingui.cpp Outdated
Show resolved Hide resolved src/qt/bitcoingui.cpp Outdated
@promag

This comment has been minimized.

Copy link
Member

promag commented Dec 11, 2018

Updated after feedback, thank you! Also updated screenshot. Windows/linux feedback would be awesome.

@promag promag force-pushed the promag:2018-10-overhaul-menus branch from 3f779b3 to 46cbcb0 Dec 11, 2018

@promag

This comment has been minimized.

Copy link
Member

promag commented Dec 11, 2018

@Sjors

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)

I think in the future we could have multiple windows, each could have multiple wallets.

@promag

This comment has been minimized.

Copy link
Member

promag commented Dec 11, 2018

Pushed a commit to remove ellipsis from the sending/receiving addresses menu actions. Please see rationale in https://stackoverflow.com/a/637708 (also updated screenshot)

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Dec 11, 2018

tACK 6c72f91

A few nits, but don't let that stop anyone from merging:

  • you could move Sign & Verify Message to Window (they do need ellipses)
  • I don't find the Zoom item very useful, but don't mind it either (I'm just not a Bitcoin Core Window Maximalist)
  • when ./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.
@promag

This comment has been minimized.

Copy link
Member

promag commented Dec 11, 2018

  • 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-wallet or disablewallet=1 you could still show Information, Console, Network Traffic and Peers in the Window menu.

I'll update with this suggestion, thanks @Sjors.

@promag promag force-pushed the promag:2018-10-overhaul-menus branch from 6c72f91 to 4768926 Dec 11, 2018

@promag promag force-pushed the promag:2018-10-overhaul-menus branch from 4768926 to 95a5a9f Dec 11, 2018

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Dec 11, 2018

tACK 9ea38d0

git range-diff `git merge-base --all HEAD 6c72f91`...6c72f91 HEAD~3...HEAD

schermafbeelding 2018-12-11 om 17 39 04

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Dec 11, 2018

Tested ACK 9ea38d0

@promag

This comment has been minimized.

Copy link
Member

promag commented Dec 13, 2018

@ken2812221 do you think you have the time to test this?

@MeshCollider

This comment has been minimized.

Copy link
Member

MeshCollider commented Dec 14, 2018

Tested ACK 9ea38d0 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"?

image

@promag

This comment has been minimized.

Copy link
Member

promag commented Dec 14, 2018

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

  • first section are actions on the current window (focused window on macos)
  • 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?)
  • then comes some dialogs — address book for now
  • then comes the debug dialogs
@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Dec 15, 2018

Rather than explicitly handling this per OS, does QT have any way to abstract that?

@promag

This comment has been minimized.

Copy link
Member

promag commented Dec 15, 2018

Not that I'm aware of.

@jonasschnelli jonasschnelli merged commit 95a5a9f into bitcoin:master Dec 16, 2018

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 Dec 16, 2018

Merge #14573: qt: Add Window menu
95a5a9f qt: Remove ellipsis from sending/receiving addresses (João Barbosa)
a96c0df qt: Add Window menu (João Barbosa)
9ea38d0 qt: Allow to inspect RPCConsole tabs (João Barbosa)

Pull request description:

  Overall this PR does the following:
   - add top level menu Window
   - add Minimize and Zoom actions to Window menu
   - move Sending/Receiving address to Window
   - remove Help->Debug window
   - add one menu entry for each debug window tab

  This removes the access to address book from the File menu.

  With wallet support:
  <img width="522" alt="screenshot 2018-12-11 at 00 33 05" src="https://user-images.githubusercontent.com/3534524/49770451-5bec0800-fcdc-11e8-91d6-f8f850ead92d.png">

  Without wallet support:
  <img width="593" alt="screenshot 2018-12-11 at 12 55 21" src="https://user-images.githubusercontent.com/3534524/49802183-19f6ac80-fd44-11e8-9973-36fcfb4f129e.png">

Tree-SHA512: 4fb03702efe18df7bae33950e462940162abe634c55d0214b8920812127b763234cc9b73f27b3702502a37b6d49bdd6c50b7c8d9a3daea75cecb0136556dd1ea

@fanquake fanquake removed this from Blockers in High-priority for review Dec 16, 2018

@promag promag deleted the promag:2018-10-overhaul-menus branch Dec 16, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Dec 17, 2018

This does not compile because the addAction function was only introduced in Qt 5.6.

@Empact

This comment has been minimized.

Copy link
Member

Empact commented Dec 17, 2018

An example of the failure: https://travis-ci.org/bitcoin/bitcoin/jobs/468706526
Min Qt version is 5.2, haven't found those docs readily available but the 5.5 QMenu api is here:
http://doc.qt.io/archives/qt-5.5/qmenu.html

laanwj added a commit that referenced this pull request Dec 17, 2018

Merge #14979: [Qt] Restore < Qt5.6 compatibility for addAction
3e21b69 [Qt] Restore < Qt5.6 compatibility for addAction (Jonas Schnelli)

Pull request description:

  #14573 broke < Qt5.6 compatibility due to calling the lambda version of `addAction` that was added in Qt5.6.

  This PR re-enables < Qt5.6 compatibility.

Tree-SHA512: b3cf055d88a76713d100be05b2298d4091967e1a43de176af2647f59e76b98b216493dd12a6d68a942ae7946f2026e33dd8e8d20fc44a9a9614a3690ad9a2417

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 24, 2018

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 24, 2018

qt: Add Window menu
Github-Pull: bitcoin#14573
Rebased-From: a96c0df

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 24, 2018

qt: Remove ellipsis from sending/receiving addresses
Considering https://stackoverflow.com/a/637708 the ellipsis in these
menu actions should be removed.

Github-Pull: bitcoin#14573
Rebased-From: 95a5a9f

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 26, 2018

qt: Add Window menu
Github-Pull: bitcoin#14573
Rebased-From: a96c0df

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 26, 2018

qt: Remove ellipsis from sending/receiving addresses
Considering https://stackoverflow.com/a/637708 the ellipsis in these
menu actions should be removed.

Github-Pull: bitcoin#14573
Rebased-From: 95a5a9f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment