QRImageWidget: Sizing and font fixups#506
QRImageWidget: Sizing and font fixups#506luke-jr wants to merge 3 commits intobitcoin-core:masterfrom
Conversation
- 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
…ines This ensures even with a font we can't scale down enough, the text doesn't get cut off
…single-line text Accomidates addresses that are just barely too long for the QRCode width
promag
left a comment
promag
left a comment
There was a problem hiding this comment.
Concept ACK, however, I'll try a different approach for the multi-line case.
auto rect = fm.boundingRect(max_rect, Qt::AlignCenter, text);
if (rect.width() > max_rect.width() * 5 / 4) {Isn't this impossible? I find the unnecessary QRect usage hard to follow.
IMO out of scope for this PR |
No because it doesn't wrap.
And I think it's unnecessary to calculate lines and manually wrap the text. What is hard to follow? |
|
@luke-jr Screenshots "before" and "after" in the PR description could be useful for further discussion. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
jarolrod
left a comment
jarolrod
left a comment
There was a problem hiding this comment.
Concept ACK, stylistic changes look fine to me.
| master | PR (rebased) |
|---|---|
 |
 |
|
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. |
|
This #506 (review):
remains unaddressed. |
pablomartin4btc
left a comment
pablomartin4btc
left a comment
There was a problem hiding this comment.
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?
|
🤔 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. |
|
Closing due to lack of support from the PR author (#506 (comment), #506 (review)). |
6 participants
(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)