-
Notifications
You must be signed in to change notification settings - Fork 126
Fix notify #475
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
Fix notify #475
Conversation
abed9ed to
69e1e19
Compare
69e1e19 to
e9cd5fb
Compare
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.
Pull request overview
This PR fixes notification issues by ensuring notify-send executes as the actual user rather than root, and makes the notification call asynchronous. The fix addresses issue #474 where notifications weren't appearing.
Key changes:
- Introduces
exec_user_async()to execute commands as the unprivileged user using pkexec - Refactors user ID and username lookup functions to use POSIX APIs directly instead of shell commands
- Updates notification system to use the new async user execution method
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/Utility/TeeJee.System.vala | Refactored user ID/username lookup to use POSIX APIs; changed get_user_id() to use SUDO_UID instead of SUDO_USER; simplified xdg_open() to use new exec_user_async() |
| src/Utility/TeeJee.Process.vala | Added new exec_user_async() function to execute commands as unprivileged user with pkexec wrapper when running with elevated privileges |
| src/Utility/OSDNotify.vala | Changed notification from synchronous exec_sync() to asynchronous exec_user_async() to run notify-send as the actual user |
| src/Gtk/MainWindow.vala | Connected AboutDialog's activate_link signal to xdg_open() to handle link clicks with user-level execution |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Btw, I'm not using copilot only, but it's another pair of 'eyes' and it's not terrible at finding stupid mistakes and occasionally interesting things. |
69e1e19 to
3c01e94
Compare
I have not tested this, as i didnt had time to setup fedora and reproduce the problem, but this shouldFix #474
(EDIT: I wasnt able to reproduce the initial issue, but my change helped some notifications to appear at all)
I branched from one of my other branches (#459) as i improved the handling of commands, that should not be run by root, what i needed here. So this branch shares some commits with #459 but they should not conflict and can be merged in any order (i will fix all conflicts otherwise)
The center of this fix is executing "notify-send" as user and not as root. But i also made the call async.