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

Fix some issues with qrcode progress #441

Merged
merged 12 commits into from
Aug 12, 2024

Conversation

tadeubas
Copy link
Contributor

@tadeubas tadeubas commented Aug 10, 2024

Description

  • pMofN would start with 1 index offset and end 1 index after the screen size
  • Whole width of the device were not used in some edge cases (ex.: yahboom 240px // 62 parts would give 3px for each bar and would end using just 186px (3*62) of the screen for the progress bar)
  • Touch / Enter to capture on "New mnemonic > Camera" working a lot better now!
  • Fixed some rare cases when an exception could occur in a menu item that uses lcd as landscape and the lcd would remain landscape breaking the menu screen
  • Refactored the code a little to favor reuse
  • Little optimizations
  • Yahboom v1.1 now behaves as v1.0 the button acts like PAGE and not PAGE_PREV
  • UR PSBT progress bar QR now highlight when new part is scanned (now it behaves more like BBQR and pMofN)
  • Removed the translation "Calculating Shannon's entropy" in favor of reusing "Processing..." as we already use in other screens
  • pyproject.toml: new missing dependency for pylint: dill
  • Updated poetry.lock file
  • Display was being created with self.portrait = True but it only changed to portrait mode after the call to initialize_lcd()

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Copy link

codecov bot commented Aug 10, 2024

Codecov Report

Attention: Patch coverage is 80.55556% with 14 lines in your changes missing coverage. Please review.

Project coverage is 94.19%. Comparing base (19f99ec) to head (96983bf).

Files Patch % Lines
src/krux/display.py 44.44% 10 Missing ⚠️
src/krux/pages/qr_capture.py 92.30% 2 Missing ⚠️
src/krux/camera.py 66.66% 1 Missing ⚠️
src/krux/input.py 88.88% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #441      +/-   ##
===========================================
- Coverage    94.31%   94.19%   -0.13%     
===========================================
  Files           58       58              
  Lines         7146     7153       +7     
===========================================
- Hits          6740     6738       -2     
- Misses         406      415       +9     

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

@tadeubas
Copy link
Contributor Author

tadeubas commented Aug 11, 2024

I am doing some final adjustments, I will place it as draft for now

Follow attached some QR code examples with edge cases for 240px width devices (e.g. yahboom):

bbqr22.mp4
pmofn.mp4

@tadeubas tadeubas marked this pull request as draft August 11, 2024 11:28
@odudex
Copy link
Member

odudex commented Aug 11, 2024

Great improvements!
But it has adjustments to do, I saw you told you're aware off, but in case you missed one of these:

  1. The progress bars have random holes in it.
    image

  2. I did not observe any alteration in the entropy capture touch behavior; what could be the mechanism behind the change? Additionally, the display's clear() at the beginning is needed.
    image

@tadeubas
Copy link
Contributor Author

tadeubas commented Aug 11, 2024

Thanks for the review!

  1. The progress bars have random holes in it.

Yes, this is fixed now with 136b228

  1. I did not observe any alteration in the entropy capture touch behavior

I've noticed a change in the Yahboom tap behavior, before I would tap 5 times or more to get the entropy capture, now I tested it and the worst case was 3 taps, maybe it's just my impression...

Additionally, the display's clear() at the beginning is needed.

Sorry, I didn't test it on Amigo, so I didn't know about the text in the back. clear() returned with 3f9e6be

@tadeubas tadeubas marked this pull request as ready for review August 11, 2024 22:15
@odudex
Copy link
Member

odudex commented Aug 12, 2024

Great improvements! Thank you!

@odudex odudex merged commit b9e840f into selfcustody:develop Aug 12, 2024
6 of 7 checks passed
@tadeubas tadeubas deleted the fix-qrcode-progress branch September 4, 2024 07:18
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.

2 participants