Paste button in Open URI dialog

kristapsk commented at 11:43 am on May 5, 2021: contributor

Picking up https://github.com/bitcoin/bitcoin/pull/17955, with some review comments addressed.

in src/qt/forms/openuridialog.ui:46 in e5c92d7ddc outdated

41+       <property name="icon">
42+        <iconset resource="../bitcoin.qrc">
43+         <normaloff>:/icons/editpaste</normaloff>:/icons/editpaste</iconset>
44+        </property>
45+        <property name="shortcut">
46+         <string>Alt+P</string>

jarolrod commented at 2:20 am on May 6, 2021:

not sure how I feel about this shortcut. How intuitive/practical is this?

hebasto commented at 5:03 am on May 6, 2021:

https://github.com/bitcoin/bitcoin/pull/17955#discussion_r368223931 https://github.com/bitcoin/bitcoin/pull/17955#discussion_r368906725

promag commented at 12:59 pm on May 10, 2021:

NACK the shortcut, see #17955 review.

jarolrod commented at 2:20 am on May 6, 2021: member

Concept ACK

While this looks ok on Linux:

Screen Shot 2021-05-05 at 10 14 42 PM

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:

Screen Shot 2021-05-05 at 1 16 12 PM

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

in src/qt/openuridialog.h:10 in e5c92d7ddc outdated

 6@@ -7,6 +7,8 @@
 7 
 8 #include <QDialog>
 9 
10+#include <qt/platformstyle.h>

hebasto commented at 5:11 am on May 6, 2021:

https://github.com/bitcoin/bitcoin/pull/17955/files#r368907744:

Just forward declare PlatformStyle.

kristapsk commented at 2:19 pm on May 6, 2021:

You mean forward declare it here, and move this include to openuridialog.cpp? Otherwise there is “invalid use of incomplete type” compiler error.

hebasto commented at 3:15 pm on May 9, 2021:

You mean forward declare it here, and move this include to openuridialog.cpp?

Yeap.

in src/qt/openuridialog.h:21 in e5c92d7ddc outdated

17@@ -16,7 +18,7 @@ class OpenURIDialog : public QDialog
18     Q_OBJECT
19 
20 public:
21-    explicit OpenURIDialog(QWidget *parent);
22+    explicit OpenURIDialog(const PlatformStyle* platformStyle, QWidget *parent);

hebasto commented at 5:12 am on May 6, 2021:

style nit:

0    explicit OpenURIDialog(const PlatformStyle* platformStyle, QWidget* parent);

in src/qt/openuridialog.cpp:14 in e5c92d7ddc outdated

 7@@ -8,13 +8,16 @@
 8 #include <qt/guiutil.h>
 9 #include <qt/sendcoinsrecipient.h>
10 
11+#include <QClipboard>
12 #include <QUrl>
13 
14-OpenURIDialog::OpenURIDialog(QWidget *parent) :
15+OpenURIDialog::OpenURIDialog(const PlatformStyle *platformStyle, QWidget *parent) :

hebasto commented at 5:14 am on May 6, 2021:

style nit:

0OpenURIDialog::OpenURIDialog(const PlatformStyle* platformStyle, QWidget* parent) :

hebasto commented at 5:26 am on May 6, 2021: member

Concept ACK.

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

kristapsk commented at 2:17 pm on May 6, 2021: contributor

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 added the label UI on May 9, 2021

hebasto added the label UX on May 9, 2021

hebasto commented at 3:46 pm on May 9, 2021: member

Btw, the following patch

 0--- a/src/qt/forms/openuridialog.ui
 1+++ b/src/qt/forms/openuridialog.ui
 2@@ -81,7 +81,9 @@
 3    <header>qt/qvalidatedlineedit.h</header>
 4   </customwidget>
 5  </customwidgets>
 6- <resources/>
 7+ <resources>
 8+  <include location="../bitcoin.qrc"/>
 9+ </resources>
10  <connections>
11   <connection>
12    <sender>buttonBox</sender>

makes the added icon available in Qt Designer.

hebasto commented at 4:24 pm on May 9, 2021: member

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:

 0diff --git a/src/qt/forms/openuridialog.ui b/src/qt/forms/openuridialog.ui
 1index 981d9255e..15c0a440e 100644
 2--- a/src/qt/forms/openuridialog.ui
 3+++ b/src/qt/forms/openuridialog.ui
 4@@ -31,7 +31,7 @@
 5       </widget>
 6      </item>
 7      <item>
 8-      <widget class="QPushButton" name="pasteButton">
 9+      <widget class="QToolButton" name="pasteButton">
10        <property name="toolTip">
11         <string>Paste address from clipboard</string>
12        </property>
13@@ -41,10 +41,16 @@
14        <property name="icon">
15         <iconset resource="../bitcoin.qrc">
16          <normaloff>:/icons/editpaste</normaloff>:/icons/editpaste</iconset>
17-        </property>
18-        <property name="shortcut">
19-         <string>Alt+P</string>
20-        </property>
21+       </property>
22+       <property name="iconSize">
23+        <size>
24+         <width>22</width>
25+         <height>22</height>
26+        </size>
27+       </property>
28+       <property name="shortcut">
29+        <string>Alt+P</string>
30+       </property>
31       </widget>
32      </item>
33     </layout>
34diff --git a/src/qt/openuridialog.cpp b/src/qt/openuridialog.cpp
35index ef4678a6a..131be0f4f 100644
36--- a/src/qt/openuridialog.cpp
37+++ b/src/qt/openuridialog.cpp
38@@ -8,7 +8,9 @@
39 #include <qt/guiutil.h>
40 #include <qt/sendcoinsrecipient.h>
41 
42+#include <QAbstractButton>
43 #include <QClipboard>
44+#include <QLineEdit>
45 #include <QUrl>
46 
47 OpenURIDialog::OpenURIDialog(const PlatformStyle* platformStyle, QWidget* parent) :
48@@ -16,8 +18,7 @@ OpenURIDialog::OpenURIDialog(const PlatformStyle* platformStyle, QWidget* parent
49     ui(new Ui::OpenURIDialog)
50 {
51     ui->setupUi(this);
52-    ui->pasteButton->setIcon(platformStyle->SingleColorIcon(":/icons/editpaste"));
53-    QObject::connect(ui->pasteButton, &QPushButton::clicked, ui->uriEdit, &QLineEdit::paste);
54+    QObject::connect(ui->pasteButton, &QAbstractButton::clicked, ui->uriEdit, &QLineEdit::paste);
55 
56     GUIUtil::handleCloseWindowShortcut(this);
57 }

On macOS Big Sur 11.3.1 (20E241):

Screenshot from 2021-05-09 19-23-38

hebasto renamed this:
~~gui: Paste button in Open URI dialog~~
Paste button in Open URI dialog
on May 9, 2021

RandyMcMillan commented at 5:23 pm on May 9, 2021: contributor

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 commented at 8:46 pm on May 11, 2021: member

@hebasto In regards to #319 (comment)

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

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

Without the above line and using:

0- <resources/>
1+ <resources>
2+  <include location="../bitcoin.qrc"/>
3+ </resources>

The icon will not be colorized white when using macOS dark mode Screen Shot 2021-05-11 at 4 33 03 PM

If we keep the line, the icon will be colorized white when starting in macOS dark mode Screen Shot 2021-05-11 at 4 28 07 PM @kristapsk

I would suggest to change the shortcut to Ctrl+v

0        <property name="shortcut">
1-         <string>Alt+P</string>
2+         <string>Ctrl+v</string>
3        </property>

hebasto commented at 3:59 pm on May 12, 2021: member

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 commented at 0:14 am on May 13, 2021: member

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 force-pushed on May 28, 2021

kristapsk commented at 11:26 am on May 28, 2021: contributor

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

kristapsk commented at 11:27 am on May 28, 2021: contributor

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

hebasto referenced this in commit 916f45eba5 on Jun 5, 2021

sidhujag referenced this in commit a14397b062 on Jun 6, 2021

hebasto commented at 10:06 am on June 6, 2021: member

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:

Screenshot from 2021-06-06 13-06-15

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

in src/qt/openuridialog.cpp:13 in 84f23e8ec5 outdated

 9 #include <qt/guiutil.h>
10+#include <qt/platformstyle.h>
11 #include <qt/sendcoinsrecipient.h>
12 
13+#include <QAbstractButton>
14+#include <QClipboard>

luke-jr commented at 10:50 pm on June 19, 2021:

Is this needed?

kristapsk force-pushed on Jun 30, 2021

kristapsk commented at 10:13 pm on June 30, 2021: contributor

Addressed comments by @hebasto and @luke-jr.

kristapsk force-pushed on Jul 1, 2021

kristapsk commented at 8:26 am on July 1, 2021: contributor

Modified changeEvent to match #366.

in src/qt/openuridialog.cpp:16 in 5062565e11 outdated

12+#include <QAbstractButton>
13+#include <QLineEdit>
14 #include <QUrl>
15 
16-OpenURIDialog::OpenURIDialog(QWidget *parent) :
17+OpenURIDialog::OpenURIDialog(const PlatformStyle* platformStyle, QWidget* parent) :

jarolrod commented at 0:54 am on August 5, 2021:

should apply clang format

 0diff --git a/src/qt/openuridialog.cpp b/src/qt/openuridialog.cpp
 1index 3cf0d0a9c..a68eee718 100644
 2--- a/src/qt/openuridialog.cpp
 3+++ b/src/qt/openuridialog.cpp
 4@@ -13,10 +13,9 @@
 5 #include <QLineEdit>
 6 #include <QUrl>
 7 
 8-OpenURIDialog::OpenURIDialog(const PlatformStyle* platformStyle, QWidget* parent) :
 9-    QDialog(parent, GUIUtil::dialog_flags),
10-    ui(new Ui::OpenURIDialog),
11-    m_platform_style(platformStyle)
12+OpenURIDialog::OpenURIDialog(const PlatformStyle* platformStyle, QWidget* parent) : QDialog(parent, GUIUtil::dialog_flags),
13+                                                                                    ui(new Ui::OpenURIDialog),
14+                                                                                    m_platform_style(platformStyle)
15 {
16     ui->setupUi(this);
17     ui->pasteButton->setIcon(m_platform_style->SingleColorIcon(":/icons/editpaste"));
18@@ -38,8 +37,7 @@ QString OpenURIDialog::getURI()
19 void OpenURIDialog::accept()
20 {
21     SendCoinsRecipient rcp;
22-    if (GUIUtil::parseBitcoinURI(getURI(), &rcp))
23-    {
24+    if (GUIUtil::parseBitcoinURI(getURI(), &rcp)) {
25         /* Only accept value URIs */
26         QDialog::accept();
27     } else {

kristapsk commented at 12:02 pm on August 12, 2021:

Any docs how to propertly use that clang format thing? Can’t figure that out.

hebasto commented at 3:18 pm on September 3, 2021:

@kristapsk

https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools#clang-format-diffpy

Just run alias clang-format-diff='git diff -U0 HEAD~1.. | ./contrib/devtools/clang-format-diff.py -p1 -i -v' after each commit, then git commit --all --amend to apply suggested changes.

in src/qt/forms/openuridialog.ui:36 in 5062565e11 outdated

29@@ -30,6 +30,27 @@
30        </property>
31       </widget>
32      </item>
33+     <item>
34+      <widget class="QToolButton" name="pasteButton">
35+       <property name="toolTip">
36+        <string>Paste address from clipboard</string>

jarolrod commented at 1:09 am on August 5, 2021:

we can add translator comments by filling in the extracomment field:

0        <string extracomment="Tooltip text for button that allows you to paste an address that is in your clipboard.">Paste address from clipboard</string>

jarolrod commented at 1:12 am on August 5, 2021: member

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.

shaavan commented at 4:41 pm on August 11, 2021: contributor

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

gui: Paste button in Open URI dialog

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>

dbde0558ce

kristapsk force-pushed on Sep 26, 2021

kristapsk commented at 0:33 am on September 26, 2021: contributor

Addressed additional review comments, this ready for another review.

shaavan approved

shaavan commented at 1:05 pm on September 27, 2021: contributor

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 commented at 5:07 am on November 2, 2021: member

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 commented at 9:38 pm on November 9, 2021: contributor

Tested ACK dbde0558ce73db4c901dbaa2eddc634701fa1d0d.

hebasto merged this on Nov 21, 2021

hebasto closed this on Nov 21, 2021

sidhujag referenced this in commit 0528061023 on Nov 22, 2021

kristapsk deleted the branch on Nov 22, 2021

sidhujag referenced this in commit 41cc657f59 on Nov 23, 2021

bitcoin-core locked this on Nov 22, 2022

Paste button in Open URI dialog #319