Merged
Conversation
abed9ed to
69e1e19
Compare
Merged
69e1e19 to
e9cd5fb
Compare
There was a problem hiding this comment.
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.
Member
|
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.