Skip to content

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

Merged
merged 10 commits into from
Jan 12, 2023
Merged

Add AppIndicator/SysTray icon #4

merged 10 commits into from
Jan 12, 2023

Conversation

blipk
Copy link
Contributor

@blipk blipk commented Jan 11, 2023

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:

image

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

@zocker-160
Copy link
Owner

Thank you very much for this, I will take a look.

I'm not sure how to assign anyone or if you have a PR process here

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.

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.

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.

Copy link
Owner

@zocker-160 zocker-160 left a 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

@blipk
Copy link
Contributor Author

blipk commented Jan 11, 2023

Thanks @zocker-160 have pushed change to fix those review items.

As for the runscript, I get the following QT error:

qt.qpa.xcb: could not connect to display 
qt.qpa.plugin: Could not load the Qt platform plugin "xcb" in "" even though it was found.
This application failed to start because no Qt platform plugin could be initialized. Reinstalling the application may fix this problem.

Available platform plugins are: eglfs, linuxfb, minimal, minimalegl, offscreen, vnc, wayland-egl, wayland, wayland-xcomposite-egl, wayland-xcomposite-glx, xcb.

/usr/bin/thinkfan-ui: line 30: 89977 Aborted                 (core dumped) ${app_command}

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.

@blipk blipk requested a review from zocker-160 January 11, 2023 21:56
@zocker-160
Copy link
Owner

zocker-160 commented Jan 12, 2023

As for the runscript, I get the following QT error:

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?
I feel like we are hitting the limit of the current implementation unless there is a way to change the permission requirements for /proc/acpi/ibm/fan.

EDIT: I can run a chmod 777 on that endpoint and it works, would be a potential solution, although I am not sure if it is a desirable one.

Copy link
Owner

@zocker-160 zocker-160 left a 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.

@zocker-160
Copy link
Owner

Thank you very much!

@zocker-160 zocker-160 merged commit 88178ba into zocker-160:master Jan 12, 2023
@blipk
Copy link
Contributor Author

blipk commented Jan 12, 2023

No worries @zocker-160

The indicator does work for me on GNOME with sudo, but it has different styling.
Perhaps a QT env issue on your end? I had to mess around with kvantum theme manager at one point

EDIT: Changing the permissions on that endpoint work for me too, weird as it didn't last time I tried.
Maybe just changing the permissions on that file once with pkexec when excepting the PermissionError?

@zocker-160
Copy link
Owner

Perhaps a QT env issue on your end? I had to mess around with kvantum theme manager at one point

Yes probably, but I will avoid this environment variable dance by just running the application as user.

EDIT: Changing the permissions on that endpoint work for me too, weird as it didn't last time I tried.
Maybe just changing the permissions on that file once with pkexec when excepting the PermissionError?

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.

@blipk
Copy link
Contributor Author

blipk commented Jan 14, 2023

no worries @zocker-160

I just added changed the command when excepting the PermissionError to:

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.

@zocker-160
Copy link
Owner

zocker-160 commented Jan 14, 2023

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

if not self.checkPermissions():

which gets executed on start, so even in a use case of doing autostart, this should work

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