Fix screensaver TTE import on Python updates#4530
Fix screensaver TTE import on Python updates#4530skibidiandulka wants to merge 12 commits intobasecamp:devfrom
Conversation
|
Im a begginer, but I thought that I cant make anything wrong by trying. I think that this could be the fix? I am open to any critisism even if my request doesnt get to the final version. Btw OMARCHY community has been the best one so far!! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b9ac433a3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Fall back to running the module via python3 -m. | ||
| if python3 -m terminaltexteffects --version >/dev/null 2>&1; then | ||
| tte() { python3 -m terminaltexteffects "$@"; } | ||
| return 0 | ||
| fi |
There was a problem hiding this comment.
When the tte binary isn’t present, this defines tte() to run python3 -m terminaltexteffects. The spawned process name is python3, so later pgrep -t ... -x tte and pkill -x tte no longer match. In that environment the inner loop exits immediately, the outer loop keeps spawning new screensaver processes, and exit_screensaver won’t terminate the running effect. Consider tracking the PID of the background process or matching terminaltexteffects instead of the tte name.
Useful? React with 👍 / 👎.
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22b709de16
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if python3 -m terminaltexteffects --version >/dev/null 2>&1; then | ||
| tte() { python3 -m terminaltexteffects "$@"; } | ||
| return 0 |
There was a problem hiding this comment.
When tte is defined as a shell function for the python fallback, running tte … & backgrounds a subshell, so $! (and thus TTE_PID) refers to that subshell, not the python3 process. In exit_screensaver, killing TTE_PID will stop the subshell but can leave the python process running, so the screensaver may keep running after input. This only happens when tte is not a real binary and the python module path is used. Consider using exec inside the function (e.g., tte() { exec python3 -m terminaltexteffects "$@"; }) so the PID corresponds to the actual process or otherwise capture the python PID directly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Codex Review: Didn't find any major issues. Keep it up!
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f531e35a93
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
bin/omarchy-cmd-screensaver
Outdated
| fi | ||
|
|
||
| while pgrep -t "${tty#/dev/}" -x tte >/dev/null; do | ||
| while kill -0 "$TTE_PID" 2>/dev/null; do |
There was a problem hiding this comment.
The loop now relies on kill -0 $TTE_PID to detect whether the screensaver effect is still running. If tte exits and its PID gets quickly reused by another process (e.g., on a busy system), kill -0 will keep succeeding, so the loop won’t restart the effect and the screensaver can stay stuck on a blank window until input or focus change. The prior pgrep -t ... -x tte avoided PID reuse by checking the process name. Consider validating the command name (e.g., pgrep -x -p $TTE_PID tte) or using wait to detect termination.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e552863108
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| tte_is_alive() { | ||
| [[ -n "${TTE_PID:-}" ]] || return 1 | ||
| [[ -r "/proc/$TTE_PID/cmdline" ]] || return 1 | ||
| kill -0 "$TTE_PID" 2>/dev/null || return 1 | ||
| tr '\0' ' ' <"/proc/$TTE_PID/cmdline" | rg -q 'terminaltexteffects|/usr/bin/tte' | ||
| } |
There was a problem hiding this comment.
tte_is_alive pipes the cmdline through rg, but this script doesn’t otherwise require ripgrep and rg isn’t referenced elsewhere in bin/. On systems without ripgrep installed, the pipeline exits non‑zero, so the first tte_is_alive call after launch fails and the screensaver exits immediately. This only happens on machines missing rg, but it makes the screensaver unusable there. Consider using grep -Eq (available in base systems) or guarding the rg call.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f13436f8c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
bin/omarchy-cmd-screensaver
Outdated
| tte_is_alive() { | ||
| [[ -n "${TTE_PID:-}" ]] || return 1 | ||
| [[ -r "/proc/$TTE_PID/cmdline" ]] || return 1 | ||
| kill -0 "$TTE_PID" 2>/dev/null || return 1 | ||
| tr '\0' ' ' <"/proc/$TTE_PID/cmdline" | grep -Eq 'terminaltexteffects|/usr/bin/tte' |
There was a problem hiding this comment.
The liveness check only matches terminaltexteffects or the literal /usr/bin/tte in /proc/$TTE_PID/cmdline. If the tte entrypoint is installed somewhere else (e.g., /usr/local/bin/tte via pip, or a venv), its command line won’t contain either substring even though the process is healthy. In that case tte_is_alive returns false and the loop immediately calls exit_screensaver, so the screensaver exits right after launch on those installs. Consider loosening the match (e.g., matching tte basename or trusting kill -0) so non-/usr/bin installs don’t get treated as dead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab62027d68
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
bin/omarchy-cmd-screensaver
Outdated
| tte_is_alive() { | ||
| [[ -n "${TTE_PID:-}" ]] || return 1 | ||
| [[ -r "/proc/$TTE_PID/cmdline" ]] || return 1 | ||
| kill -0 "$TTE_PID" 2>/dev/null || return 1 | ||
| tr '\0' ' ' <"/proc/$TTE_PID/cmdline" | grep -Eq 'terminaltexteffects|/tte(\s|$)' |
There was a problem hiding this comment.
tte cmdline to avoid false exits
The new liveness check only matches terminaltexteffects or /tte, but when tte is installed as a system command its /proc/$PID/cmdline typically contains just tte (argv[0] without a slash). In that common setup, tte_is_alive returns false even while the screensaver is running, so the loop immediately calls exit_screensaver after the 0.1s sleep. Consider matching a bare tte token as well (e.g., (^|\s)tte(\s|$) or (^|/)tte).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5939680f2b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| exit_screensaver() { | ||
| cleanup_cursor | ||
| if [[ -n "${TTE_PID:-}" ]]; then | ||
| kill "$TTE_PID" 2>/dev/null || true | ||
| else |
There was a problem hiding this comment.
If tte exits unexpectedly, tte_is_alive can return false because /proc/$TTE_PID/cmdline no longer matches, but exit_screensaver will still kill "$TTE_PID" without verifying that PID still belongs to tte. On a busy system the PID can be recycled quickly, so this can terminate an unrelated process when the screensaver exits after a crash. Consider re-validating the cmdline before killing or falling back to a name-based kill only when the PID still matches tte.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: caae6f5acf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
bin/omarchy-cmd-screensaver
Outdated
| else | ||
| pkill -x tte 2>/dev/null || true | ||
| pkill -f terminaltexteffects 2>/dev/null || true |
There was a problem hiding this comment.
When the screensaver's TTE process exits early (e.g., tte fails to start), exit_screensaver falls back to pkill -f terminaltexteffects, which will terminate any other user process running python -m terminaltexteffects outside the screensaver. This is a regression from the prior pkill -x tte behavior and can disrupt unrelated sessions whenever the screensaver fails to start or the PID check fails. Consider scoping the kill to the recorded PID or the current tty/session instead of a global pkill -f.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Codex Review: Didn't find any major issues. Keep it up!
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Codex Review: Didn't find any major issues. Nice work!
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
skibidiandulka
left a comment
skibidiandulka
left a comment
There was a problem hiding this comment.
@dhh I think my pull request should be ready to go. I believe this should solve the screensaver not working for most people because of the conflicting python versions. If there are any issues with the code I am more than happy to correct it!!
Hey, this looks great, but AFAIK, someone already opened a PR #4287 for this, and the fix will be implemented in the next release of omarchy. Pretty sure the issue is just a python version mismatch, although there are alot of other nice safeguards in here that could be implemented in a seperate PR |
|
We've fixed the immediate issue by updating the python-terminaltexteffects package. Going to review what the best next step here is. Maybe we just go all the way to a binary. |
3 participants
Description:
This PR makes the Omarchy screensaver resilient to Python updates that break the tte entrypoint. We now:
This prevents immediate exits, stuck invisible cursors, and crashes when the module is present but the tte binary is out of sync
with the system Python.