gui: Paste button in Open URI dialog #17955

cvengler · 2020-01-18T12:13:16Z

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.

hebasto · 2020-01-18T12:17:08Z

src/qt/forms/openuridialog.ui

Why this shortcut instead of traditional Ctrl+V?

To make it coherent with the other paste buttons

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

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

cvengler · 2020-01-18T13:07:44Z

Linter should be happy now

kristapsk

ACK cd5afeeaa9c90cc024eedf470a6055491e2b4b23

hebasto

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

hebasto · 2020-01-18T15:13:04Z

src/qt/openuridialog.cpp

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

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

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

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

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

Agree with @hebasto

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

@emilengler I guess @promag's approach seems simpler... interested to implement it that way?

cvengler · 2020-01-19T07:50:18Z

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 · 2020-01-19T08:47:00Z

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 · 2020-01-20T05:38:39Z

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 · 2020-01-20T19:28:47Z

Concept ACK

promag

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

promag · 2020-01-21T10:01:13Z

src/qt/forms/openuridialog.ui

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

promag · 2020-01-21T10:02:30Z

src/qt/openuridialog.cpp

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

promag · 2020-01-21T10:03:01Z

src/qt/openuridialog.h

Just forward declare PlatformStyle.

@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

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

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

src/qt/openuridialog.h

DrahtBot · 2020-01-24T10:13:09Z

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

Conflicts

No conflicts as of last run.

Github-Pull: bitcoin#17955 Rebased-From: fa5887c

jonasschnelli · 2020-05-29T08:25:53Z

Needs rebase. Otherwise ready for merge.

cvengler · 2020-05-29T09:08:25Z

Rebased

jonasschnelli · 2020-05-29T13:45:59Z

@emilengler: once #17955 (comment) is addressed, this should be ready for merge.

fanquake · 2020-07-09T13:14:52Z

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.

kristapsk · 2021-05-04T12:00:07Z

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

cvengler · 2021-05-04T13:03:01Z

@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 :^)

dbde055 gui: Paste button in Open URI dialog (Kristaps Kaupe) Pull request description: Picking up bitcoin/bitcoin#17955, with some review comments addressed. ACKs for top commit: shaavan: tACK dbde055 jarolrod: ACK dbde055 promag: Tested ACK dbde055. Tree-SHA512: db47f19673aff6becd6d1f938cd2aa5dc2291d6e80150d2b99f435674330a5eae678b20e42ef327ea9b05c44925a941fc251e622c73b3585018fc7c1d245edb5

fanquake added the GUI label Jan 18, 2020

hebasto reviewed Jan 18, 2020

View reviewed changes

cvengler force-pushed the 2020-01-paste-bitcoin-uri-button branch from 01602e4 to cd5afee Compare January 18, 2020 13:07

kristapsk approved these changes Jan 18, 2020

View reviewed changes

hebasto reviewed Jan 18, 2020

View reviewed changes

promag reviewed Jan 21, 2020

View reviewed changes

hebasto reviewed Jan 21, 2020

View reviewed changes

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

cvengler force-pushed the 2020-01-paste-bitcoin-uri-button branch from cd5afee to fa5887c Compare January 21, 2020 13:44

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Feb 9, 2020

gui: Paste button in Open URI dialog

8cc7ec5

Github-Pull: bitcoin#17955 Rebased-From: fa5887c

DrahtBot mentioned this pull request Feb 11, 2020

gui: Add close window shortcut #15768

Merged

DrahtBot added the Needs rebase label May 4, 2020

gui: Paste button in Open URI dialog

0139b42

cvengler force-pushed the 2020-01-paste-bitcoin-uri-button branch from fa5887c to 0139b42 Compare May 29, 2020 09:07

DrahtBot removed the Needs rebase label May 29, 2020

fanquake closed this Jul 9, 2020

kristapsk mentioned this pull request May 5, 2021

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

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

gui: Paste button in Open URI dialog #17955

gui: Paste button in Open URI dialog #17955

Uh oh!

Conversation

cvengler commented Jan 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cvengler commented Jan 18, 2020

Uh oh!

kristapsk left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto Jan 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cvengler commented Jan 19, 2020

Uh oh!

hebasto commented Jan 19, 2020

Uh oh!

jonasschnelli commented Jan 20, 2020

Uh oh!

laanwj commented Jan 20, 2020

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Jan 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jonasschnelli commented May 29, 2020

Uh oh!

cvengler commented May 29, 2020

Uh oh!

jonasschnelli commented May 29, 2020

Uh oh!

fanquake commented Jul 9, 2020

Uh oh!

kristapsk commented May 4, 2021

Uh oh!

cvengler commented May 4, 2021

Uh oh!

Reviewers

Assignees

hebasto Jan 19, 2020 •

edited

Loading

DrahtBot commented Jan 24, 2020 •

edited

Loading