QRImageWidget: Sizing and font fixups

luke-jr commented at 6:36 am on December 13, 2021: member

Margin of QRCode should be constant, not change based on complexity of QRCode
Text region should be sized based on height of actual text, not a constant guess
Width of image should not be adjusted based on vertical text size/margins
Adjusted constant QRCode and margin sizes to visually match what we had previously
If text won’t fit, grow up to 25% wider, or split across multiple lines

(Note: Out of scope for this PR, after #497, I think we should allow customising the font used here and default to the embedded monospace for better privacy)

Bugfix: GUI/QRImageWidget: Correctly calculate sizes

- Margin of QRCode should be constant, not change based on complexity of QRCode
- Text region should be sized based on height of actual text, not a constant guess
- Width of image should not be adjusted based on vertical text size/margins
- Adjusted constant QRCode and margin sizes to visually match what we had previously

f9d9a1d1cf

Bugfix: GUI/QRImageWidget: If text won't fit, split across multiple lines

This ensures even with a font we can't scale down enough, the text doesn't get cut off

91d24c84ba

GUI/QRImageWidget: Allow image to grow up to 25% wider to accomidate single-line text

Accomidates addresses that are just barely too long for the QRCode width

4c5143d56c

promag commented at 2:27 pm on December 13, 2021: contributor

Concept ACK, however, I’ll try a different approach for the multi-line case.

promag commented at 0:10 am on December 14, 2021: contributor

@luke-jr feel free to try the commit 0ac4f7535a90bacb0e5ffe8f110505aa44075ad6 (branch) which avoids manual text wrap by using Qt::TextWrapAnywhere flag.

Another thing I noticed is that the default minimum font size in calculateIdealFontSize is too small.

luke-jr commented at 2:11 am on December 14, 2021: member

0        auto rect = fm.boundingRect(max_rect, Qt::AlignCenter, text);
1        if (rect.width() > max_rect.width() * 5 / 4) {

Isn’t this impossible? I find the unnecessary QRect usage hard to follow.

Another thing I noticed is that the default minimum font size in calculateIdealFontSize is too small.

IMO out of scope for this PR

jarolrod added the label UI on Dec 14, 2021

promag commented at 9:02 am on December 14, 2021: contributor

Isn’t this impossible?

No because it doesn’t wrap.

I find the unnecessary QRect usage hard to follow.

And I think it’s unnecessary to calculate lines and manually wrap the text. What is hard to follow?

hebasto commented at 10:36 pm on June 1, 2022: member

@luke-jr Screenshots “before” and “after” in the PR description could be useful for further discussion.

DrahtBot commented at 9:47 pm on June 12, 2022: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	pablomartin4btc
Concept ACK	promag, jarolrod

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

jarolrod commented at 6:14 am on June 28, 2022: member

Concept ACK, stylistic changes look fine to me.

master	PR (rebased)

hebasto commented at 2:25 pm on July 19, 2022: member

Tested 4c5143d56c98d4f9f961059692f155bbe2248c44.

This PR effectively renders font size smaller that makes it harder to read.

master branch:
this PR:

DrahtBot commented at 1:21 am on May 6, 2023: contributor

There hasn’t been much activity lately. What is the status here?

Finding reviewers may take time. However, if the patch is no longer relevant, please close this pull request. If the author lost interest or time to work on this, please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

hebasto commented at 2:53 pm on May 16, 2023: member

This #506#pullrequestreview-1043535557:

This PR effectively renders font size smaller that makes it harder to read.

remains unaddressed.

DrahtBot added the label CI failed on Aug 18, 2023

DrahtBot removed the label CI failed on Aug 23, 2023

DrahtBot added the label CI failed on Aug 31, 2023

DrahtBot removed the label CI failed on Sep 4, 2023

pablomartin4btc commented at 7:54 pm on September 17, 2023: contributor

Concept ACK, I agree with the improvements on achieving a more consistent adjustment of the QR code image size. However, it appears that there is still an issue with the font size, which makes the address in the picture difficult to read. Is there a way to improve the legibility of the address text?

pablomartin4btc commented at 7:52 pm on September 18, 2023: contributor

tested ACK 4c5143d56c98d4f9f961059692f155bbe2248c44

master:

Screenshot from 2023-09-17 18-52-45

this PR (rebased):

Screenshot from 2023-09-18 16-41-03

DrahtBot added the label CI failed on Sep 22, 2023

DrahtBot removed the label CI failed on Sep 24, 2023

DrahtBot added the label CI failed on Oct 18, 2023

DrahtBot commented at 2:34 pm on February 5, 2024: contributor

🤔 There hasn’t been much activity lately and the CI seems to be failing.

If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn’t been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.

hebasto commented at 11:26 am on February 12, 2024: member

Closing due to lack of support from the PR author (https://github.com/bitcoin-core/gui/pull/506#issuecomment-1549832808, #506#pullrequestreview-1630119899).

hebasto closed this on Feb 12, 2024

QRImageWidget: Sizing and font fixups #506

Reviews

Conflicts