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.

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

gui: Paste button in Open URI dialog #17955

Open
wants to merge 1 commit into
base: master
from

Conversation

@emilengler
Copy link
Contributor

emilengler commented Jan 18, 2020

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

@fanquake fanquake added the GUI label Jan 18, 2020
<normaloff>:/icons/editpaste</normaloff>:/icons/editpaste</iconset>
</property>
<property name="shortcut">
<string>Alt+P</string>

This comment has been minimized.

Copy link
@hebasto

hebasto Jan 18, 2020

Member

Why this shortcut instead of traditional Ctrl+V?

This comment has been minimized.

Copy link
@emilengler

emilengler Jan 18, 2020

Author Contributor

To make it coherent with the other paste buttons

This comment has been minimized.

Copy link
@hebasto

hebasto Jan 21, 2020

Member

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

This comment has been minimized.

Copy link
@promag

promag Jan 21, 2020

Member

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 emilengler force-pushed the emilengler:2020-01-paste-bitcoin-uri-button branch from 01602e4 to cd5afee Jan 18, 2020
@emilengler

This comment has been minimized.

Copy link
Contributor Author

emilengler commented Jan 18, 2020

Linter should be happy now

Copy link
Contributor

kristapsk left a comment

ACK cd5afee

Copy link
Member

hebasto left a comment

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

QDialog(parent),
ui(new Ui::OpenURIDialog)
{
ui->setupUi(this);
ui->pasteButton->setIcon(platformStyle->SingleColorIcon(":/icons/editpaste"));
QObject::connect(ui->pasteButton, &QPushButton::clicked, [=] { ui->uriEdit->setText(QApplication::clipboard()->text()); });

This comment has been minimized.

Copy link
@hebasto

hebasto Jan 18, 2020

Member

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

This comment has been minimized.

Copy link
@emilengler

emilengler Jan 19, 2020

Author Contributor

What's wrong with this? The lambda functions brings it on the point

This comment has been minimized.

Copy link
@hebasto

hebasto Jan 19, 2020

Member

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:

This comment has been minimized.

Copy link
@promag

promag Jan 21, 2020

Member

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

This comment has been minimized.

Copy link
@hebasto

hebasto Jan 21, 2020

Member

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? (01602e4 was good except old syntax).

This comment has been minimized.

Copy link
@promag

promag Jan 21, 2020

Member

Agree with @hebasto

connect(ui->pasteButton, &QPushButton::clicked, ui->uriEdit, &QLineEdit::paste);
@emilengler

This comment has been minimized.

Copy link
Contributor Author

emilengler commented Jan 19, 2020

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

This comment has been minimized.

Copy link
Member

hebasto commented Jan 19, 2020

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

This comment has been minimized.

Copy link
Member

jonasschnelli commented Jan 20, 2020

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

This comment has been minimized.

Copy link
Member

laanwj commented Jan 20, 2020

Concept ACK

Copy link
Member

promag left a comment

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

<normaloff>:/icons/editpaste</normaloff>:/icons/editpaste</iconset>
</property>
<property name="shortcut">
<string>Alt+P</string>

This comment has been minimized.

Copy link
@promag

promag Jan 21, 2020

Member

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

QDialog(parent),
ui(new Ui::OpenURIDialog)
{
ui->setupUi(this);
ui->pasteButton->setIcon(platformStyle->SingleColorIcon(":/icons/editpaste"));
QObject::connect(ui->pasteButton, &QPushButton::clicked, [=] { ui->uriEdit->setText(QApplication::clipboard()->text()); });

This comment has been minimized.

Copy link
@promag

promag Jan 21, 2020

Member

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

@@ -7,6 +7,8 @@

#include <QDialog>

#include <qt/platformstyle.h>

This comment has been minimized.

Copy link
@promag

promag Jan 21, 2020

Member

Just forward declare PlatformStyle.

This comment has been minimized.

Copy link
@emilengler

emilengler Jan 21, 2020

Author Contributor

@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

This comment has been minimized.

Copy link
@promag

promag Jan 21, 2020

Member

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

This comment has been minimized.

Copy link
@emilengler

emilengler Jan 21, 2020

Author Contributor

Nevermind, I only changed the occurrence in the .cpp file and not the qt file

src/qt/openuridialog.h Outdated Show resolved Hide resolved
@emilengler emilengler force-pushed the emilengler:2020-01-paste-bitcoin-uri-button branch from cd5afee to fa5887c Jan 21, 2020
@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Jan 24, 2020

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15768 (gui: Add close window shortcut by IPGlider)

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.

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Feb 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.