qt: Fix height of QR-less ReceiveRequestDialog

hebasto commented at 7:22 pm on November 25, 2019: member

If master (89a1f7a250ef70ff2d65701564f1e24bb9280d90) is compiled without QR support, the “Request payment to…” dialog looks ugly:

Screenshot from 2019-11-25 19-58-59

With this PR:

Screenshot from 2019-12-26 00-42-46

Screenshot from 2019-12-26 00-44-34

Screenshot from 2019-12-26 00-48-51

Other minor changes:

“URI” abbreviation is not translated now
wallet name is shown if available

fanquake added the label GUI on Nov 25, 2019

in src/qt/forms/receiverequestdialog.ui:53 in 78c6606c3e outdated

49@@ -50,7 +50,7 @@
50      <property name="minimumSize">
51       <size>
52        <width>0</width>
53-       <height>50</height>
54+       <height>176</height>

laanwj commented at 7:32 pm on November 25, 2019:

We should really try to avoid relying on hardcoded heights. The necessary heght varies based on things such as font size, DPI, styling.

jonasschnelli commented at 2:18 am on November 26, 2019:

Indeed. Can we make the whole dialog floating (dynamic height)?

hebasto commented at 7:33 pm on November 25, 2019: member

We should really try to avoid relying on hardcoded heights. The necessary heght varies based on things such as font size, DPI, styling

Right, but #17597 (comment)

hebasto closed this on Nov 25, 2019

laanwj commented at 7:34 pm on November 25, 2019: member

Wasn’t there some other way to pack the window?

hebasto commented at 7:42 pm on November 25, 2019: member

@laanwj

Wasn’t there some other way to pack the window?

Qt allows many ways to do it. I’ve chosen one with the minimal diff, though.

hebasto commented at 7:50 pm on November 25, 2019: member

@laanwj

We should really try to avoid relying on hardcoded heights.

We use hardcoded sizes as defaults/minimums/maximums for all windows now.

The necessary heght varies based on things such as font size, DPI, styling.

Currently, we rely on window resizing.

This PR changes default/minimum heights and the dialog always could be resized automagically, if QR image is supplied, or manually.

hebasto reopened this on Nov 25, 2019

laanwj commented at 4:33 pm on November 26, 2019: member

The problem is that these values can be tweaked until eternity, no two people with different OS / configuration are going to arrive at exactly the same value.

emilengler commented at 4:35 pm on November 26, 2019: contributor

Haven’t tested it yet but I think just a this->adjustSize() would do the job better

hebasto commented at 1:26 pm on November 27, 2019: member

@emilengler

Haven’t tested it yet but I think just a this->adjustSize() would do the job better

IMO, QWidget::adjustSize() should not be used to fix a broken layout.

It first was introduced in our repo in #16971, but I agree with @promag’s #16971 (comment) and @Sjors’s #16971 (comment).

hebasto force-pushed on Dec 5, 2019

hebasto commented at 10:46 pm on December 5, 2019: member

@laanwj @jonasschnelli @emilengler Thank you for your reviews.

Wasn’t there some other way to pack the window?

Yes, alternative QGridLayout layout is used in the latest push.

We should really try to avoid relying on hardcoded heights. The necessary heght varies based on things such as font size, DPI, styling.

All number in forms/receiverequestdialog.ui are just Qt Designer artefacts.

hebasto force-pushed on Dec 5, 2019

DrahtBot commented at 6:27 am on December 6, 2019: member

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

Conflicts

No conflicts as of last run.

hebasto force-pushed on Dec 6, 2019

hebasto commented at 7:33 pm on December 6, 2019: member

It seems Travis is happy now.

hebasto commented at 10:11 am on December 7, 2019: member

@practicalswift w.r.t. #17658 (comment) would mind reviewing this PR?

Sjors commented at 12:52 pm on December 10, 2019: member

I would keep the buttons at the bottom of the dialog, consistent with other dialogs. It doesn’t look very nice on macOS, especially with long translations:

Otherwise code in 23138de looks good and it behaves. I also tried --without-qrencode

hebasto force-pushed on Dec 25, 2019

hebasto commented at 10:52 pm on December 25, 2019: member

@Sjors Thank you for your review.

I would keep the buttons at the bottom of the dialog, consistent with other dialogs.

Fixed; also it made diffs smaller ;)

Screenshots in OP have been updated.

Sjors commented at 10:33 am on January 2, 2020: member

Much better. ACK cc88011

in src/qt/forms/receiverequestdialog.ui:43 in cc8801145b outdated

220+      <set>Qt::TextSelectableByMouse</set>
221+     </property>
222     </widget>
223    </item>
224-   <item>
225-    <widget class="QTextEdit" name="outUri">

luke-jr commented at 5:43 pm on January 3, 2020:

The QTextEdit widget made it more obvious to users they could select…

hebasto commented at 8:12 pm on January 3, 2020:

Since QTextEdit inherits QAbstractScrollArea with very loose size policy, it is the point of this PR to get rid of it.

luke-jr commented at 9:23 pm on January 3, 2020:

… then Concept NACK I guess

Without QTextEdit, you won’t be able to select and copy everything together.

hebasto commented at 9:30 pm on January 3, 2020:

Without QTextEdit, you won’t be able to select and copy everything together.

Maybe a new button “Copy ALL” could help? Also all info, except a wallet name, is encoded in the URI.

luke-jr commented at 10:28 pm on January 3, 2020:

Sounds like a hack

Sjors commented at 5:37 am on January 4, 2020:

There’s a copy-URI button which copies the URI. You can tripple click on the URI itself to select it.

The QTextEdit used to cover all fields. Tripple click in that edit field wouldn’t select the URI, but everything, which doesn’t seem useful.

in src/qt/forms/receiverequestdialog.ui:66 in cc8801145b outdated

268+     </property>
269+     <property name="wordWrap">
270       <bool>true</bool>
271      </property>
272      <property name="textInteractionFlags">
273-      <set>Qt::TextSelectableByKeyboard|Qt::TextSelectableByMouse</set>

luke-jr commented at 5:43 pm on January 3, 2020:

Selection by keyboard can be nice…

hebasto commented at 9:13 pm on January 3, 2020:

This is not trivial, unfortunately (see #14810).

in src/qt/forms/receiverequestdialog.ui:118 in cc8801145b outdated

279@@ -115,7 +280,7 @@
280      <item>
281       <widget class="QDialogButtonBox" name="buttonBox">
282        <property name="standardButtons">
283-        <set>QDialogButtonBox::Close</set>

luke-jr commented at 5:44 pm on January 3, 2020:

Close seems more correct.

hebasto commented at 8:26 pm on January 3, 2020:

From QDialogButtonBox docs:

An “OK” button defined with the AcceptRole.
A “Close” button defined with the RejectRole.

It seems the AcceptRole is more appropriate for interaction with this dialog. Also see @promag’s comment.

in src/qt/receiverequestdialog.cpp:63 in cc8801145b outdated

83-        html += "<b>"+tr("Label")+"</b>: " + GUIUtil::HtmlEscape(info.label) + "<br>";
84-    if(!info.message.isEmpty())
85-        html += "<b>"+tr("Message")+"</b>: " + GUIUtil::HtmlEscape(info.message) + "<br>";
86-    if(model->isMultiwallet()) {
87-        html += "<b>"+tr("Wallet")+"</b>: " + GUIUtil::HtmlEscape(model->getWalletName()) + "<br>";
88+    ui->uri_content->setText("<a href=\"" + uri + "\">" + GUIUtil::HtmlEscape(uri) + "</a>");

luke-jr commented at 5:48 pm on January 3, 2020:

In a label, this becomes clickable.

I’m not sure why we linkify it (so it looks like a link?), but previously it wasn’t a clickable link

hebasto commented at 8:17 pm on January 3, 2020:

I’m not sure why we linkify it (so it looks like a link?)…

It seems so.

… but previously it wasn’t a clickable link

Agree, this change should be reverted.

hebasto commented at 9:31 pm on January 3, 2020:

Agree, this change should be reverted.

Done in the latest push.

in src/qt/forms/receiverequestdialog.ui:55 in cc8801145b outdated

69+       <weight>75</weight>
70+       <bold>true</bold>
71+      </font>
72+     </property>
73+     <property name="text">
74+      <string notr="true">URI:</string>

luke-jr commented at 5:49 pm on January 3, 2020:

This SHOULD be translated…

hebasto commented at 6:05 pm on January 3, 2020:

“URI” is a ubiquitous international abbreviation which does not require any translation, IMO. Refs:

https://www.wikipedia.org/ (in different languages)
src/qt/locale/*.ts

luke-jr commented at 6:17 pm on January 3, 2020:

What about languages which don’t use Latin characters?

eg https://ur.wikipedia.org/wiki/%DB%8C%D9%88_%D8%A2%D8%B1_%D8%A2%D8%A6%DB%8C

hebasto commented at 8:05 pm on January 3, 2020:

What about languages which don’t use Latin characters?

qt/locale/bitcoin_ur.ts does not translate “URI”.

My native language does not use Latin characters, and “URI”, “HTTP” etc are in use.

Sjors commented at 5:27 am on January 4, 2020:

Probably best to let translators decide that. I doubt 99.9% of native English speakers even know what URI means.

luke-jr changes_requested

luke-jr commented at 5:49 pm on January 3, 2020: member

Can you split the bugfix (height) from the controversial/problematic changes?

hebasto force-pushed on Jan 3, 2020

hebasto commented at 9:33 pm on January 3, 2020: member

Implemented @luke-jr’s comment:

In a label, this becomes clickable. I’m not sure why we linkify it (so it looks like a link?), but previously it wasn’t a clickable link

Fixed tab order.

Sjors approved

Sjors commented at 5:38 am on January 4, 2020: member

re-ACK b8e90f3bb36706fb08aadcc2731ee9596c3d7aed; only changes in the interface file, which I don’t have strong feelings about.

promag commented at 4:07 pm on January 6, 2020: member

It works, but the following also works

 0--- a/src/qt/forms/receiverequestdialog.ui
 1+++ b/src/qt/forms/receiverequestdialog.ui
 2@@ -6,8 +6,6 @@
 3    <rect>
 4     <x>0</x>
 5     <y>0</y>
 6-    <width>487</width>
 7-    <height>597</height>
 8    </rect>
 9   </property>
10   <layout class="QVBoxLayout" name="verticalLayout_3">
11diff --git a/src/qt/receiverequestdialog.cpp b/src/qt/receiverequestdialog.cpp
12index b4fae7d78d..cfe0bc7d52 100644
13--- a/src/qt/receiverequestdialog.cpp
14+++ b/src/qt/receiverequestdialog.cpp
15@@ -85,6 +85,7 @@ void ReceiveRequestDialog::update()
16     if (ui->lblQRCode->setQR(uri, info.address)) {
17         ui->btnSaveAs->setEnabled(true);
18     }
19+    this->adjustSize();
20 }
21
22 void ReceiveRequestDialog::on_btnCopyURI_clicked()

which would make @luke-jr happy.

I think this is fine because the text area is scrollable - I mean that it’s not really the same issue as the intro dialog.

hebasto commented at 4:31 pm on January 6, 2020: member

@promag Your suggestion was the first I’ve tried while working on this PR.

The result looks ugly for me: DeepinScreenshot_bitcoin-qt_20200106182801 Screenshot from 2020-01-06 18-35-44

Repo maintainers, feel free to close this PR as a controversial.

promag commented at 6:16 pm on January 6, 2020: member

Please don’t close, I don’t think this is controversial - I prefer structured GUI instead of a big text area.

I was just replying in regards to #17597 (review). However I wonder if there’s really the need to copy the whole text - if so I think a “copy details” button works just fine.

Only thing I can point out is if read only line edits are preferable because it’s obvious the text can be copied.

hebasto commented at 8:16 pm on January 6, 2020: member

@promag

Only thing I can point out is if read only line edits are preferable because it’s obvious the text can be copied.

QLineEdit cannot wrap lines, by design. Button names, “Copy …”, are self descriptive, no?

luke-jr commented at 8:18 pm on January 6, 2020: member

QFontMetrics should make it practical to set a minimum width for the textedit widget?

promag commented at 11:58 pm on January 6, 2020: member

QLineEdit cannot wrap lines, by design.

QLineEdit, QTextEdit.. I mean one that has it’s clear the user can select/tab focus and copy. But feel free to ignore the suggestion.

kangzixiang approved

hebasto commented at 1:00 pm on February 8, 2020: member

@luke-jr

QFontMetrics should make it practical to set a minimum width for the textedit widget?

It seems not convenient/accurate when different fonts/styles are mixed in the text: “Address: bc1…”

DrahtBot added the label Needs rebase on May 4, 2020

qt: Overhaul ReceiveRequestDialog 68288ef0c1

qt: Rename slot to updateDisplayUnit()

This commit does not change behavior.

73529f0859

hebasto force-pushed on May 5, 2020

hebasto commented at 3:07 am on May 5, 2020: member

Rebased b8e90f3bb36706fb08aadcc2731ee9596c3d7aed -> 73529f0859c060087dd41e9e176fd4198d0f385f (pr17597.06 -> pr17597.07) due to the conflict with #15768.

DrahtBot removed the label Needs rebase on May 5, 2020

jonasschnelli commented at 8:01 am on May 29, 2020: contributor

Tested ACK 73529f0859c060087dd41e9e176fd4198d0f385f

master:

With this PR:

Disabled QRCode support:

jonasschnelli merged this on May 29, 2020

jonasschnelli closed this on May 29, 2020

hebasto deleted the branch on May 29, 2020

sidhujag referenced this in commit a98ddb5ab6 on May 31, 2020

Fabcien referenced this in commit 64828befe6 on Aug 24, 2021

Fabcien referenced this in commit 22c0949762 on Aug 24, 2021

DrahtBot locked this on Feb 15, 2022

qt: Fix height of QR-less ReceiveRequestDialog #17597

Conflicts