Conversation
- Changed to image_path to match LogoScreen (PR #XXX). - Introduced is_active flag for better state tracking. - Added clear, friendly comments to guide future edits.
|
The same thing is handled by previous PR, check this out #729 |
Hi @Advaitgaur004 , Thank you so much for your helpful feedback! I’ve been working on the Code Cleanup project in small steps—my PR #730 for OpeningSplashScreen added self.image_path and and a universal flag is_active to improve consistency and tracking. I was planning a next PR to refactor ScreensaverScreen with the same approach, plus clearer comments like ‘Move logo with random increments for screensaver effect. I want to make sure I’m on the right track—does this sound good, or should I adjust my focus? I’d really appreciate your guidance! Best, |
|
Hello @kdmukai ! |
| """Initialize the logo display screen with the SeedSigner logo image path, partner logos, and active state.""" | ||
| super().__init__() | ||
| self.logo = load_image("logo_black_240.png") | ||
| self.image_path = load_image("logo_black_240.png") # Renamed for consistency |
There was a problem hiding this comment.
load_image returns a PIL.Image object. This proposed rename confuses what is being operated on in later calls.
|
NACK. I am always in favor of clear comments, but the additions here are only modestly helpful or read like AI text and many are just unnecessary. I would prefer to see the refactoring in this area be completed in PR #718 first. Then perhaps a follow up PR could be issued if there are further improvements to make. |
4 participants
Description
Hi team!
I’ve refactored the OpeningSplashScreen class to align better with SeedSigner’s overall code structure and make it more maintainable for contributors. Here’s a quick summary of the improvements:
Why These Changes Matter:
These updates support SeedSigner’s goals of:
This builds on my previous PR #728 , and I’m continuing to make steady progress.
I’ve verified that the code syntax is clean on my Windows machine. Full functionality testing will require a Raspberry Pi due to the spidev dependency.
This pull request is categorized as a:
Checklist
pytestand made sure all unit tests pass before submitting the PRIf you modified or added functionality/workflow, did you add new unit tests?
I have tested this PR on the following platforms/os:
spidevissue))Note: Keep your changes limited in scope; if you uncover other issues or improvements along the way, ideally submit those as a separate PR. The more complicated the PR the harder to review, test, and merge.