-
Notifications
You must be signed in to change notification settings - Fork 14
Add AppIndicator/SysTray icon #4
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
Conversation
Thank you very much for this, I will take a look.
All good, since this is a very small project, I don't really have any PR process set up, I actually never expected a PR xD.
So does the start script not work properly? This part is kinda tricky and I am using the same script that is also used for gparted and timeshift and in my testing it did work, although the theming did break during my testing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
review done, few suggestions and one blocker
Thanks @zocker-160 have pushed change to fix those review items. As for the runscript, I get the following QT error:
If I run it with sudo rather than pkexec it works fine, but then I have to have that sudo terminal section active somewhere. I don't mind too much, it's likely a distro configuration issue. |
…/or window are open
Ok weird, it did work just fine for me, but that being said, there seems to be another issue that I just noticed. When running the entire application as root (using sudo), the tray icon does not show up. Does it work for you? EDIT: I can run a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am willing to merge this, although the sudo issue will require a solution before I can push a new version out, as it currently will neither show a window nor the tray icon, when run using sudo on KDE,
EDIT: Alright, I think I will go down the kdesu route to reduce the required permission for writing the fan endpoint and let the application itself run with normal privileges.
It would actually even allow me to package this as a Flatpak.
Thank you very much! |
No worries @zocker-160 The indicator does work for me on GNOME with sudo, but it has different styling. EDIT: Changing the permissions on that endpoint work for me too, weird as it didn't last time I tried. |
Yes probably, but I will avoid this environment variable dance by just running the application as user.
Yep that is exactly what I am going to do and then I can also get rid of the sudo and pkexec fiddling in the start script. |
no worries @zocker-160 I just added changed the command when excepting the cmd = [f"""pkexec python -c \"import os; os.chmod('{PROC_FAN}', 0o777)
with open('{PROC_FAN}', 'w+') as soc: soc.write('level {speed}')\""""] See - master...blipk:thinkfan-ui:master This works well as I found that the file permissions where getting reset by something any time I rebooted. |
this case should be covered by Line 52 in 914e160
which gets executed on start, so even in a use case of doing autostart, this should work |
Hey thanks for this wrapper around the thinkfan fan control, it works great and I use it a lot.
I was finding that it was a bit much having the window open all the time though, so I set up a
QSystemTrayIcon
on my panel like so:Clicking any of the temp or fan readings will open up your main UI, or fan speeds can be set from the indicators menu without opening the main UI.
Could possibly add a couple of command line arguments to run in either mode if you would prefer.
Let me know if there's anything you want changed to get this merged.
EDIT:
I also added in so it makes a call to pkexec to prompt for sudo password if it's needed for setting the fan speed.
This is useful as I wasn't able to run it as root without a terminal due to missing QT env arrangements.
@zocker-160 I'm not sure how to assign anyone or if you have a PR process here