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.

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

pam/qrcodemodel: Use normal-size Qr code when in simple terminals #393

Merged
merged 2 commits into from
Jun 25, 2024

Conversation

3v1n0
Copy link
Collaborator

@3v1n0 3v1n0 commented Jun 25, 2024

If the terminal is not advanced enough to support the more complex utf-8
characters needed to print the small qrcode, just use the simpler
implementation via the big qrcode that uses characters that seems to be
supported everywhere.

In fact bigger qrcode use two times the character to do a square, and
this is supported by the normal tty fonts:

getunimap | grep █ 0x0db	U+2588	# █
setfont -v -ou /dev/stdout | grep 0x0db

Without having to go through building one via #'s.

In theory we can inspect for font support on that, but it's not something we want to
implement for now.

UDENG-3109

Closes: #361

@3v1n0 3v1n0 requested a review from a team as a code owner June 25, 2024 05:26
@3v1n0 3v1n0 force-pushed the qrcode-for-tty branch 2 times, most recently from d4072a6 to 25486ce Compare June 25, 2024 05:40
@codecov-commenter
Copy link

codecov-commenter commented Jun 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.24%. Comparing base (e3a273f) to head (4017b7c).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #393      +/-   ##
==========================================
- Coverage   85.26%   85.24%   -0.02%     
==========================================
  Files          76       76              
  Lines        6140     6161      +21     
  Branches       74       75       +1     
==========================================
+ Hits         5235     5252      +17     
- Misses        642      644       +2     
- Partials      263      265       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@3v1n0 3v1n0 force-pushed the qrcode-for-tty branch 2 times, most recently from 57da7f2 to 4c3532b Compare June 25, 2024 13:23
Copy link
Member

@denisonbarbosa denisonbarbosa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thanks for fixing this!

Copy link
Member

@didrocks didrocks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! I do have one question before approving!

pam/internal/adapter/qrcodemodel.go Show resolved Hide resolved
Copy link
Member

@didrocks didrocks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comment! Nice work

They are used by bubbletea to improve the rendering so expose them all
If the terminal is not advanced enough to support the more complex utf-8
characters needed to print the small qrcode, just use the simpler
implementation via the big qrcode that uses characters that seems to be
supported everywhere.
@3v1n0 3v1n0 merged commit fa868ac into ubuntu:main Jun 25, 2024
5 checks passed
3v1n0 added a commit to 3v1n0/authd that referenced this pull request Jun 27, 2024
…ypes

This was intended to be part of ubuntu#314 but it got lost when merging with ubuntu#393
didrocks added a commit that referenced this pull request Jun 28, 2024
… run as different users (#401)

Some tests changes that were supposed part of #314 and got lost due to
pre-merging of #393

But I also will use those assumptions in an upcoming branch.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

QR Code is not readable in a tty
4 participants