gui: Paste button in Open URI dialog #17955

pull emilengler wants to merge 1 commits into bitcoin:master from emilengler:2020-01-paste-bitcoin-uri-button changing 4 files +25 −4

emilengler commented at 12:13 pm on January 18, 2020: contributor

This adds a button to paste a URI from the clipboard. Other forms also have this and it would be useful to have this here as well.
fanquake added the label GUI on Jan 18, 2020
in src/qt/forms/openuridialog.ui:46 in 01602e453a 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>
hebasto commented at 12:17 pm on January 18, 2020:

Why this shortcut instead of traditional Ctrl+V?

emilengler commented at 12:17 pm on January 18, 2020:

To make it coherent with the other paste buttons

hebasto commented at 9:13 am on January 21, 2020:

Is it worth to present the shortcut to a user somehow? In placeholderText?

promag commented at 10:01 am on January 21, 2020:

Honestly I don’t see value in the shortcut, I’d remove it from the others and make sure the line edit has focus.
emilengler force-pushed on Jan 18, 2020
emilengler commented at 1:07 pm on January 18, 2020: contributor

Linter should be happy now
kristapsk approved
kristapsk commented at 1:38 pm on January 18, 2020: contributor

ACK cd5afeeaa9c90cc024eedf470a6055491e2b4b23
in src/qt/openuridialog.cpp:20 in cd5afeeaa9 outdated
16 QDialog(parent), 17 ui(new Ui::OpenURIDialog) 18 { 19 ui->setupUi(this); 20+ ui->pasteButton->setIcon(platformStyle->SingleColorIcon(":/icons/editpaste")); 21+ QObject::connect(ui->pasteButton, &QPushButton::clicked, [=] { ui->uriEdit->setText(QApplication::clipboard()->text()); });
hebasto commented at 3:13 pm on January 18, 2020:

The on_pasteButton_clicked() slot with automatic connection seems simpler and more explicit.

emilengler commented at 7:52 am on January 19, 2020:

What’s wrong with this? The lambda functions brings it on the point
hebasto commented at 8:41 am on January 19, 2020:

What’s wrong with this?

I did not say “wrong” ;) Yes, it works.

The lambda functions brings it on the point

Capturing [=] seems an unnecessary overhead. Traditional on_pasteButton_clicked() slot seems more expected for a such simple functionality.

You only need to define this slot. Qt makes a proper connection to &QPushButton::clicked signal automagically.

Refs:

https://stackoverflow.com/questions/48932821/what-is-the-functional-difference-between-these-two-qt-signal-slot-connections

https://doc.qt.io/qt-5/designer-using-a-ui-file.html#widgets-and-dialogs-with-auto-connect
promag commented at 10:02 am on January 21, 2020:

I kind of dislike automatic connections since it’s easy to break them - for instance renaming the item.

hebasto commented at 10:16 am on January 21, 2020:

I kind of dislike automatic connections since it’s easy to break them - for instance renaming the item.

Make sense. Maybe just a standard connection to a named slot? (01602e453a7a731fa4a6f409cae2ffa2e31709c6 was good except old syntax).
promag commented at 2:49 pm on January 21, 2020:

Agree with @hebasto

0connect(ui->pasteButton, &QPushButton::clicked, ui->uriEdit, &QLineEdit::paste);
jonasschnelli commented at 9:13 am on May 29, 2020:

@emilengler I guess @promag’s approach seems simpler… interested to implement it that way?
hebasto commented at 3:14 pm on January 18, 2020: member

Concept NACK as Ctrl+V for “paste” works fine in this dialog already.
emilengler commented at 7:50 am on January 19, 2020: contributor

Concept NACK as Ctrl+V for “paste” works fine in this dialog already.

I know this but why do we have already many paste buttons in bitcoin-qt then.
hebasto commented at 8:47 am on January 19, 2020: member

Concept NACK as Ctrl+V for “paste” works fine in this dialog already.

I know this but why do we have already many paste buttons in bitcoin-qt then.

This dialog has the only input field and it is already focused when dialog is opened. That is not the case for other dialogs you mentioned.
jonasschnelli commented at 5:38 am on January 20, 2020: contributor

Yeah. Why not. We also have the paste button in the address field in the send coins dialog. Since people paste here (the URI field) some sort of a payment-request, it makes the paste-button-approach more consistent.

Concept ACK.
laanwj commented at 7:28 pm on January 20, 2020: member

Concept ACK
in src/qt/openuridialog.h:10 in cd5afeeaa9 outdated
6@@ -7,6 +7,8 @@ 7 8 #include <QDialog> 9 10+#include <qt/platformstyle.h>
promag commented at 10:03 am on January 21, 2020:

Just forward declare PlatformStyle.

emilengler commented at 1:43 pm on January 21, 2020:

@promag

Honestly I don’t see value in the shortcut, I’d remove it from the others and make sure the line edit has focus.

The line has focus. I copied the shortcut from the other occurrences of this button

I kind of dislike automatic connections since it’s easy to break them - for instance renaming the item.

Renaming them would cause an error

promag commented at 2:44 pm on January 21, 2020:

What error? The doc at https://doc.qt.io/qt-5/qmetaobject.html#connectSlotsByName doesn’t mention it.

emilengler commented at 3:03 pm on January 21, 2020:

Nevermind, I only changed the occurrence in the .cpp file and not the qt file
promag commented at 10:04 am on January 21, 2020: member

Concept ACK adding the button, more user friendly for mouse/touch. For users that use keyboard there’s already the native paste shortcut.

in src/qt/openuridialog.h:21 in cd5afeeaa9 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 10:18 am on January 21, 2020:

style nit

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

emilengler force-pushed on Jan 21, 2020
DrahtBot commented at 10:13 am on January 24, 2020: member

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

Conflicts

No conflicts as of last run.
luke-jr referenced this in commit 8cc7ec5a0f on Feb 9, 2020
DrahtBot added the label Needs rebase on May 4, 2020
jonasschnelli commented at 8:25 am on May 29, 2020: contributor

Needs rebase. Otherwise ready for merge.
gui: Paste button in Open URI dialog 0139b42892
emilengler force-pushed on May 29, 2020
emilengler commented at 9:08 am on May 29, 2020: contributor

Rebased
DrahtBot removed the label Needs rebase on May 29, 2020
jonasschnelli commented at 1:45 pm on May 29, 2020: contributor

@emilengler: once #17955 (review) is addressed, this should be ready for merge.
fanquake commented at 1:14 pm on July 9, 2020: member

Given there has been no follow up for the last month (in regards to the different implementation), and this is purely a qt-only change, I’m going to close this, and suggest you reopen the new version in the https://github.com/bitcoin-core/gui repo.
fanquake closed this on Jul 9, 2020
kristapsk commented at 12:00 pm on May 4, 2021: contributor

@emilengler You don’t plant to re-open this in the https://github.com/bitcoin-core/gui ?
emilengler commented at 1:03 pm on May 4, 2021: contributor

@emilengler You don’t plant to re-open this in the https://github.com/bitcoin-core/gui ?

No, I currently lack time and motivation for this but feel free to pick it up :^)
hebasto referenced this in commit 79e64a053d on Nov 21, 2021
DrahtBot locked this on Aug 18, 2022

Contributors
emilengler hebasto promag kristapsk jonasschnelli laanwj DrahtBot fanquake

Labels
GUI

gui: Paste button in Open URI dialog #17955

Conflicts