Rewrite qubes-i3-sensible-terminal#24
Conversation
This allows for choosing the user to launch the terminal as (such as root) Fixes: qubes-issues/10656 Signed-off-by: Atrate <Atrate@protonmail.com>
When installed they get marked as executable anyways, but this makes it consistent with the other file in the repo which was marked as exec. Signed-off-by: Atrate <Atrate@protonmail.com>
|
Suggestion: If you intend for this pull request to resolve the associated issue and would like for it to be linked to the issue automatically, you can put |
Thanks, I wasn't fully sure where (whether) to put the |
|
It was a POSIX shellscript and now it is |
OpenQA test summaryComplete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2026041101-4.3&flavor=pull-requests Test run included the following:
New failures, excluding unstableCompared to: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2026032404-devel&flavor=update
Failed tests18 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/170766#dependencies 31 fixed
Unstable testsDetails
Performance TestsPerformance degradation:9 performance degradations
Remaining performance tests:102 tests
|
| "$arg_user:QUBESRPC qubes.StartApp+$DOMU_TERMINAL $vm" | ||
| fi | ||
|
|
||
| # If all else fails, just execute the service for the terminal |
There was a problem hiding this comment.
I don't like this comment, or that there are comments that don't explain difficult things on other sections, but this comment is misleading. It is not "If all else fails", this case happens if you are using a GUIVM, and therefore you can't run qrexec-client on it (which is faster than qvm-run). Which, okay, it is when previous cases doesn't match, but this is different than explaining why such thing happens.
Minor thing. Theoretically, this could also be </dev/null >/dev/null qrexec-client-vm -- "$vm" "qubes.StartApp+$DOMU_TERMINAL", but this commands holds unless you use &, which is not always nice.
There was a problem hiding this comment.
About comment style guide: https://doc.qubes-os.org/en/r4.3/developer/code/coding-style.html#general-programming-style-guidelines
Use comments to explain non-trivial code fragments, or expected behavior of more complex functions, if it is not clear from their name.
Do not use comments for code fragments where it is immediately clear what the code does.
There was a problem hiding this comment.
comments
I've removed some comments that could be deemed redundant. "Immediately clear" is a difficult thing to define, so I've taken the approach of imagining I'm someone who knows programming but not necessarily bash's constructs, so I've left e.g. the comment about set -eEu or the comments in the if...exec clauses, as it's not immediately clear that the exec makes this into, effectively, one big if-elif-else construct.
Minor thing. Theoretically, this could also be </dev/null >/dev/null qrexec-client-vm -- "$vm" "qubes.StartApp+$DOMU_TERMINAL", but this commands holds unless you use &, which is not always nice.
What does adding null redirection help with in this case? Not polluting the DE's logs? Security? Could there be a blocked pipe problem with qrexec here if implemented?
marmarek
left a comment
marmarek
left a comment
There was a problem hiding this comment.
On top of what @ben-grande said, you can make also one possible improvement (but it's fine to leave this alone for later).
|
|
||
| # Extract VM name from active window props | ||
| local vm | ||
| vm="$(xprop -id "$id" | grep '_QUBES_VMNAME(STRING)')" || true |
There was a problem hiding this comment.
| vm="$(xprop -id "$id" | grep '_QUBES_VMNAME(STRING)')" || true | |
| vm="$(xprop -id "$id" '_QUBES_VMNAME')" || true |
There was a problem hiding this comment.
I tried this approach and ran into an issue. It looks like xprop emits both outputs and error messages to stdout, disregarding stderr. Unless there's a way to make it work properly (or I'm mistaken), I feel like the post-processing of the output with grep would be the better approach, sadly.
There was a problem hiding this comment.
Very weird behavior. To test, make it fail by focusing a dom0 window before running the script, notice that redirecting stderr to dev null doesn't hide errors.
There was a problem hiding this comment.
FWIW, I also get an error here when running the script on xfce. Apparently, Xfce sets two window IDs in the _NET_ACTIVE_WINDOW property, the second one being 0x0. And xprop fails on it, making the script always assume it's dom0.
But it does work correctly for me on i3.
https://doc.qubes-os.org/en/r4.3/developer/code/coding-style.html#general-programming-style-guidelines Signed-off-by: Atrate <Atrate@protonmail.com>
Atrate
force-pushed
the
feat/terminal-rewrite
branch
from
56961f0 to
d237256
Compare
|
@ben-grande I've applied your suggestions and amended the most recent commit. |
GuiVM -> GUIVM, as recommended by @ben-grande in QubesOS/qubes-desktop-linux-i3-settings-qubes#24 (comment)
5 participants
This allows for choosing the user to launch the terminal as (such as root)
All bugs, errors and questionable stylistic choices are mine, not an LLMs.
Closes: QubesOS/qubes-issues#10656
CC: @ben-grande
Signed-off-by: Atrate Atrate@protonmail.com