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.

Sign up for GitHub

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

Jump to bottom

Add OptionButton control #67

Merged
merged 7 commits into from
Nov 3, 2021
Merged

Add OptionButton control #67

merged 7 commits into from
Nov 3, 2021

Conversation

promag
Copy link
Contributor

@promag promag commented Oct 22, 2021 •
edited
Loading

image

Windows
macOS

@hebasto
Copy link
Member

hebasto commented Oct 22, 2021

Rebase on top of the merged #49?

@hebasto
Copy link
Member

hebasto commented Oct 22, 2021

Concept ACK. Looks great!

@hebasto hebasto added the UX Designers' opinions are required label Oct 22, 2021
@promag promag force-pushed the 2021-10-option-button branch 2 times, most recently from a6c389f to 99d4f66 Compare October 22, 2021 17:19
@promag
Copy link
Contributor Author

promag commented Oct 22, 2021 •
edited
Loading

Rebased and split to a new component (ConnectionOptions) where OptionButton is used.

@hebasto
Copy link
Member

hebasto commented Oct 22, 2021

Tested 99d4f66:

qrc:/qml/pages/stub.qml:22:5: QML ColumnLayout: Possible anchor loop detected on centerIn.

@promag
Copy link
Contributor Author

promag commented Oct 22, 2021

@hebasto thanks, didn't notice it, will fix.

@promag promag force-pushed the 2021-10-option-button branch from 99d4f66 to 653d7cd Compare October 22, 2021 21:47
@promag
Copy link
Contributor Author

promag commented Oct 22, 2021

@hebasto see 2390239 description.

@promag
Copy link
Contributor Author

promag commented Oct 22, 2021

0b5fb6b is included so that you can test by pressing TAB to switch between options and SPACE to select it (we don't have focus feedback yet).

@hebasto
Copy link
Member

hebasto commented Oct 27, 2021 •
edited
Loading

0b5fb6b is included so that you can test by pressing TAB to switch between options and SPACE to select it (we don't have focus feedback yet).

Does not work for me on Linux. Only mouse clicks work.

@hebasto
Copy link
Member

hebasto commented Oct 27, 2021

2acd4a7 "qml: Drop import qualifier BitcoinCoreComponents"

Wouldn't it be useful to keep it as is in case if we use import "../components" and import "../controls" in the same QML file?

@promag
Copy link
Contributor Author

promag commented Oct 27, 2021

0b5fb6b is included so that you can test by pressing TAB to switch between options and SPACE to select it (we don't have focus feedback yet).

Does not work for me on Linux. Only mouse clicks work.

Built on ubuntu with depends and works by pressing TAB and SPACE. I'll include focus feedback since @GBKS already has a suggestion for that.

@promag
Copy link
Contributor Author

promag commented Oct 27, 2021

2acd4a7 "qml: Drop import qualifier BitcoinCoreComponents"

Wouldn't it be useful to keep it as is in case if we use import "../components" and import "../controls" in the same QML file?

That should be fine, I don't expect name collisions - at least I'd like to avoid ambiguities.

@promag promag force-pushed the 2021-10-option-button branch from 653d7cd to 4c4aa30 Compare October 27, 2021 22:25
@promag
Copy link
Contributor Author

promag commented Oct 27, 2021

Now with hover and focus feedback.

Rebased on top of #73 to have the macOS artifact.

@promag promag force-pushed the 2021-10-option-button branch 2 times, most recently from e143115 to 6af285e Compare October 28, 2021 14:47
@promag
Copy link
Contributor Author

promag commented Oct 28, 2021

Rebased, made adjustments suggested by @GBKS, made ConnectionOptions extend Pane to better handle keyboard focus, set 1st option as the default control to have keyboard focus.

@promag promag force-pushed the 2021-10-option-button branch from 6af285e to b70ee53 Compare October 29, 2021 08:23
@promag
Copy link
Contributor Author

promag commented Oct 29, 2021

Rebased on #74.

@promag promag force-pushed the 2021-10-option-button branch 2 times, most recently from b70ee53 to 7cde50b Compare October 29, 2021 13:45
hebasto
hebasto reviewed Oct 30, 2021
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested 7cde50b on Linux Mint 20.2 (Qt 5.12.8).

Tab (focus moving) and Space (focus selecting) work as expected.

What should be a role of the Return key in focus treatment?

Also:

$ src/qt/bitcoin-qt -signet
qrc:/qml/pages/stub.qml:34:9: QML Pane: Binding loop detected for property "implicitHeight"
qrc:/qml/pages/stub.qml:34:9: QML Pane: Binding loop detected for property "implicitHeight"

@promag
Copy link
Contributor Author

promag commented Oct 30, 2021

Thanks @hebasto, will address both comments.

promag added 2 commits November 2, 2021 10:41
@promag
qml: Use window color instead of background
cd5527f 
Follow Qt recomendation (1) since the background is a solid color.

1. 
https://doc.qt.io/qt-5/qml-qtquick-controls2-applicationwindow.html#background-prop
@promag
qml: Remove explicit height of BlockCounter
6bedd3d 
Label implicit height depends on the font size but the font size depends
on the label height. This results in a runtime anchor loop warning.
@promag promag force-pushed the 2021-10-option-button branch from 7cde50b to 8ab1626 Compare November 2, 2021 10:57
@promag
Copy link
Contributor Author

promag commented Nov 2, 2021

Rebased and fixed warning. Now using visualFocus instead of activeFocus. Clicking or touching doesn't show focus border, only if the keyboard is used.

hebasto
hebasto approved these changes Nov 2, 2021
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 8ab1626

@promag promag force-pushed the 2021-10-option-button branch from 8ab1626 to 61668e9 Compare November 2, 2021 15:14
hebasto
hebasto approved these changes Nov 2, 2021
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-ACK 61668e9

jarolrod
jarolrod reviewed Nov 2, 2021
View reviewed changes
Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 61668e9

pending on ACK from our fellow designers. Pinging @Bosch-0 @GBKS

src/qml/pages/stub.qml
@@ -5,33 +5,35 @@
import QtQuick 2.12
import QtQuick.Controls 2.12
import QtQuick.Layouts 1.11
import "../components" as BitcoinCoreComponents

import "../components"

ApplicationWindow {
id: appWindow
title: "Bitcoin Core TnG"
minimumWidth: 750
minimumHeight: 450
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have any plan on how to set the minimumHeight to correspond to the required minimum size of the contents?
Screen Shot 2021-11-02 at 7 27 24 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should set these once we settle down the main/window layout.

@Bosch-0
Copy link
Contributor

Bosch-0 commented Nov 3, 2021

ACK, LGTM on windows, this will be siloed to the onboarding / settings sections at a later stage correct?

@hebasto hebasto merged commit b72a31e into bitcoin-core:main Nov 3, 2021
@promag promag deleted the 2021-10-option-button branch November 3, 2021 09:17
hebasto added a commit that referenced this pull request Dec 2, 2021
@hebasto
Merge #87: Sync with the main repo
4b7051c 
Pull request description:

  Sync with the main repo up to bitcoin/bitcoin@c9d7d0a

  To reproduce:

  1) There are merge conflicts. To remedy this, the following commits from the following PR's must be reverted. They have been reverted from newest to oldest as listed:

  ```
  - PR #82 (Nov 12)
    - 83f1893
    - 0f5d210
  - PR #79 (Nov 5)
    - 011fcff
  - PR #69 (Nov 4)
    - 1bf3d5d
  - PR #77 (Nov 3)
    - 7dc95a0
    - 48fb724
  - PR #67 (Nov 3)
    - 61668e9
    - 37cfe4e
    - 5b00612
  - PR #65 (Oct 30)
    - 500a6d2
  - PR #74 (Oct 29)
    - 319db69
  - PR #72 (Oct 27)
    - 749470f
  - PR #55 (Oct 10)
    - a13a868
  - PR #45 (Oct 3)
    - 5ce77ad
  - PR #44 (Oct 3)
    - 6521cb8
  - PR #30 (Aug 30)
    - 3a0f948
    - f17a32a
  - PR #19 (Aug 19)
    - 8793ae7
    - 3ddb9b3
    - e61d42d
  ```

  2) Fetch the upstream commit
  ```
  git fetch https://github.com/bitcoin/bitcoin
  git merge c9d7d0a
  ```

  3) Cherry-pick back the reverted commits from oldest to newest. Note that I have left out 48fb724 and 3ddb9b3 from cherry-picking back in as it is unnecessary and leads to a cleaner commit history.

  ## GUIX HASHES
  ```
  $ find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum

  6a9def1ba270b4e987c44194bf2704899da603fb25860063fc5ff22ee3b2168b  guix-build-ef99bc925033/output/aarch64-linux-gnu/SHA256SUMS.part
  a5750927e2d5a9086ae70b0279e6d06a48602b1b7b535fe2d1f743d0d320c1b6  guix-build-ef99bc925033/output/aarch64-linux-gnu/bitcoin-ef99bc925033-aarch64-linux-gnu-debug.tar.gz
  ec1a7e9750df2d00dbb420842ef17813e6f825d06e2122be031c6daf977a9714  guix-build-ef99bc925033/output/aarch64-linux-gnu/bitcoin-ef99bc925033-aarch64-linux-gnu.tar.gz
  74aa6e3330956e0d4445c2989056fa807e3dba9a35ec2b38b84c48c0f9bd3efb  guix-build-ef99bc925033/output/arm-linux-gnueabihf/SHA256SUMS.part
  1c1db52f0bcf91e8c27b150e4c9de3cd235d565e3cdb499dfec8433b4da2119a  guix-build-ef99bc925033/output/arm-linux-gnueabihf/bitcoin-ef99bc925033-arm-linux-gnueabihf-debug.tar.gz
  9bb58c56527c28b1730dadbf9b169527e056161e30a7cf758b53fdb565f98cdb  guix-build-ef99bc925033/output/arm-linux-gnueabihf/bitcoin-ef99bc925033-arm-linux-gnueabihf.tar.gz
  325a29527468d7a5686dbcd31602cbae01a41f6cd324680df311815d976f9ca2  guix-build-ef99bc925033/output/dist-archive/bitcoin-ef99bc925033.tar.gz
  6e2adbd2a9298f335332b1661e813c9a208a76ad45a37244791756d3296b278d  guix-build-ef99bc925033/output/powerpc64-linux-gnu/SHA256SUMS.part
  e1402abb77216fe1325625404ca8c909b8229f96f58154828c04bb7f699eb9e9  guix-build-ef99bc925033/output/powerpc64-linux-gnu/bitcoin-ef99bc925033-powerpc64-linux-gnu-debug.tar.gz
  080ef0d1b1361fbb2354e24d100a49f63ed914afccc69792edd4674ba7d02bb0  guix-build-ef99bc925033/output/powerpc64-linux-gnu/bitcoin-ef99bc925033-powerpc64-linux-gnu.tar.gz
  e6fb710af41df7be3ae6029ef1114707e0516d551beae0a9928c9de84a025563  guix-build-ef99bc925033/output/powerpc64le-linux-gnu/SHA256SUMS.part
  3bb370f79e6fb55ff50ea8db3b49ed7303fce2b9e49af6d0722821b049bfcee4  guix-build-ef99bc925033/output/powerpc64le-linux-gnu/bitcoin-ef99bc925033-powerpc64le-linux-gnu-debug.tar.gz
  1ed30ff64eacab08f724e521266784ae21c6ff879cbde7baad21a4b6ceb8ef82  guix-build-ef99bc925033/output/powerpc64le-linux-gnu/bitcoin-ef99bc925033-powerpc64le-linux-gnu.tar.gz
  dadebd336d5a359105bc4f757f60eba7651ed068a0a8d196ca7a18fa22e692c7  guix-build-ef99bc925033/output/riscv64-linux-gnu/SHA256SUMS.part
  208082743c43c37cb6907fe60ddea1909271204188763ee7b214673558f3c18e  guix-build-ef99bc925033/output/riscv64-linux-gnu/bitcoin-ef99bc925033-riscv64-linux-gnu-debug.tar.gz
  a28abc53c3a6cf7ddbd7177e3aae5a4636301c2fb18a0a361cb05d98ef23ea68  guix-build-ef99bc925033/output/riscv64-linux-gnu/bitcoin-ef99bc925033-riscv64-linux-gnu.tar.gz
  6f3c8b2fc3fa4638d8adf08f53dce07a9d96a2ef01db1df74ef53799f804ddb2  guix-build-ef99bc925033/output/x86_64-apple-darwin19/SHA256SUMS.part
  46160e6460bd1dfd8943f48199daacf58f8e8249edc3757ab681978d72490db9  guix-build-ef99bc925033/output/x86_64-apple-darwin19/bitcoin-ef99bc925033-osx-unsigned.dmg
  75f45e27858931ee91201962d1ffa8cf7998da3c962c672403ac8f8888bda7f2  guix-build-ef99bc925033/output/x86_64-apple-darwin19/bitcoin-ef99bc925033-osx-unsigned.tar.gz
  ee466fca21a6353d2aa6e85be5ac14192ce71430f87c54e8c406398fdeedd73f  guix-build-ef99bc925033/output/x86_64-apple-darwin19/bitcoin-ef99bc925033-osx64.tar.gz
  44de818d70a295fcd8488142b76a893cd94126ee3aa69d0bfc02096231cfac8c  guix-build-ef99bc925033/output/x86_64-linux-gnu/SHA256SUMS.part
  bb2bc967836cf11bf0412ecc64c1cfbd4076199ac1c1a828919a11802bbd9a4c  guix-build-ef99bc925033/output/x86_64-linux-gnu/bitcoin-ef99bc925033-x86_64-linux-gnu-debug.tar.gz
  52942d2ba01535b645c6d0460f074a6d683896082fe81b2f31addfc425a631a6  guix-build-ef99bc925033/output/x86_64-linux-gnu/bitcoin-ef99bc925033-x86_64-linux-gnu.tar.gz
  e437ff82adb1ac30ca9a27bd1732d14f9b17bfa2c9f017aeccadd19685daaf95  guix-build-ef99bc925033/output/x86_64-w64-mingw32/SHA256SUMS.part
  c4d2ed7580949cc6d4207b57429651a46cc023381b81408adfe4337f360c2be6  guix-build-ef99bc925033/output/x86_64-w64-mingw32/bitcoin-ef99bc925033-win-unsigned.tar.gz
  e5116106e634d0e1d6a7f509c645de81d19bd7b91243d94a6f90d893ca141016  guix-build-ef99bc925033/output/x86_64-w64-mingw32/bitcoin-ef99bc925033-win64-debug.zip
  005710c9e3032dfdc6117a77c38f85a2a5fa723b32772bc33c907298fe91ebfa  guix-build-ef99bc925033/output/x86_64-w64-mingw32/bitcoin-ef99bc925033-win64-setup-unsigned.exe
  c85bb2671820715a97273cc57b81de73f50f95fd8c8909020b08a6be02eb4797  guix-build-ef99bc925033/output/x86_64-w64-mingw32/bitcoin-ef99bc925033-win64.zip
  ```

  [![Windows](https://img.shields.io/badge/OS-Windows-green)](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/win64/insecure_win_gui.zip?branch=pull/87)
  [![macOS](https://img.shields.io/badge/OS-macOS-green)](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/macos/insecure_mac_gui.zip?branch=pull/87)
  [![Android](https://img.shields.io/badge/OS-Android-green)](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/android/insecure_android_apk.zip?branch=pull/87)

ACKs for top commit:
  hebasto:
    ACK ef99bc9

Tree-SHA512: abefdfb363d5ec51c0e4eb079b14e8befd1e921fc19beea210f34f70e80de84c33e4e2e68145eff6cb7e516e73ca2efb0f07fdeafaf440488950170be03ee3a1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jarolrod jarolrod jarolrod left review comments

@hebasto hebasto hebasto approved these changes

Assignees
No one assigned
Labels
UX Designers' opinions are required
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@promag @hebasto @Bosch-0 @jarolrod