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

Only add color_extra controls if the keyboard supports it #12

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jdtimmerman
Copy link
Contributor

@jdtimmerman jdtimmerman commented Jan 13, 2021

I was having an error with opening the gui. It appearently crashed on loading the config for the optional 'extra' part which my particular keyboard does not have.

$ /usr/share/tuxedo-backlight-control/ui.py
Traceback (most recent call last):
  File "/usr/share/tuxedo-backlight-control/ui.py", line 349, in <module>
    init()
  File "/usr/share/tuxedo-backlight-control/ui.py", line 344, in init
    App(root)
  File "/usr/share/tuxedo-backlight-control/ui.py", line 68, in __init__
    'color_extra': tk.StringVar(self, value=backlight.color_extra.capitalize())
AttributeError: 'NoneType' object has no attribute 'capitalize'

I have tested this with my - non-extra-supporting - keyboard. Would be a good idea for someone with a fourth section to test this as well.

@jdtimmerman
Copy link
Contributor Author

Okay, so maybe not within the initial scope of this branch. But really, isn't selecting the brightness one of the most fundamental features of a backlit keyboard?

@jdtimmerman
Copy link
Contributor Author

More scope creep, i've added a button to save the settings to /etc/modprobe.d/tuxedo_keyboard.conf so the settings are applied at boot.
It takes the current keyboard values, not the selected options in the gui. For example i noticed that we actually have hex values available, while this gui only allows me to select a couple of predefined values. Implement a color selector might be my next scope-creep ;) I'll gladly split this branch in multple focussed ones if you like but for now i'll just be spitting my ideas here.

@webketje
Copy link
Owner

webketje commented Jan 18, 2021

Hey @jdtimmerman thanks a lot for your very substantial contribution. I need to review this first though, as I outlined my reasons for not including a brightness switch in tuxedocomputers/tuxedo-keyboard#12. Definitely open to providing it at least through the CLI.

The color_extra change & modprobe implementation is definitely welcome, though I am not sure the former actually makes a difference, as I think the tuxedo-keyboard driver outputs all of its kernel module param files regardless of support (but apparently in your case it didn't). If it cannot be detected at the kernel module level, it can impossibly work on a widget front-end.

My point of view is that the UI should be as straightforward as possible, but the CLI can be extended to allow more fancy developer stuff & automation. For example, this fork's commit has a nice implementation to allow HEX colors be specified through the CLI: encarsia@b2df101. IMO selecting a color is way easier and less technical than having to finetune RGB sliders, and the base colors included + option to add custom colors to a .conf file should satisfy most use cases. I would rather like to see whether it is possible to add a color square in front of the color names in the dropdowns.

As for the issue you reported (capitalize), similar variants have popped up in the issues here before, so I will add a None-safe check there in v0.8.

So for review & separate PR's:

  • 1st commit 👍 : 01d435e - good to go
  • 2nd commit: 7b8e3a6 - would like to have a separate PR that only modifies the backlight_control.py file (CLI extension) + updates the help.txt contents
  • 3rd & 4th commit 👍 : 7e8bc07 - looking good, I'd like to test this first though, and also see whether it's possible and/or makes sense to change the button into a checkbox & store the state in /etc/tuxedo-backlight-control/modprobe as 1/0.

@webketje
Copy link
Owner

@jdtimmerman I've added your first commit to master & corrected some typo's, see 01d435e

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