Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix visual quality of text in QR image #71

Merged
merged 2 commits into from
Oct 23, 2020
Merged

Fix visual quality of text in QR image #71

merged 2 commits into from
Oct 23, 2020

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Aug 23, 2020 •
edited
Loading

Master (197450f):
DeepinScreenshot_select-area_20200824001800

This PR (6954156):

  • macOS 10.15.6
    Screenshot from 2020-09-07 15-40-30

  • Linux Mint 20
    Screenshot from 2020-09-07 15-48-13

Fix #54
Fix bitcoin/bitcoin#19103

The first commit is easy to review with git diff --word-diff.

@maflcko
Copy link
Contributor

maflcko commented Aug 24, 2020

Concept ACK

@Sjors
Copy link
Member

Sjors commented Aug 28, 2020

f4016dd looks way better on macOS. Can you explain what fixed it?

@hebasto
Copy link
Member Author

hebasto commented Aug 28, 2020 •
edited
Loading

@Sjors

f4016dd looks way better on macOS. Can you explain what fixed it?

@michaelfolkson
Copy link
Contributor

Looking for Concept (N)ACKs (plus comments if you have them) from designers cc @GBKS @Bosch-0 @johnsBeharry

@michaelfolkson
Copy link
Contributor

Concept ACK from me on fixing visual quality. I'll leave the font discussion to the designers.

@hebasto hebasto mentioned this pull request Sep 3, 2020
Merged
@Bosch-0
Copy link

Bosch-0 commented Sep 3, 2020

Concept ACK

If #79 gets merged and we use an embedded mono font this should be used here.

@hebasto
Copy link
Member Author

hebasto commented Sep 3, 2020

@Bosch-0

If #79 gets merged and we use an embedded mono font this should be used here.

In image or in "Address" label?

@Bosch-0
Copy link

Bosch-0 commented Sep 3, 2020

Both

@luke-jr
Copy link
Member

luke-jr commented Sep 3, 2020

Would prefer to keep the font monospaced

@GBKS
Copy link

GBKS commented Sep 7, 2020

Concept ACK on fixing the visual quality.

A monospace font would be nice, and also a narrow space every 4 characters (like in credit card numbers, some exploration here) for easier comparison, but all of that seems like a different issue.

@hebasto
Copy link
Member Author

hebasto commented Sep 7, 2020

Updated f4016dd -> 6954156 (pr71.01 -> pr71.02, diff):

@luke-jr
Copy link
Member

luke-jr commented Sep 18, 2020

Before:
before
After:
after

I don't see a noteworthy improvement...

@hebasto
Copy link
Member Author

hebasto commented Sep 18, 2020

@luke-jr

I don't see a noteworthy improvement...

Mind sharing your system details?

On your screenshots the address string does not even fit in the allocated space...
Going to reproduce such behavior.

@luke-jr
Copy link
Member

luke-jr commented Sep 18, 2020

Qt GUI 5.14.2

Preferred font https://luke.dashjr.org/education/tonal/glyphs/fonts/Console/

(Note that it is a bitmap font and only supports one size)

@hebasto
Copy link
Member Author

hebasto commented Sep 18, 2020

(Note that it is a bitmap font and only supports one size)

This explains the string size issue.

@RandyMcMillan
Copy link
Contributor

It may be best to not add the address to the qrcode image.
This would free up space to display a larger qrcode image in the imageView.
Maybe the save image dialogue should auto populate the name field with the address.
This would allow the user to check the address as they are saving the image.
Maybe the image name field should not be editable by default - and an edit check box option added
to ensure that the user only changes the name of the image intentionally.

@jonasschnelli jonasschnelli added this to the 0.21.0 milestone Oct 23, 2020
@jonasschnelli
Copy link
Contributor

Tested ACK 6954156 - tested on macOS 10.15. Fixes the problem.

The font still remains non-HiDPI.
This is the image saved through the dialogs save function:
test

The image seems to use a HiDPI resolution but the font isn't rendered in HiDPI.

@jonasschnelli jonasschnelli merged commit 49984b4 into bitcoin-core:master Oct 23, 2020
@hebasto hebasto deleted the 200823-qr branch October 23, 2020 10:08
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 23, 2020
@jonasschnelli
Merge bitcoin-core/gui#71: Fix visual quality of text in QR image
a4909c6 
6954156 qt: Fix visual quality of text in QR image (Hennadii Stepanov)
8071c75 qt, refactor: Limit scope of QPainter object (Hennadii Stepanov)

Pull request description:

  Master (197450f):
  ![DeepinScreenshot_select-area_20200824001800](https://user-images.githubusercontent.com/32963518/90988962-96283680-e59f-11ea-8e20-42e9b23033f5.png)

  This PR (6954156):
  - macOS 10.15.6
  ![Screenshot from 2020-09-07 15-40-30](https://user-images.githubusercontent.com/32963518/92390251-2c716600-f123-11ea-96f0-0e9d35810c76.png)

  - Linux Mint 20
  ![Screenshot from 2020-09-07 15-48-13](https://user-images.githubusercontent.com/32963518/92390272-36936480-f123-11ea-8fee-4de23bb40ed9.png)

  Fix #54
  Fix bitcoin#19103

  ---

  The first commit is easy to review with [`git diff --word-diff`](bitcoin-core/gui@8071c75?w=1).

ACKs for top commit:
  jonasschnelli:
    Tested ACK 6954156 - tested on macOS 10.15. Fixes the problem.

Tree-SHA512: 6ecb3397d2a5094c5f00ee05fc09520751568404e000a8691b6de7e57f38c2d5da628694e5e45a2b4cc302a846bbc00014c40820233eb026d3ebd4f68c2c9913
apoelstra added a commit to apoelstra/elements that referenced this pull request Dec 3, 2020
@apoelstra
Merge 49984b4 into merged_master (Bitcoin PR bitcoin-core/gui#71)
5043767
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Mar 23, 2021
@apoelstra
Merge 49984b4 into merged_master (Bitcoin PR bitcoin-core/gui#71)
6682a29
@bitcoin-core bitcoin-core locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.21.0
Development

Successfully merging this pull request may close these issues.

QRCode address in image is not readable QRCode address in image is not readable
9 participants
@hebasto @maflcko @Sjors @michaelfolkson @Bosch-0 @luke-jr @GBKS @RandyMcMillan @jonasschnelli