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

chore(core): add missing parameter to set_brightness function #4411

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

chore(core): add missing parameter to set_brightness function #4411

wants to merge 2 commits into from
+46 −9

Conversation

bieleluk
Copy link
Contributor

@bieleluk bieleluk commented Nov 29, 2024 •
edited
Loading

This PR:

  1. adds the missing brightness value parameter in set_brightness function
  2. If the value has been passed, the brightness is set to this value
  3. parametrizes existing test to test different scenarios

@bieleluk bieleluk linked an issue Nov 29, 2024 that may be closed by this pull request
Missing brightness value parameter #4410
Open
@bieleluk bieleluk self-assigned this Nov 29, 2024
@github-actions GitHub Actions
Copy link

legacy UI changes device test(screens) main(screens)

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 29, 2024 •
edited
Loading

core UI changes device test click test persistence test
T2T1 Model T test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
T3B1 Safe 3 test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
T3T1 Safe 5 test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
All main(screens)

@bieleluk bieleluk added T2T1 Trezor Model T T3T1 Trezor Safe 5 bug Something isn't working as expected low hanging fruit Simple, quick task. labels Nov 29, 2024
@bieleluk bieleluk marked this pull request as ready for review December 2, 2024 11:15
@bieleluk bieleluk requested a review from matejcik as a code owner December 2, 2024 11:15
@bieleluk bieleluk requested review from matejcik and removed request for matejcik December 2, 2024 11:16
@matejcik
Copy link
Contributor

matejcik commented Dec 2, 2024

ack on code, please fix ui tests

obrusvit
obrusvit reviewed Dec 2, 2024
View reviewed changes
Copy link
Contributor

@obrusvit obrusvit left a comment

Choose a reason for hiding this comment

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

I actually don't think this fixes the issue, does it?

If you pass the value to the set_brightness dialog, it merely starts the UI with the value. But the first real change happens on NumberInputSliderDialogMsg::Changed which happens only if you change the number.

@bieleluk
Copy link
Contributor Author

bieleluk commented Dec 3, 2024 •
edited
Loading

You are right, @obrusvit
My PR just adds a missing function parameter but does not make any functional changes.
If it is not sufficient, I can implement something like the following:

  • If the parameter is a valid number, then the user will only get the confirmation screen

@bieleluk bieleluk requested a review from prusnak as a code owner December 9, 2024 14:39
@bieleluk
Copy link
Contributor Author

bieleluk commented Dec 9, 2024

@obrusvit
This commit addresses your concern 0537f0c

@bieleluk bieleluk requested review from obrusvit and removed request for prusnak December 12, 2024 10:24
@bieleluk bieleluk removed the low hanging fruit Simple, quick task. label Dec 16, 2024
@bieleluk
Copy link
Contributor Author

Rebasing due to conflicts from the latest changes

@bieleluk bieleluk force-pushed the bieleluk/brightnes-param branch from 0537f0c to 5091f3e Compare December 19, 2024 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matejcik matejcik Awaiting requested review from matejcik matejcik is a code owner

@obrusvit obrusvit Awaiting requested review from obrusvit

At least 1 approving review is required to merge this pull request.

Assignees

@bieleluk bieleluk

Labels
bug Something isn't working as expected T2T1 Trezor Model T T3T1 Trezor Safe 5
Projects
Firmware
Status: 🔎 Needs review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Missing brightness value parameter
3 participants
@bieleluk @matejcik @obrusvit