pgaskin / NickelMenu

The easiest way to launch scripts, change settings, and run actions on Kobo e-readers.
https://pgaskin.net/NickelMenu
MIT License
549 stars 29 forks source link

Don't notify if `cmd_spawn` succeeds? #12

Closed baskerville closed 4 years ago

baskerville commented 4 years ago

I'd rather not have a notification if cmd_spawn succeeds. (I won't mind a notification on failure tough.)

Maybe it could be left to the user to implement such a logic? Using the concepts mentioned in #11, it could be written as:

menu_item  :main  :Hello  :cmd_spawn  :echo Hello
and_then                  :dbg_toast  :The previous action was successful.
pgaskin commented 4 years ago

If I implement the concepts from #11 (which, in foresight, I should have done at the beginning, and am likely to do after v0.0.2), I'll probably rework it to work like this. The biggest thing is I probably don't want to go down the route of handling output values as well as success/failure, as I feel that is more the job of a shell script (and possible cmd_output). Otherwise, I'll add a quiet option to cmd_spawn (menu_item:main:Hello:cmd_spawn:quiet:echo Hello).

baskerville commented 4 years ago

If you were to add an option, maybe it would be better to keep cmd_spawn's argument at the same position:

menu_item:main:Hello:cmd_spawn:echo Hello:quiet

Maybe quiet should be the default? In which case, users wanting the developer feedback regarding the PID of the spawned process would add a verbose option?

shermp commented 4 years ago

I think the final option needs to remain the command. I seem to recall @geek1011 saying it was designed that way so the string could be passed directly to sh without further processing.

And that makes sense, because o can already think of a command that would cause all sorts of issues if one tried to parse an option at the end. (eg file syncing programs)

baskerville commented 4 years ago

Good point. I think there might also be a problem with options: if I have a command named verbose:echo, for example, cmd_spawn:verbose:echo Hello would be ambiguous… And that's probably why cmd_output's timeout isn't optional.

shermp commented 4 years ago

Yeah. Although making the timeout optional should be easy enough of one wanted to, by allowing -1, which according to the qprocess docs would disable the timeout. Whether @geek1011 wants to allow this is another matter.

baskerville commented 4 years ago

I would personally favor the following approach to solving this problem:

diff --git a/src/action_cc.cc b/src/action_cc.cc
index 43a179a..6be5729 100644
--- a/src/action_cc.cc
+++ b/src/action_cc.cc
@@ -297,7 +297,8 @@ NM_ACTION_(cmd_spawn) {
         (qint64*)(&pid)
     );
     NM_ASSERT(ok, "could not start process");
-    NM_RETURN_OK(nm_action_result_toast("Successfully started process with PID %lu.", (unsigned long)(pid)));
+    NM_LOG("successfully started process with PID %lu", (unsigned long)(pid));
+    NM_RETURN_OK(nm_action_result_silent());
     #undef NM_ERR_RET
 }
pgaskin commented 4 years ago

The other option is I implement the config:<key>:<value> type, which I'm going to do at some point anyways. It would set lesser-used options of the preceding menu item.

About quiet, I don't think there's any command which one would use called quiet:, so in this case, I'd probably add it directly and make it optional.

@baskerville: I understand your approach, but from previous experience it greatly confuses users when nothing happens (e.g. I answer the same emails almost every day about the fact that kepubify, kobopatch, and dictutil are CLI apps). For this reason, I want to ensure every common command does something visible, at least by default.

pgaskin commented 4 years ago

The other option would be to add another config entry which suppresses the output from the previous action.