[DOC] make systemd-run example more reliable

[Atemu]

This is a follow-up to #1752

If the {cmd} had spaces in it, it would error out with something like

Failed to find executable gui-22345: No such file or directory

(In this case the command was
/nix/store/6jlqjighx0v0lqx9bahc7nl6npshim2r-lact-0.5.6/bin/lact gui)

This fixes that.

What this also fixes is that this would result in incredibly long names such as --unit=app-rofi--nix-store-6jlqjighx0v0lqx9bahc7nl6npshim2r\x2dlact\x2d0.5.6-bin-lact because the desktop entry contained the full path which can be quite long in Nix and is quite common in Nix packaging.

We only really care about the executable name here.

The current iteration still has command arguments in its name but getting rid of those would be quite bothersome in bash. IME apps also usually don't have command line args and if they do it's usually something short like in the example above. This could certainly be improved though.

cc @mweinelt

[Atemu]

Aaaand I already found something that breaks this: The emacsclient desktop file does this:

Exec=sh -c "if [ -n \\"\\$*\\" ]; then exec /nix/store/fsglyaxfhlavaskr159b4yyjlajy4qq5-emacs-29.4/bin/emacsclient --alternate-editor= --display=\\"\\$DISPLAY\\" \\"\\$@\\"; else exec emacsclient --alternate-editor= --create-frame; fi" sh %F

Ugh.

What we really want here is the desktop file's name.

[DaveDavenport]

What we really want here is the desktop file's name.

We could pass the desktop file full path ? would that help?

[Atemu]

That'd be immensely helpful indeed. We only need the basename but that's easy enough to extract as shown here. Although getting the name without the .desktop suffix would be rather annoying, hm.

[DaveDavenport]

A quick test, will this work?

diff --git a/include/helper.h b/include/helper.h
index ed161931..6e9a28f1 100644
--- a/include/helper.h
+++ b/include/helper.h
@@ -330,6 +330,23 @@ gboolean helper_execute_command(const char *wd, const char *cmd,
                                 gboolean run_in_term,
                                 RofiHelperExecuteContext *context);
 
+/**
+ * @param wd The work directory (optional)
+ * @param cmd The cmd to execute
+ * @param run_in_term Indicate if command should be run in a terminal
+ * @param context The startup notification context, if any
+ * @param ... tuples of extra parameters the string can search/replace
+ *
+ * Execute command.
+ * If needed members of context are NULL, they will be filled.
+ * Pass {cmd} into the va-arg list.
+ *
+ * @returns FALSE On failure, TRUE on success
+ */
+gboolean helper_execute_command_full(const char *wd, const char *cmd,
+                                     gboolean run_in_term,
+                                     RofiHelperExecuteContext *context, ...);
+
 /**
  * @param file The file path
  * @param height The wanted height
diff --git a/source/helper.c b/source/helper.c
index 53f366bf..0bf417c6 100644
--- a/source/helper.c
+++ b/source/helper.c
@@ -72,7 +72,8 @@ void cmd_set_arguments(int argc, char **argv) {
   stored_argv = argv;
 }
 
-int helper_parse_setup(char *string, char ***output, int *length, ...) {
+static int helper_parse_setup_v(char *string, char ***output, int *length,
+                                va_list ap) {
   GError *error = NULL;
   GHashTable *h;
   h = g_hash_table_new(g_str_hash, g_str_equal);
@@ -80,8 +81,6 @@ int helper_parse_setup(char *string, char ***output, int *length, ...) {
   g_hash_table_insert(h, "{terminal}", config.terminal_emulator);
   g_hash_table_insert(h, "{ssh-client}", config.ssh_client);
   // Add list from variable arguments.
-  va_list ap;
-  va_start(ap, length);
   while (1) {
     char *key = va_arg(ap, char *);
     if (key == (char *)0) {
@@ -93,7 +92,6 @@ int helper_parse_setup(char *string, char ***output, int *length, ...) {
     }
     g_hash_table_insert(h, key, value);
   }
-  va_end(ap);
 
   char *res = helper_string_replace_if_exists_v(string, h);
   // Destroy key-value storage.
@@ -115,6 +113,13 @@ int helper_parse_setup(char *string, char ***output, int *length, ...) {
   }
   return FALSE;
 }
+int helper_parse_setup(char *string, char ***output, int *length, ...) {
+  va_list ap;
+  va_start(ap, length);
+  int retv = helper_parse_setup_v(string, output, length, ap);
+  va_end(ap);
+  return retv;
+}
 
 void helper_tokenize_free(rofi_int_matcher **tokens) {
   for (size_t i = 0; tokens && tokens[i]; i++) {
@@ -1027,15 +1032,21 @@ gboolean helper_execute(const char *wd, char **args, const char *error_precmd,
 gboolean helper_execute_command(const char *wd, const char *cmd,
                                 gboolean run_in_term,
                                 RofiHelperExecuteContext *context) {
+  return helper_execute_command_full(wd, cmd, run_in_term, context, "{cmd}",
+                                     cmd, (char *)0);
+}
+
+static gboolean helper_execute_command_full_v(const char *wd, const char *cmd,
+                                              gboolean run_in_term,
+                                              RofiHelperExecuteContext *context,
+                                              va_list ap) {
   char **args = NULL;
   int argc = 0;
 
   if (run_in_term) {
-    helper_parse_setup(config.run_shell_command, &args, &argc, "{cmd}", cmd,
-                       (char *)0);
+    helper_parse_setup_v(config.run_shell_command, &args, &argc, ap);
   } else {
-    helper_parse_setup(config.run_command, &args, &argc, "{cmd}", cmd,
-                       (char *)0);
+    helper_parse_setup_v(config.run_command, &args, &argc, ap);
   }
 
   if (args == NULL) {
@@ -1063,10 +1074,19 @@ gboolean helper_execute_command(const char *wd, const char *cmd,
 
   return helper_execute(wd, args, "", cmd, context);
 }
+gboolean helper_execute_command_full(const char *wd, const char *cmd,
+                                     gboolean run_in_term,
+                                     RofiHelperExecuteContext *context, ...) {
+  va_list ap;
+  va_start(ap, context);
+  gboolean retv =
+      helper_execute_command_full_v(wd, cmd, run_in_term, context, ap);
+  va_end(ap);
+  return retv;
+}
 
 char *helper_get_theme_path(const char *file, const char **ext,
                             const char *parent_file) {
-
   char *filename = rofi_expand_path(file);
   g_debug("Opening theme, testing: %s\n", filename);
   if (g_path_is_absolute(filename)) {
diff --git a/source/modes/drun.c b/source/modes/drun.c
index c18d8f95..bdfebfb9 100644
--- a/source/modes/drun.c
+++ b/source/modes/drun.c
@@ -402,7 +402,10 @@ static void exec_cmd_entry(DRunModeEntry *e, const char *path) {
   // terminal.
   gboolean terminal =
       g_key_file_get_boolean(e->key_file, e->action, "Terminal", NULL);
-  if (helper_execute_command(exec_path, fp, terminal, sn ? &context : NULL)) {
+  if (helper_execute_command_full(exec_path, fp, terminal, sn ? &context : NULL,
+                                  "{cmd}", fp, "{desktop_file_path}", e->path,
+                                  "{app_id}", e->app_id, "{desktop_id}",
+                                  e->desktop_id, (char *)0)) {
     char *drun_cach_path = g_build_filename(cache_dir, DRUN_CACHE_FILE, NULL);
     // Store it based on the unique identifiers (desktop_id).
     history_set(drun_cach_path, e->desktop_id);

This adds:

• {desktop_file_path}
• {app_id} // filename without .desktop
• {desktop_id} // Path with all / replaced by -. -> https://specifications.freedesktop.org/desktop-entry-spec/latest/file-naming.html

Its completely untested.

[Atemu]

It is no longer completely untested.

It works! Really well actually. Rofi is now able to launch all of my apps without issue using:

-run-command "bash -c 'systemd-run --user --unit=app-rofi-\"\$(systemd-escape \"{app_id}\")\"-\$RANDOM {cmd}'"

Thank you so much!

Tested-by: Atemu <[email protected]>

Though with the slight caveat that I'm using the wayland fork based on f2c0f75. It does not apply atop the Nixpkgs package of this repo (1.7.5 tag) but that's also quite old by now.

Should I add your patch as a commit to this branch or do you just want to do that yourself?

[DaveDavenport]

Feel free to add the patch, can you update the documentation with the entries from this patch?

[Atemu]

Hm, unfortunately I found something that breaks. scrcpy.desktop has this:

# For some users, the PATH or ADB environment variables are set from the shell
# startup file, like .bashrc or .zshrc… Run an interactive shell to get
# environment correctly initialized.
Exec=/bin/sh -c "\\$SHELL -i -c scrcpy"

That seems misguided but it should work.

However $SHELL appears to be escaped entirely in this context because it fails with this:

/bin/sh: line 1: $SHELL: command not found

It works in regular rofi execution. It also works using -run-command "bash -cx '{cmd}'" and only breaks once you add systemd-run: -run-command "bash -cx 'systemd-run --user {cmd}'".

I still haven't quite understood why this happens.

This PR's recommendation isn't a regression compared to status quo though as that also fails but with a different error:

Nov 12 08:55:02 HEPHAISTOS (sh)[257995]: app-rofi--25453.service: Invalid environment variable name evaluates to an empty string: SHELL -i -c scrcpy
Nov 12 08:55:02 HEPHAISTOS sh[257995]: /bin/sh: -c: option requires an argument

[Atemu]

Another pitfall I just noticed is that this is not compatible with run of course as there's no desktop app_id it could provide.

I'll make the --unit flag optional for that case but ideally we'd have a way to fall back onto something else such as the binary name.