Paste button in Open URI dialog #319

kristapsk · 2021-05-05T11:43:33Z

Picking up bitcoin/bitcoin#17955, with some review comments addressed.

jarolrod

Concept ACK

While this looks ok on Linux:

macOS behaves differently when appropriate maximumSize property with values for width and height are not set on Qt > 5.9, iirc correctly from my testing this was not a problem when we were using Qt 5.9.8:

You'll want to play around with width and height until you get something that looks good.

src/qt/forms/openuridialog.ui

hebasto

Concept ACK.

The icon is set both in the *.ui and in the *.cpp files. Is this required?

src/qt/openuridialog.h

src/qt/openuridialog.cpp

kristapsk · 2021-05-06T14:17:26Z

You'll want to play around with width and height until you get something that looks good.

I don't have a Mac where to test that.

hebasto · 2021-05-09T15:46:22Z

Btw, the following patch

--- a/src/qt/forms/openuridialog.ui
+++ b/src/qt/forms/openuridialog.ui
@@ -81,7 +81,9 @@
    <header>qt/qvalidatedlineedit.h</header>
   </customwidget>
  </customwidgets>
- <resources/>
+ <resources>
+  <include location="../bitcoin.qrc"/>
+ </resources>
  <connections>
   <connection>
    <sender>buttonBox</sender>

makes the added icon available in Qt Designer.

hebasto · 2021-05-09T16:24:24Z

You'll want to play around with width and height until you get something that looks good.

I don't have a Mac where to test that.

This issue could be fixed by replacing QPushButton with QToolButton.
Also suggesting to use 22x22 icon size for consistency with other cases of this icon usage.

In total:

diff --git a/src/qt/forms/openuridialog.ui b/src/qt/forms/openuridialog.ui
index 981d9255e..15c0a440e 100644
--- a/src/qt/forms/openuridialog.ui
+++ b/src/qt/forms/openuridialog.ui
@@ -31,7 +31,7 @@
       </widget>
      </item>
      <item>
-      <widget class="QPushButton" name="pasteButton">
+      <widget class="QToolButton" name="pasteButton">
        <property name="toolTip">
         <string>Paste address from clipboard</string>
        </property>
@@ -41,10 +41,16 @@
        <property name="icon">
         <iconset resource="../bitcoin.qrc">
          <normaloff>:/icons/editpaste</normaloff>:/icons/editpaste</iconset>
-        </property>
-        <property name="shortcut">
-         <string>Alt+P</string>
-        </property>
+       </property>
+       <property name="iconSize">
+        <size>
+         <width>22</width>
+         <height>22</height>
+        </size>
+       </property>
+       <property name="shortcut">
+        <string>Alt+P</string>
+       </property>
       </widget>
      </item>
     </layout>
diff --git a/src/qt/openuridialog.cpp b/src/qt/openuridialog.cpp
index ef4678a6a..131be0f4f 100644
--- a/src/qt/openuridialog.cpp
+++ b/src/qt/openuridialog.cpp
@@ -8,7 +8,9 @@
 #include <qt/guiutil.h>
 #include <qt/sendcoinsrecipient.h>
 
+#include <QAbstractButton>
 #include <QClipboard>
+#include <QLineEdit>
 #include <QUrl>
 
 OpenURIDialog::OpenURIDialog(const PlatformStyle* platformStyle, QWidget* parent) :
@@ -16,8 +18,7 @@ OpenURIDialog::OpenURIDialog(const PlatformStyle* platformStyle, QWidget* parent
     ui(new Ui::OpenURIDialog)
 {
     ui->setupUi(this);
-    ui->pasteButton->setIcon(platformStyle->SingleColorIcon(":/icons/editpaste"));
-    QObject::connect(ui->pasteButton, &QPushButton::clicked, ui->uriEdit, &QLineEdit::paste);
+    QObject::connect(ui->pasteButton, &QAbstractButton::clicked, ui->uriEdit, &QLineEdit::paste);
 
     GUIUtil::handleCloseWindowShortcut(this);
 }

On macOS Big Sur 11.3.1 (20E241):

RandyMcMillan · 2021-05-09T17:23:57Z

It may be useful to truncate the send value so that the user still has to manually input a send value.
This may help prompt the user to verify the target address? To minimize any hijacking schemes?

I am also wondering if there is a way to block access to this dialogue from AppleScript events. May be a security issue?

jarolrod · 2021-05-11T20:46:49Z

@hebasto In regards to #319 (comment)

I like the approach, except the part where we remove the following line:

- ui->pasteButton->setIcon(platformStyle->SingleColorIcon(":/icons/editpaste"));

Without the above line and using:

- <resources/>
+ <resources>
+  <include location="../bitcoin.qrc"/>
+ </resources>

The icon will not be colorized white when using macOS dark mode

If we keep the line, the icon will be colorized white when starting in macOS dark mode

@kristapsk

I would suggest to change the shortcut to Ctrl+v

        <property name="shortcut">
-         <string>Alt+P</string>
+         <string>Ctrl+v</string>
        </property>

hebasto · 2021-05-12T15:59:56Z

I would suggest to change the shortcut to Ctrl+v

Why add any shortcut to the button, if Ctrl+v is the default shortcut for QLineEdit-based widgets?
It works right now, on master :)

jarolrod · 2021-05-13T00:14:37Z

Why add any shortcut to the button, if Ctrl+v is the default shortcut for QLineEdit-based widgets?
It works right now, on master :)

right 🤦. Let's remove the shortcut on the pasteButton

kristapsk · 2021-05-28T11:26:50Z

Addressed review comments above, this is ready for another review.

kristapsk · 2021-05-28T11:27:42Z

@RandyMcMillan Your comment is out of scope of this PR, this just adds visual button to existing paste functionality.

8b419b5 qt: make console buttons look clickable (Jarol Rodriguez) Pull request description: On master, for macOS, the console buttons' hitboxes are quite small. This makes clicking on the button with your mouse a little more tedious than it should be. The Issue is related to recent versions of Qt (>5.9.8) not playing so nice on macOS when there are "incorrect" `width` and `height` values set for a `QPushButton` (here is another example: bitcoin-core/gui#319 (review)). This fixes this small hitbox issue by converting the buttons from `QPushButton` to `QToolButton`, which in turn makes the buttons look explicitly clickable. This approach was chosen as it helps us avoid having to play around with `width` and `height` values until we find values that play nice with macOS and look good on Linux & Windows. Also, `QToolButton` is an appropriate class for these buttons. Per [Qt Docs](https://doc.qt.io/qt-5/qtoolbutton.html#details): > A tool button is a special button that provides quick-access to specific commands or options. As opposed to a normal command button, a tool button usually doesn't show a text label, but shows an icon instead. Since we are changing the type of the buttons, we need to change the respective actions connection logic in `rpcconsole`. Instead of plugging in `QToolButton`, we abstract it to the base class: `QAbstractButton`. per [Qt Dev Notes](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Developer-Notes-for-Qt-Code#inherited-signals-and-slot) > Use base class functions as this makes the code more general, e.g., use QAbstractButton::clicked instead of QPushButton::clicked. While here, we also update the size of the icons to `22x22` to be consistent with other tool buttons. **macOS: Master vs PR:** | Master | PR | | ----------- | ----------- | | ![master-ss-macos](https://user-images.githubusercontent.com/23396902/118339460-e9079c80-b4e6-11eb-864b-d394aca5df61.png) | ![pr-ss-macos](https://user-images.githubusercontent.com/23396902/118339468-ec9b2380-b4e6-11eb-9a9e-30620216750e.png) | **Linux: Master vs PR:** | Master | PR | | ----------- | ----------- | | ![master-ss-linux](https://user-images.githubusercontent.com/23396902/118339520-13595a00-b4e7-11eb-86d0-96dd1264c198.png) | ![pr-ss-linux](https://user-images.githubusercontent.com/23396902/118339533-1c4a2b80-b4e7-11eb-8d7f-f733d999c8fd.png) | ACKs for top commit: hebasto: ACK 8b419b5, tested on Linux Mint 20.1 (Qt 5.12.8). promag: Tested ACK 8b419b5 on macOS Big Sur M1, this drops only relevant usages to `flat` buttons. Tree-SHA512: 3f3cdcbe83398136a1d1ee8fc2835be8681f2ed39e79db1e939cab6a00a779f528343d54992807a845cc84d9ef13591affb7a6dbca9e5753a2b8665b0af4d611

hebasto · 2021-06-06T10:06:45Z

After #275 is merged, we should be sure that every new added widget supports runtime appearance adjustment on macOS. That is not the case now:

Also there is no need to adjust copyright year here, because we have a dedicated script which is ran once per year.

src/qt/openuridialog.cpp

kristapsk · 2021-06-30T22:13:33Z

Addressed comments by @hebasto and @luke-jr.

kristapsk · 2021-07-01T08:26:50Z

Modified changeEvent to match #366.

jarolrod

in regards to #319 (comment):

Modified changeEvent to match #366.

This doesn't include #275. Can you rebase to include this. This allows for easier testing and might be a cause for the CI failure.

src/qt/openuridialog.cpp

src/qt/forms/openuridialog.ui

shaavan

Concept ACK
Successfully compiled and tested on Ubuntu 20.04

This PR adds a button to the openbitcoinuri dialog box that allows the pasting of URI in the QLineEdit box. This is done by adding a button to the openbitcoinuri dialog box, an object of the platformStyle class, which allows setting a custom icon on the button. Finally, the pasting of URI is connected with pressing of this button, resulting in given the button its functionality.
I am adding a screenshot to show the correct working of PR.

As already mentioned by @jarolrod, OP should fix the CI formatting. Also, OP should rebase this PR on the current master head to allow the proper test of the UI changing on macOS

Co-authored-by: Emil Engler <me@emilengler.com> Co-authored-by: =?UTF-8?q?Jo=C3=A3o=20Barbosa?= <joao.paulo.barbosa@gmail.com> Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Co-authored-by: Jarol Rodriguez <jarolrod@tutanota.com>

kristapsk · 2021-09-26T00:33:57Z

Addressed additional review comments, this ready for another review.

shaavan

tACK dbde055
Changes since my last review:

The formatting of the code has been fixed
The PR is rebased over the current master

I retested this PR, and the paste button is working perfectly.

Though it might just be because of the screenshot software difference, but the paste button looks slightly different since I last reviewed this PR.

Old Commit	Latest Commit

Btw if it is a deliberate change, the new button looks a lot better!

jarolrod

ACK dbde055

Tested on macOS 12 and on Ubuntu. Confirmed that this works as intended. Also confirmed that this responds to macOS theme switching.

Start Dark	Switch to Light

promag

Tested ACK dbde055.

jarolrod reviewed May 6, 2021

View reviewed changes

src/qt/forms/openuridialog.ui Outdated Show resolved Hide resolved

hebasto reviewed May 6, 2021

View reviewed changes

src/qt/openuridialog.h Outdated Show resolved Hide resolved

src/qt/openuridialog.h Outdated Show resolved Hide resolved

src/qt/openuridialog.cpp Outdated Show resolved Hide resolved

hebasto added UI All about "look and feel" UX All about "how to get things done" labels May 9, 2021

hebasto changed the title ~~gui: Paste button in Open URI dialog~~ Paste button in Open URI dialog May 9, 2021

jarolrod mentioned this pull request May 14, 2021

Make console buttons look clickable #329

Merged

kristapsk force-pushed the uri-paste branch from 38e2a54 to 84f23e8 Compare May 28, 2021 11:25

luke-jr reviewed Jun 19, 2021

View reviewed changes

src/qt/openuridialog.cpp Outdated Show resolved Hide resolved

kristapsk force-pushed the uri-paste branch 2 times, most recently from b14c6fc to 980adfe Compare June 30, 2021 22:11

kristapsk force-pushed the uri-paste branch from 980adfe to 5062565 Compare July 1, 2021 08:25

jarolrod reviewed Aug 5, 2021

View reviewed changes

src/qt/openuridialog.cpp Outdated Show resolved Hide resolved

src/qt/forms/openuridialog.ui Outdated Show resolved Hide resolved

shaavan reviewed Aug 11, 2021

View reviewed changes

kristapsk force-pushed the uri-paste branch from 5062565 to dbde055 Compare September 26, 2021 00:33

shaavan approved these changes Sep 27, 2021

View reviewed changes

jarolrod reviewed Nov 2, 2021

View reviewed changes

promag reviewed Nov 9, 2021

View reviewed changes

hebasto merged commit 79e64a0 into bitcoin-core:master Nov 21, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 22, 2021

Merge bitcoin-core/gui#319: Paste button in Open URI dialog

0528061

kristapsk deleted the uri-paste branch November 22, 2021 21:36

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 23, 2021

Merge bitcoin-core/gui#319: Paste button in Open URI dialog

41cc657

bitcoin-core locked and limited conversation to collaborators Nov 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Paste button in Open URI dialog #319

Paste button in Open URI dialog #319

kristapsk commented May 5, 2021

jarolrod left a comment •

edited

Loading

hebasto left a comment

kristapsk commented May 6, 2021

hebasto commented May 9, 2021

hebasto commented May 9, 2021

RandyMcMillan commented May 9, 2021

jarolrod commented May 11, 2021

hebasto commented May 12, 2021 •

edited

Loading

jarolrod commented May 13, 2021

kristapsk commented May 28, 2021

kristapsk commented May 28, 2021

hebasto commented Jun 6, 2021

kristapsk commented Jun 30, 2021

kristapsk commented Jul 1, 2021

jarolrod left a comment

shaavan left a comment

kristapsk commented Sep 26, 2021

shaavan left a comment

jarolrod left a comment

promag left a comment

Paste button in Open URI dialog #319

Paste button in Open URI dialog #319

Conversation

kristapsk commented May 5, 2021

jarolrod left a comment • edited Loading

Choose a reason for hiding this comment

hebasto left a comment

Choose a reason for hiding this comment

kristapsk commented May 6, 2021

hebasto commented May 9, 2021

hebasto commented May 9, 2021

RandyMcMillan commented May 9, 2021

jarolrod commented May 11, 2021

hebasto commented May 12, 2021 • edited Loading

jarolrod commented May 13, 2021

kristapsk commented May 28, 2021

kristapsk commented May 28, 2021

hebasto commented Jun 6, 2021

kristapsk commented Jun 30, 2021

kristapsk commented Jul 1, 2021

jarolrod left a comment

Choose a reason for hiding this comment

shaavan left a comment

Choose a reason for hiding this comment

kristapsk commented Sep 26, 2021

shaavan left a comment

Choose a reason for hiding this comment

jarolrod left a comment

Choose a reason for hiding this comment

promag left a comment

Choose a reason for hiding this comment

jarolrod left a comment •

edited

Loading

hebasto commented May 12, 2021 •

edited

Loading