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

change default qr brightness #380

Merged
merged 1 commit into from
Jul 31, 2023
Merged

Conversation

pythcoiner
Copy link
Contributor

as requested in #377

@kdmukai
Copy link
Contributor

kdmukai commented Jun 1, 2023

I wonder if we should define some kind of structure that explicitly sets the different possible brightness levels.

This PR would then set the default as BRIGHTNESS__2 or something.

The current implementation isn't terrible but isn't exactly elegant. Currently allows for 7 brightness ranges: 256/32 (the missing 8th value would be pure black), shifted by one (sorta; I think there's a tiny bug there) for the zero-indexing.

see: https://github.com/SeedSigner/seedsigner/blob/dev/src/seedsigner/gui/screens/screen.py#L714-L720

Some simple helpers like increase_qr_brightness and decrease_qr_brightness can hide away most of the details, even if it's still just doing the +/- 32 under the hood.

Just spitballing out loud here. Not sure yet what the ideal refactor would look like.

@jdlcdl
Copy link

jdlcdl commented Jul 30, 2023

I second Keith's comment above. Maybe just a soft rule for devs like "default value+1 mod 32 == 0" (or mod something reasonable, the step-up/step-down is currently 31.... Huh? so I chose that as an example, no clue where 189 as a default came from...Huh? (prolly an acute human eye that said "Right about... a lil dimmer... not that much... RIGHT THERE!!!")
Other devs should chime in, my first thought was "multiples of 32", knowing that QRDisplayScreen defines the edges at 31 and 255.

I'll add that our test suite currently covers all default values, so @pythcoiner, without an edit to the bottom of tests/test_controller.py this pr would fail our test suite.

Otherwise, a dimmer default qr_brightness get's my ACK, I've never heard of anyone NOT having to dim qr-codes.

SpecialNote: this pr plays well with pr #416 (after correcting the test_suite noted above).

@pythcoiner
Copy link
Contributor Author

I thought this change had been canceled because the new hint for notice brightness is changeable by thumbstick?

@kdmukai
Copy link
Contributor

kdmukai commented Jul 30, 2023

@pythcoiner my guess is that typically bad webcams will still perform better at a lower default brightness.

So I'm all for improving the default experience (this PR) AND ensuring the user is aware of how to further adjust it.

@newtonick
Copy link
Collaborator

ACK Tested

@newtonick newtonick merged commit 69b3122 into SeedSigner:dev Jul 31, 2023
@newtonick newtonick added this to the 0.7.0 milestone Jul 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

4 participants