qt: Fix height of QR-less ReceiveRequestDialog #17597

hebasto · 2019-11-25T19:22:44Z

If master (89a1f7a) is compiled without QR support, the "Request payment to..." dialog looks ugly:

With this PR:

Other minor changes:

"URI" abbreviation is not translated now
wallet name is shown if available

src/qt/forms/receiverequestdialog.ui

hebasto · 2019-11-25T19:33:47Z

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)

laanwj · 2019-11-25T19:34:37Z

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

hebasto · 2019-11-25T19:42:15Z

@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 · 2019-11-25T19:50:06Z

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

laanwj · 2019-11-26T16:33:42Z

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.

cvengler · 2019-11-26T16:35:45Z

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

hebasto · 2019-11-27T13:26:15Z

@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 · 2019-12-05T22:46:15Z

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

DrahtBot · 2019-12-06T06:27:45Z

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

Conflicts

No conflicts as of last run.

hebasto · 2019-12-06T19:33:01Z

It seems Travis is happy now.

hebasto · 2019-12-07T10:11:49Z

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

Sjors · 2019-12-10T12:52:23Z

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 · 2019-12-25T22:52:40Z

@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 · 2020-01-02T10:33:03Z

Much better. ACK cc88011

luke-jr

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

luke-jr · 2020-01-03T17:43:15Z

src/qt/forms/receiverequestdialog.ui

    </widget>
   </item>
-   <item>
-    <widget class="QTextEdit" name="outUri">


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

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

... then Concept NACK I guess

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

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.

Sounds like a hack

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.

luke-jr · 2020-01-03T17:43:41Z

src/qt/forms/receiverequestdialog.ui

      <bool>true</bool>
     </property>
     <property name="textInteractionFlags">
-      <set>Qt::TextSelectableByKeyboard|Qt::TextSelectableByMouse</set>


Selection by keyboard can be nice...

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

luke-jr · 2020-01-03T17:44:16Z

src/qt/forms/receiverequestdialog.ui

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


Close seems more correct.

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.

src/qt/receiverequestdialog.cpp

luke-jr · 2020-01-03T17:49:27Z

src/qt/forms/receiverequestdialog.ui

+      </font>
+     </property>
+     <property name="text">
+      <string notr="true">URI:</string>


This SHOULD be translated...

"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

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

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.

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

hebasto · 2020-01-03T21:33:44Z

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

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

Sjors · 2020-01-04T05:27:03Z

src/qt/forms/receiverequestdialog.ui

+      </font>
+     </property>
+     <property name="text">
+      <string notr="true">URI:</string>


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

Sjors · 2020-01-04T05:37:37Z

src/qt/forms/receiverequestdialog.ui

    </widget>
   </item>
-   <item>
-    <widget class="QTextEdit" name="outUri">


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.

promag · 2020-01-06T16:07:03Z

It works, but the following also works

--- a/src/qt/forms/receiverequestdialog.ui
+++ b/src/qt/forms/receiverequestdialog.ui
@@ -6,8 +6,6 @@
    <rect>
     <x>0</x>
     <y>0</y>
-    <width>487</width>
-    <height>597</height>
    </rect>
   </property>
   <layout class="QVBoxLayout" name="verticalLayout_3">
diff --git a/src/qt/receiverequestdialog.cpp b/src/qt/receiverequestdialog.cpp
index b4fae7d78d..cfe0bc7d52 100644
--- a/src/qt/receiverequestdialog.cpp
+++ b/src/qt/receiverequestdialog.cpp
@@ -85,6 +85,7 @@ void ReceiveRequestDialog::update()
     if (ui->lblQRCode->setQR(uri, info.address)) {
         ui->btnSaveAs->setEnabled(true);
     }
+    this->adjustSize();
 }

 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 · 2020-01-06T16:31:20Z

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

The result looks ugly for me:

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

promag · 2020-01-06T18:16:54Z

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 (comment). 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 · 2020-01-06T20:16:58Z

@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 · 2020-01-06T20:18:48Z

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

promag · 2020-01-06T23:58:08Z

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.

hebasto · 2020-02-08T13:00:17Z

@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..."

This commit does not change behavior.

hebasto · 2020-05-05T03:07:26Z

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

jonasschnelli · 2020-05-29T08:01:58Z

Tested ACK 73529f0

master:

With this PR:

Disabled QRCode support:

73529f0 qt: Rename slot to updateDisplayUnit() (Hennadii Stepanov) 68288ef qt: Overhaul ReceiveRequestDialog (Hennadii Stepanov) Pull request description: If master (89a1f7a) is compiled without QR support, the "Request payment to..." dialog looks ugly: ![Screenshot from 2019-11-25 19-58-59](https://user-images.githubusercontent.com/32963518/69566647-3d9c1c80-0fc0-11ea-8ff6-183cea9372c5.png) With this PR: ![Screenshot from 2019-12-26 00-42-46](https://user-images.githubusercontent.com/32963518/71451226-221c6100-277a-11ea-94ae-c19a5c6256f7.png) ![Screenshot from 2019-12-26 00-44-34](https://user-images.githubusercontent.com/32963518/71451228-29436f00-277a-11ea-8ac5-1bafe6d73b5c.png) ![Screenshot from 2019-12-26 00-48-51](https://user-images.githubusercontent.com/32963518/71451230-2e082300-277a-11ea-8c4c-726ca7b776e7.png) Other minor changes: - "URI" abbreviation is not translated now - wallet name is shown if available ACKs for top commit: jonasschnelli: Tested ACK 73529f0 Tree-SHA512: 45f9a41d3c72978d78eb2e8ca98e274b8be5abf5352fd3e1f4f49514ce744994545fb3012e80600ded936ae41c6be4d4825a0c5f7bcb3db671d69e0299f0f65b

Summary: > If master is compiled without QR support, the "Request payment to..." dialog looks ugly > Other minor changes: > > - "URI" abbreviation is not translated now > - wallet name is shown if available This is a backport of [[bitcoin/bitcoin#17597 | core#17597]] [1/2] bitcoin/bitcoin@68288ef Test Plan: `ninja && ninja check` Build without QR code support and inspect the result. ``` cmake .. -GNinja -DENABLE_QRCODE=OFF ninja && src/qt/bitcoin-qt ``` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9922

Summary: This commit does not change behavior. This is a backport of [[bitcoin/bitcoin#17597 | core#17597]] [2/2] bitcoin/bitcoin@73529f0 Test Plan: `ninja` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9923

fanquake added the GUI label Nov 25, 2019

laanwj reviewed Nov 25, 2019

View reviewed changes

src/qt/forms/receiverequestdialog.ui Outdated Show resolved Hide resolved

hebasto closed this Nov 25, 2019

hebasto reopened this Nov 25, 2019

hebasto force-pushed the 20191125-height-without-qr branch from 78c6606 to 56c2d01 Compare December 5, 2019 22:34

hebasto mentioned this pull request Dec 5, 2019

qt: Remove connection for unused accepted() signal in ReceiveRequestDialog #17600

Closed

hebasto force-pushed the 20191125-height-without-qr branch from 56c2d01 to 04df22e Compare December 5, 2019 22:57

hebasto force-pushed the 20191125-height-without-qr branch from 04df22e to 23138de Compare December 6, 2019 18:46

hebasto force-pushed the 20191125-height-without-qr branch from 23138de to cc88011 Compare December 25, 2019 22:49

luke-jr suggested changes Jan 3, 2020

View reviewed changes

hebasto force-pushed the 20191125-height-without-qr branch from cc88011 to b8e90f3 Compare January 3, 2020 21:31

Sjors approved these changes Jan 4, 2020

View reviewed changes

kangzixiang approved these changes Jan 14, 2020

View reviewed changes

DrahtBot mentioned this pull request Feb 11, 2020

gui: Add close window shortcut #15768

Merged

DrahtBot mentioned this pull request Feb 22, 2020

UI external signer support (e.g. hardware wallet) #16549

Closed

This was referenced Mar 2, 2020

wip: gui: Drop connectSlotsByName usage #18246

Closed

wip: gui: Refactor to drop client and wallet models setters #18239

Closed

DrahtBot added the Needs rebase label May 4, 2020

hebasto added 2 commits May 5, 2020 05:56

qt: Overhaul ReceiveRequestDialog

68288ef

qt: Rename slot to updateDisplayUnit()

73529f0

This commit does not change behavior.

hebasto force-pushed the 20191125-height-without-qr branch from b8e90f3 to 73529f0 Compare May 5, 2020 02:57

DrahtBot removed the Needs rebase label May 5, 2020

jonasschnelli merged commit 3431699 into bitcoin:master May 29, 2020

hebasto deleted the 20191125-height-without-qr branch May 29, 2020 09:02

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

qt: Fix height of QR-less ReceiveRequestDialog #17597

qt: Fix height of QR-less ReceiveRequestDialog #17597

Uh oh!

Conversation

hebasto commented Nov 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

hebasto commented Nov 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Nov 25, 2019

Uh oh!

hebasto commented Nov 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Nov 25, 2019

Uh oh!

laanwj commented Nov 26, 2019

Uh oh!

cvengler commented Nov 26, 2019

Uh oh!

hebasto commented Nov 27, 2019

Uh oh!

hebasto commented Dec 5, 2019

Uh oh!

DrahtBot commented Dec 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

hebasto commented Dec 6, 2019

Uh oh!

hebasto commented Dec 7, 2019

Uh oh!

Sjors commented Dec 10, 2019

Uh oh!

hebasto commented Dec 25, 2019

Uh oh!

Sjors commented Jan 2, 2020

Uh oh!

luke-jr 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!

hebasto Jan 3, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

hebasto Jan 3, 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!

hebasto commented Jan 3, 2020

Uh oh!

hebasto commented Nov 25, 2019 •

edited

Loading

hebasto commented Nov 25, 2019 •

edited

Loading

hebasto commented Nov 25, 2019 •

edited

Loading

DrahtBot commented Dec 6, 2019 •

edited

Loading

hebasto Jan 3, 2020 •

edited

Loading

hebasto Jan 3, 2020 •

edited

Loading

hebasto commented Jan 6, 2020 •

edited

Loading

promag commented Jan 6, 2020 •

edited

Loading